cudaPackages.nccl: refactor to fix #220340 and #221895

[ConnorBaker]

Description of changes

• moves to autoAddOpenGLRunpathHook instead of manual invocation of addOpenGLRunpath
  • closes cudaPackages.nccl: switch to autoAddOpenGLRunpathHook #220340.
• corrects bug where nccl was only built with default version of cudaPackages by changing the nccl derivation to accept the actual CUDA packages it requires as arguments
  • closes cudaPackages.nccl: only ever builds with default version of cudaPackages #221895
  • fixes a bug where NCCL would not build with CUDA 12 or newer without overriding the default version of cudaPackages in Nixpkgs
• fixes paths given to the environment for cuda_cudart to prevent breakage when cudaPackages: multiple outputs for redistributables #240498 is merged
• fixes a bug where NCCL would not build with CUDA 12.0 due to a comparison with a version string of the wrong precision
• removed unnecessary which entry in nativeBuildInputs
  • NOTE: While it seems like which is unused, it's used to locate NVCC and extract the CUDA version. This is in turn used for conditional compilation.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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/)
• 23.05 Release Notes (or backporting 22.11 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.

[ConnorBaker]

Waiting for #220402 to be merged prior to proceeding.

[nixos-discourse]

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

[ConnorBaker]

Result of nixpkgs-review pr 217619 --extra-nixpkgs-config '{ allowUnfree = true; cudaSupport = true; cudaForwardCompat = false; cudaCapabilities = [ "8.6" ]; }' run on x86_64-linux 1

16 packages built:

• cudaPackages.nccl
• cudaPackages.nccl.dev
• python310Packages.cupy
• python310Packages.cupy.dist
• python310Packages.jaxlibWithCuda
• python310Packages.jaxlibWithCuda.dist
• python310Packages.tensorflowWithCuda
• python310Packages.tensorflowWithCuda.dist
• python310Packages.torchWithCuda
• python310Packages.torchWithCuda.dev
• python310Packages.torchWithCuda.dist
• python310Packages.torchWithCuda.lib
• python311Packages.cupy
• python311Packages.cupy.dist
• python311Packages.jaxlibWithCuda
• python311Packages.jaxlibWithCuda.dist

[ConnorBaker]

CC @NixOS/cuda-maintainers

[samuela]

I'm not familiar with what the runtime dependencies of nccl are, but these changes look reasonable to me. Perhaps @mdaiter or @orivej could comment?

As we move towards more of a "python-packages.nix" model, I think it might be worth collecting all cudaPackages.* packages into a single directory like python does. We have a bunch of random stuff spread out all over the codebase for no good reason except that that's just how things evolved over time. But we don't need to keep living like this. NCCL isn't really math software anyhow.

[ConnorBaker]

As we move towards more of a "python-packages.nix" model, I think it might be worth collecting all cudaPackages.* packages into a single directory like python does. We have a bunch of random stuff spread out all over the codebase for no good reason except that that's just how things evolved over time. But we don't need to keep living like this. NCCL isn't really math software anyhow.

I like that idea! It would certainly make it easier to keep track of different components.

How do you envision stuff like torch or magma working? They're packages which can rely on CUDA, but (at least torch) perhaps doesn't belong in the CUDA packages directory.

As a separate point, in the same way that we can get torch with different versions of python through python3*Packages, should we likewise have cudaPackages_*.torch available? If yes, how would we handle different versions of Python? If we were to want to match Anaconda, the user should be able to specify the version of CUDA and Python for torch. Of course this can be done by using overlays and changing what cudaPackages and python3Packages points to, but that's not necessarily user friendly if it's hard to discover/arrive at. (Although maybe this is tangential -- not enough coffee yet. If it is, let me know and I can throw it in a separate issue to track/discuss further!)

[samuela]

How do you envision stuff like torch or magma working? They're packages which can rely on CUDA, but (at least torch) perhaps doesn't belong in the CUDA packages directory.

Good question! The line is a little fuzzy but I envision that the general policy would be that cudaPackages.* would be only for packages that are part of CUDA toolkit or related packages from NVIDIA, and all consumers of those packages (torch, magma, etc) would continue to live externally. This would more or less match the current Python package situation AFAIU it -- eg C++ packages that rely on python packages are still maintained outside of pkgs/development/python-modules.

As a separate point, in the same way that we can get torch with different versions of python through python3Packages, should we likewise have cudaPackages_.torch available? If yes, how would we handle different versions of Python? If we were to want to match Anaconda, the user should be able to specify the version of CUDA and Python for torch. Of course this can be done by using overlays and changing what cudaPackages and python3Packages points to, but that's not necessarily user friendly if it's hard to discover/arrive at. (Although maybe this is tangential -- not enough coffee yet. If it is, let me know and I can throw it in a separate issue to track/discuss further!)

I believe this may indeed be a tangential point :P Actually, I think we should focus on supporting fewer version combinations, not more. Having to maintain support for all combinations increases testing and maintenance burden. Users who are interested in using older versions can always use custom overlays or pin nixpkgs to previous commits.

[orivej]

I'm not familiar with what the runtime dependencies of nccl are, but these changes look reasonable to me. Perhaps @mdaiter or @orivej could comment?

nccl does not have any runtime dependencies.

The changes look good to me (although I'm not familiar with the concept of backendStdenv and don't understand why build dependencies have to be moved to nativeBuildInputs).

[SomeoneSerge]

Thanks @orivej, backendStdenv is just a hacky way to refer to a nvcc-compatible gccStdenv. We had some libstdc++ mismatch issues

[ConnorBaker]

Rebased, moved cuda_cccl and cuda_cudart to buildInputs from nativeBuildInputs, and changed license from bsd3 to unfreeRedistributable. Running nixpkgs-review now!

[ConnorBaker]

I let it run overnight on my i9 13900K and it still didn't finish -- hopefully this time around it'll be faster :x

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-46/26872/1

[SomeoneSerge]

What's it use which for again? Can we control it w/o which?

[SomeoneSerge]

I don't remember where we stalled, but this LGTM. Nixpkgs-review and merge?

[samuela]

I don't remember where we stalled, but this LGTM. Nixpkgs-review and merge?

I believe @ConnorBaker left a TODO item for himself here, but not sure what the status of that is?

[ConnorBaker]

Found out this was still open when I tried to update NCCL to 2.18.3.

Let me try to pick this back up :)

[ConnorBaker]

Rebased, updated, and added more notes about what it's fixing.

Testing with a super simple test suite built around nccl-tests: https://github.com/ConnorBaker/nix-cuda-test

nix build -L --override-input nixpkgs github:nixos/nixpkgs/pull/217619/head github:ConnorBaker/nix-cuda-test/3074628f4a0ece4928e032f9e2f2f1307b6ed22d#nccl-test-suite

Seemed to work for me, though it's just one device.

https://gist.github.com/ConnorBaker/ea3c49e23c1eaf2544fe97ae6fdd67a5