Skip to content

cudaPackages: introduce cudaLib and switch from backendStdenv to cudaStdenv#405751

Closed
ConnorBaker wants to merge 8 commits intoNixOS:masterfrom
ConnorBaker:feat/cuda-packages-uses-cudaStdenv
Closed

cudaPackages: introduce cudaLib and switch from backendStdenv to cudaStdenv#405751
ConnorBaker wants to merge 8 commits intoNixOS:masterfrom
ConnorBaker:feat/cuda-packages-uses-cudaStdenv

Conversation

@ConnorBaker
Copy link
Copy Markdown
Contributor

@ConnorBaker ConnorBaker commented May 10, 2025

Important

Introduction of cudaLib has been split into #406531. This PR is closed as discussions about goals and implementation details of cudaStdenv need to happen first.

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@ConnorBaker ConnorBaker self-assigned this May 10, 2025
@ConnorBaker ConnorBaker added the 6.topic: cuda Parallel computing platform and API label May 10, 2025
@github-actions github-actions Bot added 6.topic: python Python is a high-level, general-purpose programming language. 8.has: documentation This PR adds or changes documentation labels May 10, 2025
@ConnorBaker ConnorBaker force-pushed the feat/cuda-packages-uses-cudaStdenv branch 2 times, most recently from d4bcc1f to c8dfb5e Compare May 10, 2025 02:26
@github-actions github-actions Bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels May 10, 2025
@ConnorBaker ConnorBaker moved this from New to 🏗 In progress in CUDA Team May 10, 2025
@ConnorBaker ConnorBaker force-pushed the feat/cuda-packages-uses-cudaStdenv branch 2 times, most recently from 646b010 to e11f0c0 Compare May 10, 2025 15:02
@ConnorBaker ConnorBaker force-pushed the feat/cuda-packages-uses-cudaStdenv branch from e11f0c0 to 40baa7d Compare May 12, 2025 15:24
@SomeoneSerge
Copy link
Copy Markdown
Contributor

SomeoneSerge commented May 12, 2025

  1. IMO we should reserve the name cudaStdenv for when it's actually sufficient to build a package with CUDA support. The reason backendStdenv was not named that is because it isn't a "cuda stdenv". Let's brainstorm what's missing: having to simultaneously add nvcc and cudart (and the custom stdenv) is one part of the puzzle
  2. I think we should make as many of the helpers we have "private" as we can: this applies to most of the content of cudaFlags (flags? let's delete one of these too) and I think should apply to cudaLib. By "making private" I mean changing the names to begin with __ and documenting them as private (subject to change, including in a backport) in the manual. EDIT: I see now you're not introducing it into the attribute tree yet, so it's not a concern I suppose?

Comment thread pkgs/development/cuda-modules/lib/default.nix Outdated
@ConnorBaker
Copy link
Copy Markdown
Contributor Author

  1. IMO we should reserve the name cudaStdenv for when it's actually sufficient to build a package with CUDA support. The reason backendStdenv was not named that is because it isn't a "cuda stdenv". Let's brainstorm what's missing: having to simultaneously add nvcc and cudart (and the custom stdenv) is one part of the puzzle

Fair point -- I'm thinking:

{
  # NVCC is required to compile CUDA code.
  nativeBuildInputs = [ cuda_nvcc ];
  # The CUDA runtime is required by most CUDA applications and depends on the headers
  # provided by CCCL.
  buildInputs =
    [
      cuda_cudart
      cuda_cccl
    ]
    # CUDA compat is included when non-null and available on the host platform.
    ++ lib.optionals (cuda_compat != null && lib.availableOn stdenv.hostPlatform cuda_compat) [
      cuda_compat
    ];
}
  1. I think we should make as many of the helpers we have "private" as we can: this applies to most of the content of cudaFlags (flags? let's delete one of these too) and I think should apply to cudaLib. By "making private" I mean changing the names to begin with __ and documenting them as private (subject to change, including in a backport) in the manual.

My intention with introducing cudaLib was to create a centralized library of functions and data necessary to create, modify, or extend CUDA package sets, both to float out commonalities in our packaging in-tree and to enable out-of-tree users to make use of our tooling without needing to vendor or duplicate a bunch of stuff we don't otherwise expose. I'm not opposed to making some of them private (or migrating them to lib proper), but I feel that doing that for the majority would be counter to what I want for cudaLib. Does that make sense, and or do you have thoughts on how to expose a minimal public interface which still allows for re-use?

On the topic of flags, I've had a TODO hanging around for a while to remove cudaFlags -- do you have a preference for one name over the other? I like flags because it's short, but cudaFlags makes it more clear what it is in cases where it's inherited in package expressions outside of the cudaPackages scope. In the same vein, I'm supposing we'd call our standard environment cudaStdenv and not just stdenv. On the other hand, having the cuda prefix within the cudaPackages scope feels repetitive... but at least you know which scope it's coming from!

Signed-off-by: Connor Baker <[email protected]>
@SomeoneSerge
Copy link
Copy Markdown
Contributor

SomeoneSerge commented May 12, 2025 via email

@ConnorBaker
Copy link
Copy Markdown
Contributor Author

Excellent points!

In terms of moving this PR forward, is it acceptable to you if:

  1. I keep the name of backendStdenv instead of renaming it to cudaStdenv (all the additional attributes would remain as needed for further refactors)
  2. I update the names and documentation of the functions in cudaLib.utils to indicate there are no stability guarantees (I'm thinking a single underscore, as double-underscore seems to be mostly used by Nix)
  3. I do a tree-wide refactor to remove cudaFlags and replace it with flags

I want to have a further discussion with you about the implementation of a cudaStdenv with the properties you've outlined above, but I think that will be a longer series of discussions and I don't want to hold up landing fixes for package set leakage and introduction of pkgsCuda.

@SomeoneSerge
Copy link
Copy Markdown
Contributor

SomeoneSerge commented May 12, 2025 via email

@ConnorBaker
Copy link
Copy Markdown
Contributor Author

Introduction of cudaLib has been split into #406531. This PR is closed as discussions about goals and implementation details of cudaStdenv need to happen first.

@github-project-automation github-project-automation Bot moved this from 🏗 In progress to ✅ Done in CUDA Team May 12, 2025
@ConnorBaker ConnorBaker deleted the feat/cuda-packages-uses-cudaStdenv branch May 14, 2025 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: cuda Parallel computing platform and API 6.topic: python Python is a high-level, general-purpose programming language. 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

2 participants