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

Install test dependencies at the same time as cuml packages. #5781

Merged
merged 4 commits into from
Feb 26, 2024

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Feb 22, 2024

This PR creates a conda environment containing both test dependencies and cuml packages. This is a workaround for some issues seen with conda being unable to downgrade from Arrow 15 (in the initial environment with test dependencies) to Arrow 14 (currently pinned by cudf, which is a dependency of cuml).

This is a partial solution for rapidsai/build-planning#22. (More work is needed for other RAPIDS repos.)

Depends on rapidsai/dependency-file-generator#67.

@github-actions github-actions bot added the ci label Feb 22, 2024
@bdice bdice added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change ci and removed ci labels Feb 22, 2024
@bdice bdice self-assigned this Feb 22, 2024
@bdice bdice mentioned this pull request Feb 22, 2024
Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

I really like the approach even though it'd be nice in the future if RDFG could handle this automatically

ci/test_cpp.sh Outdated
EOF

# TODO: install conda-merge in the CI images
pip install conda-merge
Copy link
Member

Choose a reason for hiding this comment

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

Small question: why use pip for installing conda-merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fastest to do it this way for the prototype. I am installing this into the conda environment in rapidsai/ci-imgs#112.

Copy link
Member

Choose a reason for hiding this comment

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

We may also want to add conda-merge to the base image if we find it useful. Could imagine this pattern showing up in other projects' CI

@bdice
Copy link
Contributor Author

bdice commented Feb 22, 2024

I really like the approach even though it'd be nice in the future if RDFG could handle this automatically

I'm still thinking about ways to improve this. Just wanted to get a prototype up first. What do you think about something like this, with CLI arguments for --extra-channels and --extra-packages? Other ideas welcome!

rapids-logger "Downloading artifacts from previous jobs"
CPP_CHANNEL=$(rapids-download-conda-from-s3 cpp)
PYTHON_CHANNEL=$(rapids-download-conda-from-s3 python)

rapids-dependency-file-generator \
  --output conda \
  --file_key test_python \
  --matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION}" \
  --extra-channels "${CPP_CHANNEL};${PYTHON_CHANNEL}" \
  --extra-packages "libcuml cuml" | tee env.yaml

rapids-mamba-retry env create --force -f env.yaml -n test

ci/test_cpp.sh Outdated
- libcuml-tests
EOF

# TODO: install conda-merge in the CI images
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a PR to the CI images for this: rapidsai/ci-imgs#112 but that would be unnecessary if we adopt this alternative approach: #5781 (comment)

ci/test_cpp.sh Outdated
EOF

# TODO: install conda-merge in the CI images
pip install conda-merge
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fastest to do it this way for the prototype. I am installing this into the conda environment in rapidsai/ci-imgs#112.

@bdice
Copy link
Contributor Author

bdice commented Feb 22, 2024

@dantegd If you prefer the rapids-dependency-file-generator approach proposed above, I will work on that. If this fixes CI, we might just merge it as-is and clean it up after the necessary features are added to rapids-dependency-file-generator.

ci/test_cpp.sh Outdated
Comment on lines 22 to 23
- libcuml
- libcuml-tests
Copy link
Member

Choose a reason for hiding this comment

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

Assuming that conda-merge will place CPP_CHANNEL at a sufficient priority so that we grab build artifacts, but do we lose anything also constraining the packages to that channel?

Suggested change
- libcuml
- libcuml-tests
- ${CPP_CHANNEL}::libcuml
- ${CPP_CHANNEL}::libcuml-tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good question. I designed rapidsai/dependency-file-generator#67 in such a way that the CI artifact channels will always be prepended. It's still possible that another channel could be used, but there's not a good way to inject the channel names corresponding to those packages if we go with the route of rapidsai/dependency-file-generator#67.

@dantegd
Copy link
Member

dantegd commented Feb 22, 2024

@bdice I think the rapids-dependency-file-generator is nicer for doing it in general in all RAPIDS repos, but also very happy to merge this as is since it seems to fix CI. Thanks!

@bdice
Copy link
Contributor Author

bdice commented Feb 22, 2024

I put up a PR with a feature --prepend-channels for rapids-dependency-file-generator: rapidsai/dependency-file-generator#67

Some dask test failed, seems like a network issue that a rerun will fix. Then let's merge this to unblock other PRs. I'll work on a follow-up for cuml (and the rest of RAPIDS) after we get rapidsai/dependency-file-generator#67 merged.

Copy link
Member

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Looks like we end up pulling stable packages instead of from the build channels?

https://github.com/rapidsai/cuml/actions/runs/7997932570/job/21846716467?pr=5781

We might need to tighten the pinnings on the added packages - is it reasonable to pin these to the current nightly version and update the release scripts to bump this version?

ci/test_cpp.sh Outdated

# TODO: install conda-merge in the CI images
pip install conda-merge
conda-merge test.yaml cuml.yaml > env.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Could we cat the output of this file to see the placement of the new channels in the resulting environment file?

Copy link
Member

Choose a reason for hiding this comment

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

Just checked this locally, looks like the order isn't super defined when we don't have a common channel between the merged environments:

channels:
- rapidsai
- /tmp/cpp_channel
- rapidsai-nightly
- /tmp/python_channel
- dask/label/dev
- conda-forge
- nvidia

Think that putting rapidsai below the artifact channels in the new environment file should be sufficient to get the channel ordering we desire (at least for this PR, think the approach in rapidsai/dependency-file-generator#67 should improve things)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks for checking that. I agree the approach with prepend-channels will solve this.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure about the CPP test failure, but looks like we're grabbing from the correct channel now 🎉

https://github.com/rapidsai/cuml/actions/runs/8005972527/job/21867897863?pr=5781

ci/test_cpp.sh Outdated Show resolved Hide resolved
ci/test_notebooks.sh Outdated Show resolved Hide resolved
ci/test_python_common.sh Outdated Show resolved Hide resolved
Copy link
Member

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Thanks for this work @bdice; is there anything else that needs to happen now that rapidsai/shared-workflows#178 is in for these changes to be reflected?

@bdice
Copy link
Contributor Author

bdice commented Feb 22, 2024

Thanks for this work @bdice; is there anything else that needs to happen now that rapidsai/shared-workflows#178 is in for these changes to be reflected?

😢 I made an empty commit earlier to rerun CI but didn't push it. I came back to this PR hoping it would be ready to merge by now. At this point, the CI images with the "proper" solution of rapids-dependency-file-generator are nearly published (status). I will refactor this PR to use the proper long-term solution since it will be only a few more minutes of work relative to the long CI testing time.

@bdice
Copy link
Contributor Author

bdice commented Feb 23, 2024

I updated the implementation of this PR to use --prepend-channels. I'll check the CI results in a bit, but I think this is a lot cleaner than the previous implementation.

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

LGTM, I agree this is much cleaner. Thanks!

Comment on lines +382 to +393
test_libcuml:
common:
- output_types: conda
packages:
- libcuml==24.4.*
- libcuml-tests==24.4.*
test_cuml:
common:
- output_types: conda
packages:
- libcuml==24.4.*
- cuml==24.4.*
Copy link
Member

Choose a reason for hiding this comment

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

Would we want this for wheels too?

Copy link
Contributor Author

@bdice bdice Feb 26, 2024

Choose a reason for hiding this comment

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

No, it's not needed. This dependency list is used exclusively for conda CI, to generate the environment with the PR artifacts and test dependencies in one solve. Wheels go through a different installation path and I'm not sure if it's possible to consolidate that environment generation into one step or not. Even if it is possible, it's unlikely that listing cuml-cu12 in the requirements file would be sufficient, since we need to install a specific filename for the wheel that was fetched from CI artifacts.

@bdice
Copy link
Contributor Author

bdice commented Feb 26, 2024

/merge

@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 5661d02 into rapidsai:branch-24.04 Feb 26, 2024
55 checks passed
@vyasr vyasr mentioned this pull request Mar 1, 2024
3 tasks
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request May 24, 2024
rapids-bot bot pushed a commit that referenced this pull request Oct 10, 2024
Contributes to rapidsai/build-planning#106

Proposes specifying the RAPIDS version in `conda install` calls that install CI artifacts, to reduce the risk of CI jobs picking up artifacts from other releases.

## Notes for Reviewers

This only changes the docs build, because other packages already solve in a single `conda install` with version constraints, thanks to #5781.

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

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: #6103
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci 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.

7 participants