Skip to content

gguf-py : avoid requiring PySide6 for packaged scripts #13036

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

Merged

Conversation

compilade
Copy link
Collaborator

@compilade compilade commented Apr 20, 2025

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 gguf-py : avoid requiring PySide6 for packaged scripts #13036 (comment))
  • 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 compilade added nix Issues specific to consuming flake.nix, or generally concerned with ❄ Nix-based llama.cpp deployment bugfix fixes an issue or bug python python script changes labels Apr 20, 2025
@compilade compilade requested a review from CISC April 20, 2025 20:39
@github-actions github-actions bot added the devops improvements to build systems and github actions label Apr 20, 2025
@CISC
Copy link
Collaborator

CISC commented Apr 21, 2025

$ 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
Copy link
Collaborator

CISC commented Apr 21, 2025

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

CISC commented Apr 21, 2025

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

@compilade
Copy link
Collaborator Author

@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).

@CISC
Copy link
Collaborator

CISC commented May 5, 2025

@compilade Any reason for not merging yet?

If nix/flake changes are a concern they can be left out for now, ref #13005 (comment)

- gguf-py : remove gguf-py/gguf/scripts/__init__.py because it's not needed

Implicit namespaces are supported since Python 3.3 (https://peps.python.org/pep-0420/),
and the entrypoints in pyproject.toml can directly refer to the main functions.
@compilade compilade force-pushed the compilade/gguf-py-deps-pyside6 branch from 7aef532 to a994bda Compare May 6, 2025 02:09
@compilade
Copy link
Collaborator Author

compilade commented May 6, 2025

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 #13005 (comment)

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 #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 compilade merged commit a7366fa into ggml-org:master May 6, 2025
4 checks passed
gabe-l-hart added a commit to gabe-l-hart/llama.cpp that referenced this pull request May 6, 2025
* origin/master: (27 commits)
llama : fix build_ffn without gate (ggml-org#13336)
CUDA: fix bad asserts for partial offload (ggml-org#13337)
convert : qwen2/3moe : set yarn metadata if present (ggml-org#13331)
CUDA: fix --split-mode row for MMQ (ggml-org#13323)
gguf-py : avoid requiring pyside6 for other scripts (ggml-org#13036)
CUDA: fix logic for clearing padding with -ngl 0 (ggml-org#13320)
sampling : Integrate Top-nσ into main sampling chain (and add it to the server) (ggml-org#13264)
server : Webui - change setText command from parent window to also send the message. (ggml-org#13309)
mtmd : rename llava directory to mtmd (ggml-org#13311)
clip : fix confused naming ffn_up and ffn_down (ggml-org#13290)
convert : bailingmoe : set yarn metadata if present (ggml-org#13312)
SYCL: Disable mul_mat kernels for noncontiguous tensor b (ggml-org#13308)
mtmd : add C public API (ggml-org#13184)
rpc : use backend registry, support dl backends (ggml-org#13304)
ggml : activate s390x simd for Q3_K (ggml-org#13301)
llava/mtmd : fixes to fully support dl backends (ggml-org#13303)
llama : build windows releases with dl backends (ggml-org#13220)
CUDA: fix race condition in MMQ stream-k fixup (ggml-org#13299)
CUDA: fix race condition in MMQ ids_dst (ggml-org#13294)
vulkan: Additional type support for unary, binary, and copy (ggml-org#13266)
...
@kurnevsky
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix fixes an issue or bug devops improvements to build systems and github actions nix Issues specific to consuming flake.nix, or generally concerned with ❄ Nix-based llama.cpp deployment python python script changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misc. bug: in version 0.16.2, the gguf-dump CLI tool fails due to a missing PySide6 module, indicating an unintended GUI depende
3 participants