-
Notifications
You must be signed in to change notification settings - Fork 10.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build(python): Package scripts with pep-0517 compliance #5745
Conversation
Would appreciate a review here. I can rebase to resolve conflicts once a review begins. |
Update script names in |
fb6dd20
to
39f15ec
Compare
39f15ec
to
db3b069
Compare
9e836a1
to
bc0f159
Compare
Rebased, resolved conflicts and addressed a minor review point. Requesting review again |
scripts/check-requirements.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is moving fast, jaja, the web ui already shows conflicts 🙃.
I'll look at this tonight.
Personally, I still think that the stupid-simple setuptools backend (used with pyproject.toml) is preferable over poetry, but maybe that's just me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think that the stupid-simple setuptools backend (used with pyproject.toml) is preferable over poetry
I definitely second this thought as it is easier to install directly using pip
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I like that with poetry we get a lockfile, which is nice to have for executables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
easier to install directly using pip.
I believe pip install
and python -m build
still work with the poetry
backend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is moving fast, jaja, the web ui already shows conflicts 🙃.
Fast indeed :'( resolved again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On further thought, it actually makes sense now to not include gguf-py. I had a misguided notion that the poetry build included gguf-py, but of course an isolated package is not part of its job. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to push a pdm migration, but it seems to break scripts in dev virtual environments, all other things being the same as before.
❯ cat --plain $(which llama-convert-hf-to-gguf)
#!/home/dt/ghq/github.com/ggerganov/llama.cpp/.venv/bin/python
import re
import sys
from convert_hf_to_gguf import main
if __name__ == "__main__":
sys.argv[0] = re.sub(r"(-script\.pyw|\.exe)?$", "", sys.argv[0])
sys.exit(main())
❯ /home/dt/ghq/github.com/ggerganov/llama.cpp/.venv/bin/python -c 'from convert_hf_to_gguf import main ; print("ok")'
ok
❯ llama-convert-hf-to-gguf # why
Traceback (most recent call last):
File "/home/dt/ghq/github.com/ggerganov/llama.cpp/.venv/bin/llama-convert-hf-to-gguf", line 5, in <module>
from convert_hf_to_gguf import main
ModuleNotFoundError: No module named 'convert_hf_to_gguf'
Any idea what it could be doing wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears there's a difference in the effective sys.path
for scripts installed by poetry vs pdm, which explains why this failure occurs. pdm
really wants an src
layout even though the includes are relative to root in our case:
❯ llama-convert-hf-to-gguf
sys.path: ['/home/dt/ghq/github.com/ggerganov/llama.cpp/.venv/bin', '/home/dt/.local/share/mise/installs/python/3.11/lib/python311.zip', '/home/dt/.local/share/mise/installs/python/3.11/lib/python3.11', '/home/dt/.local/share/mise/installs/python/3.11/lib/python3.11/lib-dynload', '/home/dt/ghq/github.com/ggerganov/llama.cpp/.venv/lib/python3.11/site-packages', '/home/dt/ghq/github.com/ggerganov/llama.cpp/src']
Traceback (most recent call last):
File "/home/dt/ghq/github.com/ggerganov/llama.cpp/.venv/bin/llama-convert-hf-to-gguf", line 7, in <module>
from convert_hf_to_gguf import main
ModuleNotFoundError: No module named 'convert_hf_to_gguf'
❯ readlink .venv/bin/python
/home/dt/.local/share/mise/installs/python/3.11/bin/python
I'm not entirely sure how pdm manages the syspath even with a direct interpreter invocation though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I mostly care about the build-system aspect of pdm/poetry, not about editable installs (which are nearly universally broken everywhere), and not about poetry/pdm managing environments: if you pip wheel
the llama.cpp
repo (delegating either to poetry or to pdm), and manually install the wheel in a manually created venv, does convert_hf_to_gguf
correctly end up in the respective site-packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works fine with both both poetry and pdm. I've opened a ticket for the other issue in pdm-project/pdm#2996. Will follow up with a migration PR once that's resolved.
bc0f159
to
4b05bf7
Compare
There is again a trivial conflict here. I'll resolve it once the PR is otherwise ready to merge, looking forward to an approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ggerganov objections to merging?
❯ pip wheel git+https://github.com/ditsuke/llama.cpp.git@build/python/pip-0517
❯ unzip -l llama_cpp_scripts-0.0.0-py3-none-any.whl
Archive: llama_cpp_scripts-0.0.0-py3-none-any.whl
Length Date Time Name
--------- ---------- ----- ----
146465 01-01-1980 00:00 convert_hf_to_gguf.py
13600 01-01-1980 00:00 convert_hf_to_gguf_update.py
18993 01-01-1980 00:00 convert_llama_ggml_to_gguf.py
33717 01-01-1980 00:00 llama_cpp_scripts-0.0.0.dist-info/AUTHORS
1078 01-01-1980 00:00 llama_cpp_scripts-0.0.0.dist-info/LICENSE
59708 01-01-1980 00:00 llama_cpp_scripts-0.0.0.dist-info/METADATA
88 01-01-1980 00:00 llama_cpp_scripts-0.0.0.dist-info/WHEEL
194 01-01-1980 00:00 llama_cpp_scripts-0.0.0.dist-info/entry_points.txt
793 01-01-2016 00:00 llama_cpp_scripts-0.0.0.dist-info/RECORD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not familiar with poetry
, but I think it's fine to merge
Not namespaced though :(
7134924
to
4c6f3d6
Compare
Rebased |
Thanks for merging |
@SomeoneSerge We prefer to squash-merge all PRs, even when the individual commits are well organized |
@ggerganov Acknowledged. I rushed a bit to sneak in before new conflicts |
[tool.poetry.dependencies] | ||
python = ">=3.9" | ||
numpy = "^1.25.0" | ||
sentencepiece = ">=0.1.98,<0.2.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, would you have time to sync and/or relax the constraints?
#7246 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure
Gist
Packages the python scripts in this repo in a PEP 517-compliant way.
Other changes:
entrypoints declared in
pyproject.toml
.Ref: #5664