Skip to content

Explicitly select for CUDA-suffixed dependencies in rapids-make-pip-dependencies #365

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

Merged
trxcllnt merged 12 commits into from
Jul 23, 2024
Merged

Explicitly select for CUDA-suffixed dependencies in rapids-make-pip-dependencies #365

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8cd4ba2
select CUDA-suffixed dependencies for pip env
jameslamb Jul 8, 2024
c8bffd4
change more places
jameslamb Jul 22, 2024
7fa1e1f
Merge branch 'branch-24.08' of github.com:rapidsai/devcontainers into…
jameslamb Jul 22, 2024
072f5e5
expand comment
jameslamb Jul 22, 2024
5c93127
one more place
jameslamb Jul 22, 2024
52f567d
bump version
jameslamb Jul 22, 2024
23179ed
Merge branch 'more-rapids-build-backend' of github.com:jameslamb/devc…
jameslamb Jul 22, 2024
c40f39f
move matrix entry to argument
jameslamb Jul 22, 2024
9e15d70
always keep cuda, arch, and py in the rapids-dependency-file-generato…
jameslamb Jul 22, 2024
0df12df
Update features/src/rapids-build-utils/opt/rapids-build-utils/bin/mak…
jameslamb Jul 23, 2024
378b87b
Merge branch 'branch-24.08' of github.com:rapidsai/devcontainers into…
jameslamb Jul 23, 2024
3df408c
update make-conda-dependencies too
jameslamb Jul 23, 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
2 changes: 1 addition & 1 deletion features/src/rapids-build-utils/devcontainer-feature.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "NVIDIA RAPIDS devcontainer build utilities",
"id": "rapids-build-utils",
"version": "24.8.14",
"version": "24.8.15",
"description": "A feature to install the RAPIDS devcontainer build utilities",
"containerEnv": {
"BASH_ENV": "/etc/bash.bash_env"
Expand Down
47 changes: 29 additions & 18 deletions features/src/rapids-build-utils/opt/rapids-build-utils/bin/make-pip-dependencies.sh
View file
Open in desktop

@trxcllnt trxcllnt Jul 23, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we also make these changes in make-conda-dependencies.sh?

@jameslamb jameslamb Jul 23, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah I think that makes sense. I'd originally not done that when I was just going to hard-code this behavior as defaults inside make-pip-dependencies (since conda lists should never care about CUDA suffixes), but now that we're exposing --matrix-entry as an argument, I agree it should be supported in make-conda-dependencies too.

Just pushed that change.

Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,17 @@ make_pip_dependencies() {
local python_version="${PYTHON_VERSION:-$(python3 --version 2>&1 | cut -d' ' -f2)}";
python_version="$(cut -d'.' -f3 --complement <<< "${python_version}")";

# Projects that depend on different pip libraries across different CUDA versions
# (e.g. 'cudf' only depending on 'pynvjitlink' from CUDA 12.0 onwards), split up their
# dependency lists with 'cuda_suffixed={true,false}'.
#
# Here we want the suffixed versions (like 'pynvjitlink-cu12').
#
# It's ok for other RAPIDS libraries to end up in this list (like 'rmm-cu12')... in builds
# where those are also being built in the devcontainer, they'll be filtered out via
# inclusion in the 'pip_noinstall' list below.
local -r matrix_selectors="arch=$(uname -m);cuda=${cuda_version};py=${python_version};cuda_suffixed=true"

@trxcllnt trxcllnt Jul 22, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we should add this as an option to the script instead of relying on the sed at the end to filter out the package names. Something like --matrix-arg cuda_suffixed=true would allow us to pass arbitrary matrix arguments to the script in the future.

That last filtering step is really a hack around the fact that we don't have the ability to request DFG doesn't include certain packages in its output. Ideally eventually we could remove it in favor of doing the filtering in DFG.

@jameslamb jameslamb Jul 22, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok yeah, that makes sense to me. I just pushed commits adding a --matrix-entry argument to the script. Could you take a look and let me know if that's what you were looking for?

Saw the new argument show up in docs.

rapids-make-pip-env --help
rapids-make-pip-dependencies --help

Commented out

devcontainers/features/src/rapids-build-utils/opt/rapids-build-utils/bin/make-pip-dependencies.sh

Line 170 in 331dd88

| sed -E "s/-cu([0-9]+)/-cu${cuda_version_major}/g" \
and then saw these commands produce the expected lists of dependencies

# all suffixed stuff has -cu11, list includes cubinlinker but not pynvjitlink
rapids-make-pip-dependencies \
    --force \
    --matrix-entry 'cuda=11.8' \
    --matrix-entry 'arch=x86_64'

# all suffixed stuff has -cu12, list includes pynvjitlink but not cubinlinker
rapids-make-pip-dependencies \
    --force \
    --matrix-entry 'cuda=12.2' \
    --matrix-entry 'arch=x86_64'

# all suffixed stuff has -cu12, list includes pynvjitlink but not cubinlinker
# (using the defaults)
rapids-make-pip-dependencies \
    --force

I do think it still makes sense to have cuda_suffixed=true as a default, so that running rapids-make-pip-env --force interactively pulls dependencies the same way as the postStart command that runs when first starting up the images.

That's why I'm proposing not modifying these to use this new argument:

devcontainers/features/src/rapids-build-utils/devcontainer-feature.json

Line 14 in 331dd88

"rapids-post-start-command"

devcontainers/features/src/rapids-build-utils/opt/rapids-build-utils/bin/post-start-command.sh

Line 9 in 331dd88

rapids-make-"${PYTHON_PACKAGE_MANAGER}"-env "$@" || true;

@trxcllnt trxcllnt Jul 22, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we want to keep the defaults we had before, and then add the additional values to them. The devcontainer controls the arch, CUDA, and Python versions, and we don't want to allow those to be overridden.

@trxcllnt trxcllnt Jul 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I do think it still makes sense to have cuda_suffixed=true as a default, so that running rapids-make-pip-env --force interactively pulls dependencies the same way as the postStartCommand that runs when first starting up the images.

What would DFG do if it saw a matrix of --matrix arch=x86_64;cuda=12.5;cuda_suffixed=true;py=3.10;cuda_suffixed=false? Does it take the last value for cuda_suffixed?

@jameslamb jameslamb Jul 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

what would DFG do ... does it take the last value for cuda_suffixed?

I think that later values in the list override earlier ones, yes:

https://github.com/rapidsai/dependency-file-generator/blob/3c7348a3c2f85037e3be36bbbc4db567a928e882/src/rapids_dependency_file_generator/_cli.py#L97-L99

Not sure if that's intentional though, so if we started relying on it I'd want to add a test there (if there isn't one already) making it clear that that behavior needs to be preserved.

Here's why I'm thinking cuda_suffixed=true should be one of the defaults:

  • it preserves the current behavior for devcontainers
    • (rapids-make-pip-dependencies with no additional arguments generates a requirements.txt containing suffixed dependencies)
  • If we roll out dependencies.yaml changes like the ones I've described, then calls to rapids-dependency-file-generator which don't set a value for cuda_suffixed will start matching the fallback matrix instead
    • and I'm assuming we don't want everyone calling rapids-make-pip-env / rapids-make-pip-dependencies interactively to have to know about this

Consider:

# test.yaml
files:
  all:
    output: requirements
    includes:
      - depends_on_rmm
dependencies:
  depends_on_rmm:
    specific:
      - output_types: [requirements]
        matrices:
          - matrix:
              cuda: "12.*"
              cuda_suffixed: "true"
            packages:
              - rmm-cu12
          - matrix:
              cuda: "12.*"
              cuda_suffixed: "false"
            packages:
              - rmm
          - matrix:
              cuda: "11.*"
              cuda_suffixed: "true"
            packages:
              - rmm-cu11
          - matrix:
              cuda: "11.*"
              cuda_suffixed: "false"
            packages:
              - rmm
          - matrix:
            packages:
              - rmm-FALLBACK
rapids-dependency-file-generator \
  --file-key all \
  --output requirements \
  --matrix 'cuda=12.2;arch=x86_64;py=3.11' \
  --config ./test.yaml
# This file is generated by `rapids-dependency-file-generator`.
# To make changes, edit ../test.yaml and run `rapids-dependency-file-generator`.
rmm-FALLBACK

Which is what's happening in the pip devcontainers CI job on rapidsai/cudf#16183.

@jameslamb jameslamb Jul 22, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I just pushed a new commit, which I think achieves the behavior you're looking for... arch, cuda, py, and cuda_suffixed will all get default values (determined based on the devcontainer), and --matrix-entry becomes additive.

e.g. rapids-make-pip-env --matrix-entry 'cuda_suffixed=false' will still use the arch, py, and cuda appropriate for that devcontainer.

Tested again as described in #365 (comment).

@trxcllnt trxcllnt Jul 23, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I'm fine with cuda_suffixed=true being the default, for all the reasons you mentioned. I just want to make sure if we pass --matrix-entry cuda_suffixed=false, that will take precedence over the default.

@jameslamb jameslamb Jul 23, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep makes sense! Thanks for bringing it up. I'll check rapids-dependency-file-generator's tests and if there aren't any, add some to ensure we keep this behavior.

But I don't think we have to wait on that to merge this.

@jameslamb jameslamb Jul 23, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Put up rapidsai/dependency-file-generator#102 with those tests.


local pip_reqs_txts=();

eval "$(rapids-list-repos "${OPTS[@]}")";
Expand All @@ -81,12 +92,12 @@ make_pip_dependencies() {
for ((keyi=0; keyi < ${#repo_keys[@]}; keyi+=1)); do
local file="/tmp/${!repo_name}.${repo_keys[$keyi]}.requirements.txt";
pip_reqs_txts+=("${file}");
generate_requirements \
"${file}" \
--file-key "${repo_keys[$keyi]}" \
--output requirements \
--config ~/"${!repo_path}/dependencies.yaml" \
--matrix "arch=$(uname -m);cuda=${cuda_version};py=${python_version}" \
generate_requirements \
"${file}" \
--file-key "${repo_keys[$keyi]}" \
--output requirements \
--config ~/"${!repo_path}/dependencies.yaml" \
--matrix "${matrix_selectors}" \
;
done

Expand All @@ -103,12 +114,12 @@ make_pip_dependencies() {
for ((keyi=0; keyi < ${#repo_keys[@]}; keyi+=1)); do
local file="/tmp/${!repo_name}.lib${!cpp_name}.${repo_keys[$keyi]}.requirements.txt";
pip_reqs_txts+=("${file}");
generate_requirements \
"${file}" \
--file-key "${repo_keys[$keyi]}" \
--output requirements \
--config ~/"${!repo_path}/dependencies.yaml" \
--matrix "arch=$(uname -m);cuda=${cuda_version};py=${python_version}" \
generate_requirements \
"${file}" \
--file-key "${repo_keys[$keyi]}" \
--output requirements \
--config ~/"${!repo_path}/dependencies.yaml" \
--matrix "${matrix_selectors}" \
;
done
done
Expand All @@ -126,12 +137,12 @@ make_pip_dependencies() {
for ((keyi=0; keyi < ${#repo_keys[@]}; keyi+=1)); do
local file="/tmp/${!repo_name}.${!py_name}.${repo_keys[$keyi]}.requirements.txt";
pip_reqs_txts+=("${file}");
generate_requirements \
"${file}" \
--file-key "${repo_keys[$keyi]}" \
--output requirements \
--config ~/"${!repo_path}/dependencies.yaml" \
--matrix "arch=$(uname -m);cuda=${cuda_version};py=${python_version}" \
generate_requirements \
"${file}" \
--file-key "${repo_keys[$keyi]}" \
--output requirements \
--config ~/"${!repo_path}/dependencies.yaml" \
--matrix "${matrix_selectors}" \
;
done
done
Expand Down