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

[jameslamb]

Contributes to rapidsai/build-planning#31.

In rapidsai/cudf#16183 (and likely most other RAPIDS projects soon), I'm proposing decomposing lists of RAPIDS dependencies in dependencies.yaml into cuda_suffixed=true and cuda_suffixed=false groups, like this:

specific:
  - output_types: [requirements, pyproject]
    matrices:
      - matrix:
           cuda: "12.*"
           cuda_suffixed: "true"
         packages:
           - "rmm-cu12>=24.8.*"
      - matrix:
           cuda: "12.*"
           cuda_suffixed: "false"
         packages:
           - "rmm>=24.8.*"

This proposes always selecting those cuda_suffixed=true lists when rapids-make-pip-dependencies is called.

That should guarantee that that always generates a list of project names that can really be installed from PyPI, pypi.nvidia.com, and pypi.anaconda.org/rapidsai-wheels-nightly.

Notes for Reviewers

This should be totally safe to do even without that cuda_suffixed convention being rolled out to all the repos. rapids-dependency-file-generator does not care if you use matrix selectors that don't match anything.

Try yourself, e.g. in rmm:

rapids-dependency-file-generator \
  --output requirements \
  --file-key all \
  --matrix "cuda=12.2;nonsense=yes"

But don't we want unsuffixed dependencies for DLFW builds or unified devcontainers?

Yes, in those context we want to produce wheels with unsuffixed names (e.g. cudf) and for those wheels to depend on unsuffixed versions of everything else built from source there (e.g. rmm)... but this change doesn't affect that.

Both the suffixed and unsuffixed versions of every project with an entry in manifest.yaml is already being filtered out of the list of requirements generated by rapids-make-pip-dependencies.

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

Lines 152 to 160 in 331dd88

	for pkg in $(rapids-python-pkg-names); do
	pip_noinstall+=("${pkg}" "${pkg}-cu.*");
	if test -z "${pkg##*"-"*}"; then
	pip_noinstall+=("${pkg//"-"/"_"}" "${pkg//"-"/"_"}-cu.*")
	fi
	if test -z "${pkg##*"_"*}"; then
	pip_noinstall+=("${pkg//"_"/"-"}" "${pkg//"_"/"-"}-cu.*")
	fi
	done

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

Line 163 in 331dd88

cat "${requirement[@]}" "${pip_reqs_txts[@]}" \

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

Line 167 in 331dd88

| (grep -v -P "^($(tr -d '[:blank:]' <<< "${pip_noinstall[@]/%/|}"))(=.*|>.*|<.*)?$" || [ "$?" == "1" ]) \

This only affects the code used to generate a virtual env that builds are performed in.

[trxcllnt]

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]

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]

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]

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]

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]

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]

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]

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]

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

[trxcllnt]

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

[jameslamb]

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.