llama.cpp icon indicating copy to clipboard operation
llama.cpp copied to clipboard

gguf-py : avoid requiring PySide6 for packaged scripts

Open compilade opened this issue 8 months ago • 4 comments

I'm using Nix devShells for my development, most often with nix develop .#default-extra.

Problem

I wanted to use gguf-dump with some model using the wrapper which that devShell puts in the $PATH, and was greeted with

$ gguf-dump --help
Traceback (most recent call last):
  File "/nix/store/l06dc60pbanrbm5ksvf0wh3n2q9blw4z-python3.12-gguf-0.0.0/bin/.gguf-dump-wrapped", line 6, in <module>
    from gguf.scripts import gguf_dump_entrypoint
  File "/nix/store/l06dc60pbanrbm5ksvf0wh3n2q9blw4z-python3.12-gguf-0.0.0/lib/python3.12/site-packages/gguf/scripts/__init__.py", line 9, in <module>
    from .gguf_editor_gui import main as gguf_editor_gui_entrypoint
  File "/nix/store/l06dc60pbanrbm5ksvf0wh3n2q9blw4z-python3.12-gguf-0.0.0/lib/python3.12/site-packages/gguf/scripts/gguf_editor_gui.py", line 15, in <module>
    from PySide6.QtWidgets import (
ModuleNotFoundError: No module named 'PySide6'

That should not be a fatal error, since gguf-dump doesn't require that module.

This is a problem likely introduced in #12930.

Note that this problem also happens when using pip install gguf in a venv.

Changes

  • Remove gguf-py/gguf/scripts/__init__.py and directly refer to the main functions of the scripts as their entrypoint in pyproject.toml (as suggested in https://github.com/ggml-org/llama.cpp/pull/13036#issuecomment-2818129809)
  • Add pyside6 to the python dependencies included in *-extra devShells
    • Its transitive dependencies are quite big, though (300 MB compressed). Hopefully that is fine with others who use the *-extra devShells
  • Update flake.lock
    • Because the version which is used by gguf_editor_gui is ^6.9, and 6.9.0 is relatively recent (and not in the previous version of nixpkgs in flake.lock)

Make sure to read the contributing guidelines before submitting a PR

compilade avatar Apr 20 '25 20:04 compilade

$ gguf-dump --help
Traceback (most recent call last):
  File "/nix/store/l06dc60pbanrbm5ksvf0wh3n2q9blw4z-python3.12-gguf-0.0.0/bin/.gguf-dump-wrapped", line 6, in <module>
    from gguf.scripts import gguf_dump_entrypoint
  File "/nix/store/l06dc60pbanrbm5ksvf0wh3n2q9blw4z-python3.12-gguf-0.0.0/lib/python3.12/site-packages/gguf/scripts/__init__.py", line 9, in <module>
    from .gguf_editor_gui import main as gguf_editor_gui_entrypoint
  File "/nix/store/l06dc60pbanrbm5ksvf0wh3n2q9blw4z-python3.12-gguf-0.0.0/lib/python3.12/site-packages/gguf/scripts/gguf_editor_gui.py", line 15, in <module>
    from PySide6.QtWidgets import (
ModuleNotFoundError: No module named 'PySide6'

Interesting, so this is clearly a problem with the way the entrypoints are all made from the same __init__.py, which perhaps is not ideal anyway? Is there a better/simpler way to define them?

* Add `pyside6` to the python dependencies included in `*-extra` devShells
  * Its transitive dependencies are quite big, though (300 MB compressed). Hopefully that is fine with others who use the `*-extra` devShells

Hmmm, yeah, let's hope so.

CISC avatar Apr 21 '25 09:04 CISC

Interesting, so this is clearly a problem with the way the entrypoints are all made from the same __init__.py, which perhaps is not ideal anyway? Is there a better/simpler way to define them?

So, to answer my own question, I just tried emptying __init__.py, and defining the script entrypoints as the following instead:

[tool.poetry.scripts]
gguf-convert-endian = "gguf.scripts.gguf_convert_endian:main"
gguf-dump = "gguf.scripts.gguf_dump:main"
gguf-set-metadata = "gguf.scripts.gguf_set_metadata:main"
gguf-new-metadata = "gguf.scripts.gguf_new_metadata:main"
gguf-editor-gui = "gguf.scripts.gguf_editor_gui:main"

Built and installed the package and problem seems to be solved, know why it wasn't done like this to begin with?

CISC avatar Apr 21 '25 10:04 CISC

@compilade Looking at the commit history I think it sort of just evolved into this without any serious consideration as to why, I think clearing __init__.py and pointing directly to the scripts' main function is the solution.

Let me know if you need me to tag the release later.

CISC avatar Apr 21 '25 18:04 CISC

@CISC I went ahead and changed gguf-py/pyproject.toml as you suggest and removed gguf-py/gguf/scripts/__init__.py because it's not really needed since implicit namespaces added in https://peps.python.org/pep-0420/ (in Python 3.3).

I've tested the scripts after building, and they work fine even when PySide6 is not installed (apart from the gguf-editor-gui script, of course, which requires it).

compilade avatar Apr 24 '25 02:04 compilade

@compilade Any reason for not merging yet?

If nix/flake changes are a concern they can be left out for now, ref https://github.com/ggml-org/llama.cpp/pull/13005#issuecomment-2831250849

CISC avatar May 05 '25 07:05 CISC

Any reason for not merging yet?

Not really, sorry (I got distracted).

If nix/flake changes are a concern they can be left out for now, ref https://github.com/ggml-org/llama.cpp/pull/13005#issuecomment-2831250849

I've been wanting to try importing the llama-cpp flake without a flake.lock to see if the suggestion from that comment is doable, and it is (removing the flake.lock works).

Okay, I've reverted the changes to the flake. flake.lock is no longer changed by this PR. (and also I'm not adding the pyside6 dependency to the python-script flake anymore in this PR, because it's kind of big for the limited uses it has in the project (and those who really want it can easily add it back))

So the only changes included are now only for fixing https://github.com/ggml-org/llama.cpp/issues/13054. And you've already seen the changes kept here (since I did not change that further (I'm strictly changing fewer things)), so I will merge this now.

compilade avatar May 06 '25 02:05 compilade

I've been wanting to try importing the llama-cpp flake without a flake.lock

Please don't remove it. It contains exact hashes of nixpkgs and other repos that should be confirmed to work. They don't really need to be updated regularly if you don't want to. Users of flake can easily override it, so there are no downsides of having it. But the downside of removing it is that you can't know which version of nixpkgs can build it, and nixpkgs quite regularly have breaking changes. For instance, it can't be build with the latest unstable nixpkgs without couple of small fixes because of the rocm stack update.

kurnevsky avatar May 16 '25 17:05 kurnevsky