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.

Sign up for GitHub

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
Merged

Refactor CUDA versions in dependencies.yaml. #1422

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3eb8518
Refactor CUDA versions in dependencies.yaml.
bdice Jan 9, 2024
2dae27c
Alphabetize dependency lists.
bdice Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions dependencies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ files:
includes:
- build
- checks
- cudatoolkit
- cuda_version
- cuda
bdice marked this conversation as resolved.
Show resolved Hide resolved
- develop
- docs
- py_version
Expand All @@ -17,13 +18,13 @@ files:
test_python:
output: none
includes:
- cudatoolkit
- cuda_version
- py_version
- test_python
test_cpp:
output: none
includes:
- cudatoolkit
- cuda_version
- test_cpp
checks:
output: none
Expand All @@ -33,7 +34,7 @@ files:
docs:
output: none
includes:
- cudatoolkit
- cuda_version
- docs
- py_version
py_build:
Expand Down Expand Up @@ -102,9 +103,8 @@ dependencies:
packages:
- nvcc_linux-aarch64=11.8
- matrix:
cuda: "12.0"
cuda: "12.*"
packages:
- cuda-version=12.0
- cuda-nvcc
- output_types: [conda, requirements, pyproject]
matrices:
Expand All @@ -126,39 +126,45 @@ dependencies:
- output_types: conda
packages:
- &doxygen doxygen=1.9.1
cudatoolkit:
Copy link
Contributor Author

@bdice bdice Jan 9, 2024 •
edited
Loading

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.

cuda_version:
specific:
- output_types: conda
matrices:
- matrix:
cuda: "11.2"
packages:
- cuda-version=11.2
Comment on lines 133 to 136
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

- cudatoolkit
- matrix:
cuda: "11.4"
packages:
- cuda-version=11.4
- cudatoolkit
- matrix:
cuda: "11.5"
packages:
- cuda-version=11.5
- cudatoolkit
- matrix:
cuda: "11.6"
packages:
- cuda-version=11.6
- cudatoolkit
- matrix:
cuda: "11.8"
packages:
- cuda-version=11.8
- cudatoolkit
- matrix:
cuda: "12.0"
packages:
- cuda-version=12.0
cuda:
specific:
- output_types: conda
matrices:
- matrix:
cuda: "11.*"
packages:
- cudatoolkit
- matrix:
cuda: "12.*"
packages:
Copy link
Contributor Author

@bdice bdice Jan 9, 2024 •
edited
Loading

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.

develop:
common:
- output_types: [conda, requirements]
Expand Down
Loading