Skip to content
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

Merged
merged 8 commits into from
Jul 4, 2024

Conversation

ditsuke
Copy link
Contributor

@ditsuke ditsuke commented Feb 27, 2024

Gist

Packages the python scripts in this repo in a PEP 517-compliant way.
Other changes:

  • Script names had to be converted to snake_case so they could be used as modules via
    entrypoints declared in pyproject.toml.

Ref: #5664

@ditsuke
Copy link
Contributor Author

ditsuke commented Mar 10, 2024

Would appreciate a review here. I can rebase to resolve conflicts once a review begins.

@ggerganov
Copy link
Owner

Update script names in ci/run.sh to use underscores and check other places that might need updates too

@ditsuke ditsuke force-pushed the build/python/pip-0517 branch 2 times, most recently from fb6dd20 to 39f15ec Compare March 10, 2024 18:31
@mofosyne mofosyne added Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix python python script changes refactoring Refactoring help wanted Extra attention is needed labels May 10, 2024
pyproject.toml Outdated Show resolved Hide resolved
@github-actions github-actions bot added script Script related devops improvements to build systems and github actions build Compilation issues examples labels Jul 2, 2024
@ditsuke ditsuke force-pushed the build/python/pip-0517 branch 2 times, most recently from 9e836a1 to bc0f159 Compare July 2, 2024 10:20
@ditsuke ditsuke requested a review from lsetiawan July 2, 2024 10:35
@ditsuke
Copy link
Contributor Author

ditsuke commented Jul 2, 2024

Rebased, resolved conflicts and addressed a minor review point. Requesting review again

Copy link
Collaborator

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.

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

@ditsuke ditsuke Jul 2, 2024

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

Copy link
Contributor Author

@ditsuke ditsuke Jul 4, 2024

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. 😅

Copy link
Contributor Author

@ditsuke ditsuke Jul 4, 2024

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?

Copy link
Contributor Author

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

Copy link
Collaborator

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?

Copy link
Contributor Author

@ditsuke ditsuke Jul 4, 2024

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.

@ditsuke
Copy link
Contributor Author

ditsuke commented Jul 3, 2024

There is again a trivial conflict here. I'll resolve it once the PR is otherwise ready to merge, looking forward to an approval.

Copy link
Collaborator

@SomeoneSerge SomeoneSerge left a 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

Copy link
Owner

@ggerganov ggerganov left a 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

CMakeLists.txt Outdated Show resolved Hide resolved
@ditsuke ditsuke changed the title build(python): Package scripts with pip-0517 compliance build(python): Package scripts with pep-0517 compliance Jul 4, 2024
@ditsuke
Copy link
Contributor Author

ditsuke commented Jul 4, 2024

Rebased

@SomeoneSerge SomeoneSerge merged commit 51d2eba into ggerganov:master Jul 4, 2024
50 of 53 checks passed
@ditsuke
Copy link
Contributor Author

ditsuke commented Jul 4, 2024

Thanks for merging

@ggerganov
Copy link
Owner

@SomeoneSerge We prefer to squash-merge all PRs, even when the individual commits are well organized

@SomeoneSerge
Copy link
Collaborator

@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"
Copy link
Collaborator

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Compilation issues devops improvements to build systems and github actions examples help wanted Extra attention is needed python python script changes refactoring Refactoring Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix script Script related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants