-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
torchvision: fix #217878; migrate to cudaPackages #218035
torchvision: fix #217878; migrate to cudaPackages #218035
Conversation
Result of 2 packages marked as broken and skipped:
3 packages failed to build:
20 packages built:
|
fdec768
to
a38527c
Compare
Result of 1 package marked as broken and skipped:
2 packages failed to build:
21 packages built:
|
Failed derivations
|
a38527c
to
d397194
Compare
Running again after rebasing on master |
Result of 1 package marked as broken and skipped:
2 packages failed to build:
21 packages built:
|
Failed derivations
|
cc @NixOS/cuda-maintainers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Builds and runs (some random snippet)
For wrapping the compiler, we could probably do something like
and use |
Actually, I think we should just upgrade the cudatoolkit_12 gcc to gcc12 to match stdenv: https://cs.github.com/NixOS/nixpkgs/blob/4a6cd14f37b22c9d2f90e3774b8ea9601025ad6b/pkgs/development/compilers/cudatoolkit/versions.toml#L79 It was only ever held back to gcc11 because stdenv was stuck on gcc11. now that stdenv is on gcc12 IIUC that should resolve these issues? |
I'm in support of that upgrade, though it might be problematic because some packages (cough torch cough) have explicit requirements for CUDA versions: nixpkgs/pkgs/development/python-modules/torch/default.nix Lines 49 to 50 in 6169ccb
On a different note, is it reasonable to have asserts like that in packages? I don't know how one would be able to circumvent that if they wanted to build against HEAD (which mostly supports CUDA 12) by overriding the |
Do we know that pytorch doesn't support CUDA 12, or is that assert overly cautious? We can always pin pytorch to a CUDA 11.x version until they get up to speed. |
iirc you can build with whatever, and a quick once-over of However, they do have a tracking issue for CUDA 12 support: pytorch/pytorch#91122 In my experience, when built against HEAD it mostly just works, though there are a few outstanding issues. |
I think it's safe to say that we should prefer setting meta.broken or showing warnings to just killing Nix eval. The particular piece of code is legacy |
Ok, yeah in that case we should wait until pytorch/pytorch#91122 is resolved. Curious what this all has to do with the gcc upgrade though? |
That's my bad -- I misinterpreted your comment about cudaPackages_12 and GCC 12 (#218035 (comment)) to mean "we should make that the default." Sorry! |
d397194
to
16dbe78
Compare
…ages - when building with CUDA, we must make sure to use the same C/C++ compiler as NVCC to avoid symbol errors when linking - move to cudaPackages to reduce closure size - separate build-time and run-time CUDA dependencies
16dbe78
to
ed00c38
Compare
Result of 1 package marked as broken and skipped:
2 packages failed to build:
23 packages built:
|
Failures:
are not related. |
LGTM thanks @ConnorBaker ! |
(We already had bumped cuda12's gcc in the -ccbin pr iirc) |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/tweag-nix-dev-update-45/26397/1 |
Description of changes
Currently,
torchvision
doesn't even build onmaster
because the default GCC is 12, which is unsupported by NVCC until CUDA 12.0.This fixes #217878.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)