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

Allow testing of earliest/latest dependencies #1613

Merged
merged 5 commits into from
Aug 22, 2024

Conversation

seberg
Copy link
Contributor

@seberg seberg commented Jul 16, 2024

Mostly identical to gh-1606, which allows the testing of oldest/latest dependencies. What oldest/latest dependencies means exactly has to be set by each project in their dependencies.yaml file for the test env.

See rapidsai/shared-workflows#228 for the workflow changes.

This is part of preparing for 2.0 (which should just work, but in the end no need to include it in the same PR).

Modifications from draft PR:

  • I renamed it to oldest, as suggested by Matthew.
  • Noticed that the name is different between wheel and conda workflows, so modified both to include all matrix information.

(Draft, since the shared workflow should be pushed in first to revert the branch renaming probably. Need to test that the wheel workflow is correct.)

Copy link

copy-pr-bot bot commented Jul 16, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added the ci label Jul 16, 2024
@seberg
Copy link
Contributor Author

seberg commented Jul 16, 2024

/ok to test

@seberg seberg added non-breaking Non-breaking change feature request New feature or request labels Jul 16, 2024
@seberg seberg force-pushed the test-oldest branch 3 times, most recently from ddc6263 to 0047966 Compare July 16, 2024 13:13
ci/test_wheel.sh Outdated
--output requirements \
--file-key test_python \
--matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION};dependencies=${RAPIDS_DEPENDENCIES}" \
| tee oldest-dependencies.txt
Copy link
Contributor Author

@seberg seberg Jul 16, 2024

Choose a reason for hiding this comment

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

Ok, this works now also for the wheel tests. A bit unfortunate that we need the branch and maybe the dependencies.yaml entries.

On the up-side, it should be impossible to forgot to update the dependency file, as the below install will fail if there is an inconsistency:
EDIT: Sorry, pip doesn't notice it for rmm since the test requirements don't include numpy/numba. Not sure it is vital, but I'll try to see if I can make one of them fail (or the conda one already fails).

Copy link
Member

Choose a reason for hiding this comment

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

I think this could be simplified a bit, and that it'd be better to use constraints instead of requirements:

  • so we don't end up installing anything unnecessary
  • so this test can still catch issues of the form "wheel forgot to include some required dependencies in its metadata

Would you consider something like this instead?

echo "" > ./constraints.txt
if [[ $RAPIDS_DEPENDENCIES == "oldest" ]]; then
  rapids-dependency-file-generator \
        --output requirements \
        --file-key test_python \
        --matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION};dependencies=${RAPIDS_DEPENDENCIES}" \
      | tee ./constraints.txt
fi

# echo to expand wildcard before adding '[extra]' requires for pip
python -m pip install \
    -v \
    --constraint ./constraints.txt \
    "$(echo "${WHEELHOUSE}"/rmm_${RAPIDS_PY_CUDA_SUFFIX}*.whl)[test]"

ci/test_wheel.sh Outdated
if [[ $RAPIDS_DEPENDENCIES != "oldest" ]]; then
python -m pip install -v "${PIP_PACKAGE}[test]"
else
python -m pip install rapids-dependency-file-generator && pyenv rehash # TODO: include in image
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
python -m pip install rapids-dependency-file-generator && pyenv rehash # TODO: include in image

This should be included in the image now (just to note).

@harrism
Copy link
Member

harrism commented Jul 29, 2024

Should this be retargeted to 24.10 since we are in code freeze?

@seberg seberg changed the base branch from branch-24.08 to branch-24.10 July 31, 2024 11:48
@seberg seberg marked this pull request as ready for review August 15, 2024 12:21
@seberg seberg requested review from a team as code owners August 15, 2024 12:21
@seberg seberg requested a review from jameslamb August 15, 2024 12:21
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks! I'm generally supportive of this pattern. Left some suggestions for your consideration.

ci/test_wheel.sh Outdated
--output requirements \
--file-key test_python \
--matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION};dependencies=${RAPIDS_DEPENDENCIES}" \
| tee oldest-dependencies.txt
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be simplified a bit, and that it'd be better to use constraints instead of requirements:

  • so we don't end up installing anything unnecessary
  • so this test can still catch issues of the form "wheel forgot to include some required dependencies in its metadata

Would you consider something like this instead?

echo "" > ./constraints.txt
if [[ $RAPIDS_DEPENDENCIES == "oldest" ]]; then
  rapids-dependency-file-generator \
        --output requirements \
        --file-key test_python \
        --matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION};dependencies=${RAPIDS_DEPENDENCIES}" \
      | tee ./constraints.txt
fi

# echo to expand wildcard before adding '[extra]' requires for pip
python -m pip install \
    -v \
    --constraint ./constraints.txt \
    "$(echo "${WHEELHOUSE}"/rmm_${RAPIDS_PY_CUDA_SUFFIX}*.whl)[test]"

@@ -312,3 +312,14 @@ dependencies:
- cuda-nvcc
- matrix:
packages:
specific:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
specific:

This second specific: is going to clobber the first one.

Try this out:

import yaml
import pprint

with open("dependencies.yaml") as f:
    parsed = yaml.safe_load(f)

pprint.pprint(parsed["dependencies"]["test_python"])

You'll see that entry with cuda-nvcc is lost.

import yaml
import pprint

with open("dependencies.yaml") as f:
    parsed = yaml.safe_load(f)

pprint.pprint(parsed["dependencies"]["test_python"])

This is because:

  • rapids-dependency-file-generator is written in Python
  • the YAML-loading library it uses represents YAML data as a Python dictionary

rapidsai/dependency-file-generator#104 tracks the (not currently active) work to change that. A YAML linter might also be able to detect and prevent this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks (also for the other tip)! So one of the jobs shouldn't end up with the constraints, will look into it.
(Over at cudf, I had to notice that it becomes even a bit more annoying to add cupy into the mix, since it requires cupy_cu11 or cupy_cu12 depending on cuda version.)

Copy link
Member

Choose a reason for hiding this comment

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

since it requires cupy_cu11 or cupy_cu12 depending on cuda version

For sure. I'm very familiar with the requirements for these dependencies.yaml files and why the suffixes are split up the way they are (I did rapidsai/cudf#16183 and similar across RAPIDS), so @ me any time and I'm happy to help with a review.

@seberg
Copy link
Contributor Author

seberg commented Aug 16, 2024

Thanks @jameslamb adopted your suggestions, the constraints file is not just cleaner but also helpful :).
Having an invalid package name in the constraints might not be noticed for pip, but it actually helps with the cupy situation, since I can just include all of cupy==12.0, cupy-cuda11x==12.0, and cupy-cuda12x=12.0. Slightly odd but seems a lot nicer than the dance needed otherwise.

(Close/reopen to kick CI to run fully once more.)

@seberg seberg closed this Aug 16, 2024
@seberg seberg reopened this Aug 16, 2024
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Hey great, thanks for considering the suggestions!

This looks great to me... very little new complexity in exchange for a really useful testing improvement. I like it 🤩

@vyasr vyasr mentioned this pull request Aug 22, 2024
3 tasks
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 Sebastian for working on this and James for reviewing! 🙏

@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 8adedd0 into rapidsai:branch-24.10 Aug 22, 2024
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci feature request New feature or request non-breaking Non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants