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(nix): Package gguf-py #5664

Merged
merged 20 commits into from
Sep 2, 2024
Merged

Conversation

ditsuke
Copy link
Contributor

@ditsuke ditsuke commented Feb 22, 2024

What and why

Currently, the Nix build does not ship with the gguf-py lib, rendering scripts like convert-hf-to-gguf unusable.

This is an attempt to package gguf-py with buildPythonPackage. Very barebones at the moment and needs some work to exclude the Nix built gguf from the devShells, and some thought into other things like packaging the heavier python dependencies (we don't want them to be included by default, but we need a way to opt-in).

CC: @SomeoneSerge

@ditsuke ditsuke force-pushed the build/nix/gguf-py-inc branch 2 times, most recently from 7efb80d to 1d0a97c Compare February 23, 2024 09:55
@ditsuke ditsuke marked this pull request as ready for review February 23, 2024 12:01
@ditsuke
Copy link
Contributor Author

ditsuke commented Feb 23, 2024

This PR is good for review for the standalone packaging of gguf-py. However for the convert* scripts that ship with this repo to work, they need a Python environment that has the full superset of dependencies they rely on including torch. We'll need to ship with these heavy dependencies, or alternatively we could:

  1. Decouple the scripts from default llama package, and expose them through a llama-scripts package instead.
  2. By default have them run against the default python interpreter in the environment and ship the static-environment version as another option.

Open to other ideas.

{
lib,
llamaVersion,
python3,
Copy link
Collaborator

@SomeoneSerge SomeoneSerge Feb 23, 2024

Choose a reason for hiding this comment

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

buildPythonPackage, poetry-core, numpy as arguments. No need for python3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The evaluating scope should set them explicitly then?

Copy link
Contributor Author

@ditsuke ditsuke Feb 23, 2024

Choose a reason for hiding this comment

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

Not sure if there's a cleaner way: 00fbda0

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, the scope inherits from python3Packages, which in turn inherits from pkgs

Copy link
Collaborator

@SomeoneSerge SomeoneSerge Feb 23, 2024

Choose a reason for hiding this comment

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

Ah, sorry, mistake on my part. This one only inherits from pkgs, we need to handle python separately.

EDIT: this is good, we'll see about making one for python, and making it easy to reuse in pythonPackagesExtensions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, then we're good to go for this PR?

Copy link
Collaborator

@SomeoneSerge SomeoneSerge Feb 24, 2024

Choose a reason for hiding this comment

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

If by "good to go" you mean we need to move the python scripts out of the main derivation so that it doesn't depend on the choice of python version 😅

EDIT: I phrased this badly. What I meant is, I think the way to introduce support for the python scripts is to separate the python and C++ parts and to add means to extend python3Packages. I'm pretty exhausted today and won't be able to make my explanation any better, but I'll try to send some code tomorrow. I do acknowledge that we shouldn't extend the scope of the PR too much and you likely want this feature merged in a timely manner, etc, etc, but right now I think there's a bit more work to be done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. I gave splitting the Python/C++ devShells and derivation a shot. I don't think it addresses extending python3Packages, but it should address the other concern about the mixed-in Python and C++ derivation, and get the python scripts exposed with .#python-scripts. Please take a look and tell me what you think, then maybe we can get the python3Packages thing done and get this in :D

@ditsuke ditsuke force-pushed the build/nix/gguf-py-inc branch 2 times, most recently from 4957125 to 00fbda0 Compare February 23, 2024 17:32
pyproject.toml Outdated Show resolved Hide resolved
@SomeoneSerge SomeoneSerge added the nix Issues specific to consuming flake.nix, or generally concerned with ❄ Nix-based llama.cpp deployment label Mar 21, 2024
@ditsuke
Copy link
Contributor Author

ditsuke commented Jul 8, 2024

Rebased to after merge of #5745. Commits will need to be cleaned up, but this is open for review otherwise.

@github-actions github-actions bot added python python script changes devops improvements to build systems and github actions labels Jul 8, 2024
@ditsuke ditsuke force-pushed the build/nix/gguf-py-inc branch 2 times, most recently from 7729067 to 0188318 Compare August 19, 2024 21:28
Copy link
Collaborator

@compilade compilade left a comment

Choose a reason for hiding this comment

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

This seems to work for my workflow with nix develop .#default-extra. Thanks!

I've tested running pyright on the whole repository (which requires access to all python libraries used within), and it works without problem.

I've also tested building a -DGGML_VULKAN=TRUE build with nix develop .#vulkan-extra, and it works too.

There are more things in the $PATH (because python libraries only were added to the python interpreter before, not also the $PATH), but I think this might be useful in some situations.

Rebuilding gguf-py at each change might be cumbersome, though, but I think this is better than before because I simply never tested building gguf-py with poetry before, but with this it's now handled when creating a devShell. (I never needed to actually build the package because convert_hf_to_gguf.py uses some other trick anyway to include the local gguf-py instead of the package)

I think I like this.

.devops/nix/package.nix Outdated Show resolved Hide resolved
.devops/nix/python-scripts.nix Show resolved Hide resolved
@ditsuke
Copy link
Contributor Author

ditsuke commented Sep 2, 2024

Rebased to master. @ggerganov this should be ready for merge. cc @SomeoneSerge

@ggerganov ggerganov merged commit 9c1ba55 into ggerganov:master Sep 2, 2024
8 checks passed
@ditsuke
Copy link
Contributor Author

ditsuke commented Sep 2, 2024

Thanks for merging!

llamaVersion ? "0.0.0",
}:

let
pythonPackages = python3.pkgs;
Copy link
Collaborator

Choose a reason for hiding this comment

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

A note for the future: I'd discourage using python3.pkgs in favour of python3Packages because the former does not support splicing. We're not using any cross-compilation in this flake, but I think it's important to stay minimal and idiomatic so as not to spread confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's good to know. This should be changed to use python3Packages next time someone visits these files.

Comment on lines +10 to +16
buildPythonPackage = pythonPackages.buildPythonPackage;
numpy = pythonPackages.numpy;
tqdm = pythonPackages.tqdm;
sentencepiece = pythonPackages.sentencepiece;
pyyaml = pythonPackages.pyyaml;
poetry-core = pythonPackages.poetry-core;
pytestCheckHook = pythonPackages.pytestCheckHook;
Copy link
Collaborator

Choose a reason for hiding this comment

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

None of this would be necessary if we made a subscope for python packages, but this can be left for future. And the UX for implementing that currently sucks anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes would be sure nice to have a python scope at some point, this is rather ugly.

dsx1986 pushed a commit to dsx1986/llama.cpp that referenced this pull request Oct 29, 2024
* style: format with nixfmt/rfc101-style

* build(nix): Package gguf-py

* build(nix): Refactor to new scope for gguf-py

* build(nix): Exclude gguf-py from devShells

* build(nix): Refactor gguf-py derivation to take in exact deps

* build(nix): Enable pytestCheckHook and pythonImportsCheck for gguf-py

* build(python): Package python scripts with pyproject.toml

* chore: Cleanup

* dev(nix): Break up python/C devShells

* build(python): Relax pytorch version constraint

Nix has an older version

* chore: Move cmake to nativeBuildInputs for devShell

* fmt: Reconcile formatting with rebase

* style: nix fmt

* cleanup: Remove unncessary __init__.py

* chore: Suggestions from review

- Filter out non-source files from llama-scripts flake derivation
- Clean up unused closure
- Remove scripts devShell

* revert: Bad changes

* dev: Simplify devShells, restore the -extra devShell

* build(nix): Add pyyaml for gguf-py

* chore: Remove some unused bindings

* dev: Add tiktoken to -extra devShells
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Nov 15, 2024
* style: format with nixfmt/rfc101-style

* build(nix): Package gguf-py

* build(nix): Refactor to new scope for gguf-py

* build(nix): Exclude gguf-py from devShells

* build(nix): Refactor gguf-py derivation to take in exact deps

* build(nix): Enable pytestCheckHook and pythonImportsCheck for gguf-py

* build(python): Package python scripts with pyproject.toml

* chore: Cleanup

* dev(nix): Break up python/C devShells

* build(python): Relax pytorch version constraint

Nix has an older version

* chore: Move cmake to nativeBuildInputs for devShell

* fmt: Reconcile formatting with rebase

* style: nix fmt

* cleanup: Remove unncessary __init__.py

* chore: Suggestions from review

- Filter out non-source files from llama-scripts flake derivation
- Clean up unused closure
- Remove scripts devShell

* revert: Bad changes

* dev: Simplify devShells, restore the -extra devShell

* build(nix): Add pyyaml for gguf-py

* chore: Remove some unused bindings

* dev: Add tiktoken to -extra devShells
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants