Skip to content

python3Packages.vllm: 0.3.3->0.5.2#328258

Merged
SomeoneSerge merged 8 commits intoNixOS:masterfrom
cfhammill:vllm-version-bump
Aug 2, 2024
Merged

python3Packages.vllm: 0.3.3->0.5.2#328258
SomeoneSerge merged 8 commits intoNixOS:masterfrom
cfhammill:vllm-version-bump

Conversation

@cfhammill
Copy link
Contributor

Description of changes

Updates vllm. Requires two new python packages, lm-format-enforcer and prometheus-fastapi-instrumentator, this PR adds both.

I was able to get the cuda version to build on a slightly older revision of nixpkgs. Would have needed to rebuild the entire python/pytorch/cuda stack to try against master. Compiling vllm alone takes > 1h so this was slow going. Also have not built against rocm.

@CertainLach you mentioned that this need a special triton? It compiles ok for me with the version I fixed in #325843.

It compiles and can be imported, but since tests have not been enabled (and likely wouldn't work anyway) unsure if this works as expected.

CC @SomeoneSerge @ConnorBaker @samuela

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@cfhammill cfhammill requested a review from natsukium as a code owner July 18, 2024 20:14
@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Jul 18, 2024
@CertainLach
Copy link
Member

@CertainLach you mentioned that this need a special triton? It compiles ok for me with the version I fixed in #325843.

It was failing at runtime without new triton for me, I still maintain fork of nixpkgs with updated triton because of that.
Not sure if they have fixed this version requirement now, will check it on this PR later

@cfhammill cfhammill force-pushed the vllm-version-bump branch from b69e62d to e30f59e Compare July 19, 2024 14:47
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jul 19, 2024
@ofborg ofborg bot requested review from CertainLach and happysalada July 19, 2024 15:53
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jul 19, 2024
@cfhammill
Copy link
Contributor Author

I think I've addressed everything, it should be good to go @samuela @SomeoneSerge @CertainLach.

Let me know if you want me to squash the commits.

@SomeoneSerge
Copy link
Contributor

Result of nixpkgs-review pr 328258 --extra-nixpkgs-config '{ cudaSupport = true; cudaCapabilities = [ "7.0" "8.0" "8.6" ]; cudaEnableForwardCompat = false; allowUnfreePredicate = p: builtins.all ( license: license.free || builtins.elem license.shortName [ "CUDA EULA" "cuDNN EULA" "cuTENSOR EULA" "NVidia OptiX EULA" ] ) (if builtins.isList p.meta.license then p.meta.license else [ p.meta.license ]); permittedInsecurePackages = [ "electron-24.8.6" "zotero-6.0.27" ]; }' run on x86_64-linux 1

4 packages marked as broken and skipped:
  • python311Packages.vllm
  • python311Packages.vllm.dist
  • python312Packages.vllm
  • python312Packages.vllm.dist
8 packages built:
  • python311Packages.lm-format-enforcer
  • python311Packages.lm-format-enforcer.dist
  • python311Packages.prometheus-fastapi-instrumentator
  • python311Packages.prometheus-fastapi-instrumentator.dist
  • python312Packages.lm-format-enforcer
  • python312Packages.lm-format-enforcer.dist
  • python312Packages.prometheus-fastapi-instrumentator
  • python312Packages.prometheus-fastapi-instrumentator.dist

@cfhammill
Copy link
Contributor Author

thanks @SomeoneSerge, any idea why the vllm build is marked broken? Your config should have had cuda enabled

@cfhammill cfhammill force-pushed the vllm-version-bump branch from b6bcace to 51c2fe6 Compare July 23, 2024 20:53
@SomeoneSerge
Copy link
Contributor

┃ 
┃        error: Package ‘python3.12-outlines-0.0.45’ in /home/ss/.cache/nixpkgs-review/pr-328258-1/nixpkgs/pkgs/development/python-modules/outlines/default.nix:56 is marked as broken, refusing to evaluate.
┃ 
┃        a) To temporarily allow broken packages, you can use an environment variable
┃           for a single invocation of the nix tools.
┃ 

broken on master (Ofborg rebases PRs on master before running any checks)

@cfhammill
Copy link
Contributor Author

outlines was broken in ff7d46d by an untested version bump, and marked as broken a few days ago when someone noticed.

I fixed and tacked it on to this PR.

@cfhammill
Copy link
Contributor Author

A new version of vllm has been released, seems like a small one so hopefully no need to modify the build other than bump version and hash. Trying now.

@SomeoneSerge
Copy link
Contributor

Result of nixpkgs-review pr 328258 --extra-nixpkgs-config '{ cudaSupport = true; cudaCapabilities = [ "7.0" "8.0" "8.6" ]; cudaEnableForwardCompat = false; allowUnfreePredicate = p: builtins.all ( license: license.free || builtins.elem license.shortName [ "CUDA EULA" "cuDNN EULA" "cuTENSOR EULA" "NVidia OptiX EULA" ] ) (if builtins.isList p.meta.license then p.meta.license else [ p.meta.license ]); permittedInsecurePackages = [ "electron-24.8.6" "zotero-6.0.27" ]; }' run on x86_64-linux 1

8 packages marked as broken and skipped:
  • python311Packages.outlines
  • python311Packages.outlines.dist
  • python311Packages.vllm
  • python311Packages.vllm.dist
  • python312Packages.outlines
  • python312Packages.outlines.dist
  • python312Packages.vllm
  • python312Packages.vllm.dist
12 packages built:
  • python311Packages.lm-format-enforcer
  • python311Packages.lm-format-enforcer.dist
  • python311Packages.prometheus-fastapi-instrumentator
  • python311Packages.prometheus-fastapi-instrumentator.dist
  • python311Packages.pyairports
  • python311Packages.pyairports.dist
  • python312Packages.lm-format-enforcer
  • python312Packages.lm-format-enforcer.dist
  • python312Packages.prometheus-fastapi-instrumentator
  • python312Packages.prometheus-fastapi-instrumentator.dist
  • python312Packages.pyairports
  • python312Packages.pyairports.dist

@cfhammill
Copy link
Contributor Author

cfhammill commented Jul 24, 2024

sorry, removing the broken annotation on outlines (I'm working across two branches [a semi-stable one that builds and works for me and includes a vllm patch for MIG support, and this one] and I'm evidently failing to merge/cherrypick properly, apologies!)

@SomeoneSerge
Copy link
Contributor

evidently failing to merge/cherrypick properly, apologies!)

No worries. It's just

git fetch origin # assuming origin refers to git@github.com:NixOS/nixpkgs
git rebase -i origin/master

Also you might want to look into lazygit

@cfhammill
Copy link
Contributor Author

Thanks @SomeoneSerge. Turns out this was not the issue I thought, this PR does have the broken annotation removed, not sure how it got reintroduced, checking now.

@cfhammill cfhammill force-pushed the vllm-version-bump branch from 0823906 to 949004f Compare July 24, 2024 17:41
Copy link
Member

@bcdarwin bcdarwin left a comment

Choose a reason for hiding this comment

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

Made some largely cosmetic suggestions, but please do squash into commits for vllm, outlines, and pyairports.

@cfhammill
Copy link
Contributor Author

will squash shortly, thanks @bcdarwin

@cfhammill cfhammill force-pushed the vllm-version-bump branch from 8bccdc3 to a694582 Compare July 25, 2024 14:04
@bcdarwin bcdarwin requested a review from samuela July 25, 2024 16:27
Comment on lines 101 to 103
Copy link
Contributor

@SomeoneSerge SomeoneSerge Jul 25, 2024

Choose a reason for hiding this comment

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

Side note: torch in nativeBuildInputs looks 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.

yeah, not sure why that was put there.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe one of those 'let the user bring their own torch' ideas a la the current torchmetrics?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe one of those 'let the user bring their own torch' ideas a la the current torchmetrics?

I rather meant that vllm likely needs the host platform's torch, not the build platform's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok - I think this torch can be removed, torch is already in propagatedBuildInputs, shouldn't be needed twice.

@cfhammill cfhammill force-pushed the vllm-version-bump branch 2 times, most recently from a2e5cc8 to 067bbf0 Compare July 25, 2024 20:16
@SomeoneSerge
Copy link
Contributor

Result of nixpkgs-review pr 328258 --extra-nixpkgs-config '{ cudaSupport = true; cudaCapabilities = [ "7.0" "8.0" "8.6" ]; cudaEnableForwardCompat = false; allowUnfreePredicate = p: builtins.all ( license: license.free || builtins.elem license.shortName [ "CUDA EULA" "cuDNN EULA" "cuTENSOR EULA" "NVidia OptiX EULA" ] ) (if builtins.isList p.meta.license then p.meta.license else [ p.meta.license ]); permittedInsecurePackages = [ "electron-24.8.6" "zotero-6.0.27" ]; }' run on x86_64-linux 1

2 packages failed to build:
  • python312Packages.vllm
  • python312Packages.vllm.dist
14 packages built:
  • python311Packages.lm-format-enforcer
  • python311Packages.lm-format-enforcer.dist
  • python311Packages.outlines
  • python311Packages.outlines.dist
  • python311Packages.pyairports
  • python311Packages.pyairports.dist
  • python311Packages.vllm
  • python311Packages.vllm.dist
  • python312Packages.lm-format-enforcer
  • python312Packages.lm-format-enforcer.dist
  • python312Packages.outlines
  • python312Packages.outlines.dist
  • python312Packages.pyairports
  • python312Packages.pyairports.dist

@cfhammill
Copy link
Contributor Author

@SomeoneSerge do you have the build logs for python312Packages.vllm?

@SomeoneSerge
Copy link
Contributor

I had to re-run nixpkgs-review because logs got GC-ed, but here: https://gist.github.com/SomeoneSerge/6a3344368c146171160a92d6865ffd57

TLDR: upstream marked vllm as only supporting up to 3.11; I highly doubt that was intentional though

@SomeoneSerge
Copy link
Contributor

SomeoneSerge commented Jul 30, 2024

https://github.com/vllm-project/vllm/blob/af647fb8b3ea9d910f7d1bc104af8986d048a8e2/CMakeLists.txt#L17

I'm tempted to patch this out and either replace the list with the current interpreter's version or replace the whole thing with an unwrapped find_package

@SomeoneSerge
Copy link
Contributor

So uh I'm not a vllm user, you probably know better: is there value in providing python311Packages.vllm but not python3Packages.vllm? I'm content to merge the PR (as is plus the disabled = ... attribute) since it's upstream who decided not to support 3.12

@cfhammill
Copy link
Contributor Author

I'm using python311 so not having 312 doesn't bother me personally, but it looks like there isn't a good reason it's disabled.
vllm-project/vllm#6877
other than excessively tight version bounds. I'd be ok with either disabling 312 or patching the cmake file to allow 312 and hoping that works.

cfhammill and others added 8 commits July 31, 2024 21:56
vllm remove comment, minor version bump

use refs/tags and format=pyproject

use @Args and realphabetize inputs

move stdenv, mv cuda_home, add/clean cuda deps, restore comment

nixfmt rfc style

remove stale comment
pep-517 attr names, no native torch, phase order

Co-authored-by: cfhammill <christopher.hammill@unityhealth.to>
Co-authored-by: cfhammill <christopher.hammill@unityhealth.to>
Co-authored-by: cfhammill <christopher.hammill@unityhealth.to>
Co-authored-by: cfhammill <christopher.hammill@unityhealth.to>
Co-authored-by: cfhammill <christopher.hammill@unityhealth.to>
@SomeoneSerge
Copy link
Contributor

@cfhammill I just added the stuff I said was optional, idk why; the python 3.12 build... doesn't immediately fail (it's been running for 40' now) so I intend to leave it at that; I'll check on Ofborg tomorrow and probably just merge, but please do run some checks and skim over the extra stuff I pushed that it makes sense. I don't use vllm so I can't reason much about this

@cfhammill
Copy link
Contributor Author

I'm happy with this! Thanks @SomeoneSerge! Merge when ready.

@SomeoneSerge SomeoneSerge merged commit 90e1179 into NixOS:master Aug 2, 2024
@SomeoneSerge
Copy link
Contributor

Thanks @cfhammill

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants