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

Add wheel tests to CI #1416

Open
wants to merge 6 commits into
base: branch-25.02
Choose a base branch
from
Open

Add wheel tests to CI #1416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2e5c020
deps(kvikio): add kvikio to CUDA version matrices
gforsyth Dec 13, 2024
f286c95
test(wheels): enable wheel tests in CI
gforsyth Dec 13, 2024
76f1886
chore: add test_wheel script
gforsyth Dec 13, 2024
57dfa82
chore: add wheel-tests to pr builder workflow
gforsyth Dec 13, 2024
4afa054
chore: chmod +x
gforsyth Dec 13, 2024
4ab1ed0
chore: fix wheel name generation
gforsyth Dec 16, 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
10 changes: 10 additions & 0 deletions .github/workflows/pr.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ jobs:
- conda-python-tests
- docs-build
- wheel-build
- wheel-tests
secrets: inherit
uses: rapidsai/shared-workflows/.github/workflows/[email protected]
checks:
Expand Down Expand Up @@ -53,3 +54,12 @@ jobs:
# This selects "ARCH=amd64 + the latest supported Python + CUDA".
matrix_filter: map(select(.ARCH == "amd64")) | max_by([(.PY_VER|split(".")|map(tonumber)), (.CUDA_VER|split(".")|map(tonumber))]) | [.]
script: "ci/build_wheel.sh"
wheel-tests:
needs: wheel-build
secrets: inherit
uses: rapidsai/shared-workflows/.github/workflows/[email protected]
with:
build_type: pull-request
script: "ci/test_wheel.sh"
# This selects "ARCH=amd64 + the latest supported Python + CUDA".
matrix_filter: map(select(.ARCH == "amd64")) | max_by([(.PY_VER|split(".")|map(tonumber)), (.CUDA_VER|split(".")|map(tonumber))]) | [.]
Comment on lines +64 to +65
Copy link
Member

@jameslamb jameslamb Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
# This selects "ARCH=amd64 + the latest supported Python + CUDA".
matrix_filter: map(select(.ARCH == "amd64")) | max_by([(.PY_VER|split(".")|map(tonumber)), (.CUDA_VER|split(".")|map(tonumber))]) | [.]

I think we should cover the full RAPIDS PR CI matrix here.

Even though the dask-cuda wheels are pure-Python and not CPU architecture or CUDA-version dependent, with tags like this:

dask_cuda-25.2.0a10-py3-none-any.whl

... some of its dependencies do produce different variants for those different environments, and it's possible dask-cuda could interact with them in different ways that we'd want to catch. I'm reminded for example of microsoft/LightGBM#6509 and microsoft/LightGBM#6654, where I found that there was some aarch64-specific issue whose resolution was "load OpenMP as early as possible".

By the way, for PRs that matrix is only 2 jobs. You can find them here:

https://github.com/rapidsai/shared-workflows/blob/3313dce51b2cfea4601c30a986cd5980087fe2c1/.github/workflows/wheels-test.yaml#L81-L85

11 changes: 11 additions & 0 deletions ci/test_wheel.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#!/bin/bash
# Copyright (c) 2023, NVIDIA CORPORATION.
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
# Copyright (c) 2023, NVIDIA CORPORATION.
# Copyright (c) 2024, NVIDIA CORPORATION.

This is a brand new file, so the copyright date should be 2024. In a follow-up PR, could you add the verify-copyright pre-commit hook here to catch and fix stuff like this?

ref: https://github.com/rapidsai/cuspatial/blob/ad2900ded3c4a855774fc47dcec9a22323f07c0b/.pre-commit-config.yaml#L48


set -eou pipefail

RAPIDS_PY_VERSION="312" RAPIDS_PY_WHEEL_NAME="dask-cuda" rapids-download-wheels-from-s3 ./dist
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
RAPIDS_PY_VERSION="312" RAPIDS_PY_WHEEL_NAME="dask-cuda" rapids-download-wheels-from-s3 ./dist
RAPIDS_PY_WHEEL_NAME="dask-cuda" RAPIDS_PY_WHEEL_PURE="1" rapids-download-wheels-from-s3 python ./dist

Let's match the way this is spelled in other pure-Python cases like:

Keeping these scripts looking similar is helpful when we do touch-all-the-repos types of migrations.


# echo to expand wildcard before adding `[extra]` requires for pip
python -m pip install $(echo ./dist/dask_cuda*.whl)[test]

python -m pytest -n 8 ./python/dask_cuda/tests
4 changes: 2 additions & 2 deletions dependencies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,21 +188,21 @@ dependencies:
- numactl-devel-cos7-aarch64
- output_types: [requirements, pyproject]
matrices:
# kvikio should be added to the CUDA-version-specific matrices once there are wheels available
# ref: https://github.com/rapidsai/kvikio/pull/369
- matrix:
cuda: "12.*"
cuda_suffixed: "true"
packages:
- cudf-cu12==25.2.*,>=0.0.0a0
- dask-cudf-cu12==25.2.*,>=0.0.0a0
- kvikio-cu12==25.2.*,>=0.0.0a0
- ucx-py-cu12==0.42.*,>=0.0.0a0
- matrix:
cuda: "11.*"
cuda_suffixed: "true"
packages:
- cudf-cu11==25.2.*,>=0.0.0a0
- dask-cudf-cu11==25.2.*,>=0.0.0a0
- kvikio-cu11==25.2.*,>=0.0.0a0
Copy link
Member

Choose a reason for hiding this comment

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

oh nice, thank you!

- ucx-py-cu11==0.42.*,>=0.0.0a0
- matrix:
packages:
Expand Down
Loading