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

Allow testing of earliest/latest dependencies #1613

Merged
merged 5 commits into from
Aug 22, 2024
Merged

Allow testing of earliest/latest dependencies #1613

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ea36861
Use earliest dependencies in conda tests.
bdice Jul 11, 2024
0b11b2e
Rename earliest to oldest
seberg Jul 16, 2024
9abbe26
Use oldest test dependencies from dependencies.yaml in wheel tests
seberg Jul 15, 2024
4a93f1c
Address James review comments
seberg Aug 16, 2024
82ff07e
Merge branch 'branch-24.10' into test-oldest
seberg Aug 22, 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 ci/test_python.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ rapids-logger "Create test conda environment"
rapids-dependency-file-generator \
--output conda \
--file-key test_python \
--matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION}" | tee env.yaml
--matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION};dependencies=${RAPIDS_DEPENDENCIES}" | tee env.yaml

rapids-mamba-retry env create --yes -f env.yaml -n test
set +u
Expand Down
15 changes: 13 additions & 2 deletions ci/test_wheel.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,19 @@ set -eou pipefail
RAPIDS_PY_CUDA_SUFFIX="$(rapids-wheel-ctk-name-gen ${RAPIDS_CUDA_VERSION})"
WHEELHOUSE="${PWD}/dist/"
RAPIDS_PY_WHEEL_NAME="rmm_${RAPIDS_PY_CUDA_SUFFIX}" rapids-download-wheels-from-s3 python "${WHEELHOUSE}"
PIP_PACKAGE=$(echo "${WHEELHOUSE}"/rmm_"${RAPIDS_PY_CUDA_SUFFIX}"*.whl | head -n1)

# echo to expand wildcard before adding '[extra]' requires for pip
python -m pip install -v "$(echo "${WHEELHOUSE}"/rmm_${RAPIDS_PY_CUDA_SUFFIX}*.whl)[test]"
# Use `package[test]` to install latest test dependencies or explicitly install oldest.
if [[ $RAPIDS_DEPENDENCIES != "oldest" ]]; then
python -m pip install -v "${PIP_PACKAGE}[test]"
else
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 oldest-dependencies.txt
Copy link
Contributor Author

@seberg seberg Jul 16, 2024 •
edited
Loading

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]"


python -m pip install -v "$PIP_PACKAGE" -r oldest-dependencies.txt
fi

python -m pytest ./python/rmm/rmm/tests
11 changes: 11 additions & 0 deletions dependencies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

- output_types: [conda, requirements, pyproject]
matrices:
- matrix:
dependencies: "oldest"
packages:
- numba==0.57.*
- numpy==1.23.*
# No pinnings except for oldest dependencies
- matrix:
packages:
Loading