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

Refactor CUDA versions in dependencies.yaml. #1422

Merged
merged 2 commits into from
Jan 11, 2024

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Jan 9, 2024

Description

This is a follow-up PR to #1414. I thought some more about how to separate cuda-version pinnings (which control the CUDA version we use to build and test in conda) from actual CUDA Toolkit package dependencies (which we can handle according to only the major version 11/12). I discussed this PR on a call with @jameslamb in the context of upgrading to CUDA 12.2 (rapidsai/build-planning#6). This set of changes is mostly important for conda builds/tests, since cuda-version only controls conda. The pip wheel build/test process is unchanged, since its CUDA versions are controlled by the shared-workflows CI images.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@bdice bdice requested a review from a team as a code owner January 9, 2024 22:57
- cudatoolkit
- matrix:
cuda: "12.*"
packages:
Copy link
Contributor Author

@bdice bdice Jan 9, 2024

Choose a reason for hiding this comment

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

This dependency list is empty for CUDA 12. I considered listing cuda-cudart-dev here but no packages aside from the compiler are technically needed for CUDA 12. The cuda-nvcc package (from the build dependency list) will include the cuda-cudart pieces needed for runtime. I can't think of a current use of dependencies.yaml where this distinction would matter, but it's possible that we might want to add cuda-cudart-dev here in the future for reasons I can't think of at the moment. I decided to leave it out because it would result in a new line for cuda-cudart-dev in the conda/environments/all_cuda-120_arch-x86_64.yaml file which didn't seem necessary. (Also discussed with @jameslamb)

Copy link
Member

Choose a reason for hiding this comment

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

So in cuDF (or another RAPIDS library), this would include a few more packages

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, that's correct.

@@ -126,39 +126,45 @@ dependencies:
- output_types: conda
packages:
- &doxygen doxygen=1.9.1
cudatoolkit:
Copy link
Contributor Author

@bdice bdice Jan 9, 2024

Choose a reason for hiding this comment

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

The inclusion of cudatoolkit is now handled by a separate dependency list called cuda which abstracts over major versions instead of major.minor versions. It turns out that the dependency files where the cudatoolkit dependency list was being included should already get their required CUDA parts as dependencies of the built rmm packages (for test_cpp, test_python, or docs environments) or cuda-nvcc itself (if using build.sh with conda/environments/all_cuda-120_arch-x86_64.yaml, see also https://github.com/rapidsai/rmm/pull/1422/files#r1446707857).

Therefore, the only relevant purpose this dependency list must serve is to constrain the CUDA major.minor version by providing a pinning on cuda-version that matches the input matrix's cuda key.

dependencies.yaml Outdated Show resolved Hide resolved
@bdice bdice added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Jan 9, 2024
@bdice bdice self-assigned this Jan 9, 2024
@jakirkham
Copy link
Member

Seems like a nice simplification. Thanks Bradley! 🙏

@jameslamb
Copy link
Member

This all makes sense to me and should make future upgrades a little easier. Thanks for taking the time to explain it to me!

I'll try something similar with cudf tomorrow.

Comment on lines 133 to 136
- matrix:
cuda: "11.2"
packages:
- cuda-version=11.2
Copy link
Member

Choose a reason for hiding this comment

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

Does rapids-dependency-file-generator have some concept of templating that we could use?

If so, maybe we could rewrite this to something like and allow that to be reused across different CUDA versions and cutdown on the boilerplate further

          - matrix:
              cuda: "{{ cuda }}"
            packages:
              - cuda-version={{ cuda }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, there is not support for anything like that. I think we’ve avoided introducing a templating layer on purpose, in part. It’s a little bit like how conda recipes want to move away from template logic… we’ve gotten quite far without it for dependencies, but I see the value.

Copy link
Member

Choose a reason for hiding this comment

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

For clarity Conda recipes will still have templating. It's more that we want the recipe format to reflect YAML. The reason being we can read them with a normal YAML library in one pass (not lots of passes through with Jinja, selectors, etc.)

There are ways to have templating and make it YAML friendly. Though this is maybe another discussion for a different issue

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Bradley! 🙏

@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 5c30e87 into rapidsai:branch-24.02 Jan 11, 2024
47 checks passed
ajschmidt8 pushed a commit to rapidsai/cudf that referenced this pull request Jan 11, 2024
Follow-up to #14644. 

Contributes to rapidsai/build-planning#7.

Similar to rapidsai/rmm#1422, this proposes splitting the `cuda-version` dependency in `dependencies.yaml` out to its own thing, separate from the bits of the CUDA Toolkit `cudf` needs.

Some other simplifications:

* removes the notebook-specific stuff added in #14722 (which I think were added specifically because `cuda-version` and CTK stuff was coupled)
* consolidates two sections with selectors only based on CUDA `{major}.{minor}`

Authors:
   - James Lamb (https://github.com/jameslamb)

Approvers:
   - Ray Douglass (https://github.com/raydouglass)
   - Bradley Dice (https://github.com/bdice)
   - Vyas Ramasubramani (https://github.com/vyasr)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants