CI Add third-party testing of cuml - #21897
Conversation
This runs a subset of the cuml test suite as part of the PR checks. The goal is to give PR authors a signal as to whether or not they are breaking cuml with their changes.
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
Seems fine to me. Please add a similar job in .github/workflows/test.yaml so we get nightly coverage too.
|
Done and done |
mroeschke
added
improvement
|
From looking at a subset of the failed tests I think they are all unrelated to the changes here. Looks like they are about misaligned memory in C++ tests :-/ |
Yeah I think RMM had an ABI break that's unrelated. However, I think the test discovery in the cuml job still needs a little more tweaking e.g. https://github.com/rapidsai/cudf/actions/runs/23548878530/job/68577344344?pr=21897#step:13:1416 RAPIDS logger » [03/25/26 17:26:28]
┌──────────────────────────────────────┐
| pytest cuml cuDF-compat subset |
└──────────────────────────────────────┘
ERROR: module or package not found: cuml.tests.test_array (missing __init__.py?)
============================= test session starts ==============================
platform linux -- Python 3.14.3, pytest-9.0.2, pluggy-1.6.0
benchmark: 5.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /__w/cudf/cudf
configfile: pyproject.toml
plugins: cov-7.1.0, benchmark-5.2.3, cases-3.10.1, hypothesis-6.151.9, xdist-3.8.0
collected 0 items
============================ no tests ran in 1.31s ============================= |
|
The problem is that cuml does not ship the tests in the coda package or the wheel. I see two possible ways forward: we convince cuml to ship the tests or we check them out from the cuml repo in the CI job. Shipping the tests with cuml feels like the right thing to do, checking out from the repo feels like the quick fix (but also a tedious nightmare of making sure the right tests are checked out, etc). So I'll open an issue on cuml to propose shipping the tests. |
FWIW we checkout the pandas, Polars, and Narwhals repos (a specific tag) to run their tests in cuDF CI, so it's not the biggest deal to do the same with cuML |
The conda/wheel packages do not ship the tests, so we need to go to the source.
| CUML_TESTS_DIR=/tmp/cuml/python/cuml/tests | ||
|
|
||
| rapids-logger "pytest cuml cuDF-compat subset" | ||
| timeout 15m python -m pytest --cache-clear \ |
There was a problem hiding this comment.
Is it worth adding other pytest flags here like -W error?
There was a problem hiding this comment.
As you wish. I have no opinion really on how strict you want these tests to be :D
Test files were renamed in cuml
|
One of the test files was deleted, latest commit updates this. Let's wait and see if this happens again (test re-org in cuml), if yes I think we should find a mechanism where the list of tests executed is part of the cuml repo so that when someone makes a change in cuml they can also update it. Avoiding spurious failures here. |
mroeschke
left a comment
mroeschke
left a comment
There was a problem hiding this comment.
Overall LGTM. Let us know when you think this is in a merge-able state with cuml test refactorings
|
Resolved the conflicts. For me this is ready to go. What I meant to say is that if this breaks again because of test renaming we should consider a different approach, but until then "let's merge it!" |
vyasr
left a comment
vyasr
left a comment
There was a problem hiding this comment.
Can we please make sure this job is set up properly as non-blocking before we merge?
|
The latest commit should make the job actually optional. I also temporarily added a |
📝 WalkthroughWalkthroughThis PR adds cuML compatibility testing to CI: a dependency bundle and file group, a CI script that runs selected cuML tests, and new workflow jobs in PR and test pipelines that run the script conditionally and non-blockingly. ChangescuML Compatibility Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr.yaml:
- Around line 596-598: The workflow passes undefined inputs (inputs.branch,
inputs.date, inputs.sha) into the cuml-compat-tests/custom-job invocation;
either remove those three parameters or replace them with GitHub context
variables (e.g., github.ref or github.sha and a generated date) so the push
workflow no longer references an undefined `inputs` object. Update the job call
that sets branch/date/sha to use github.ref/github.sha (or omit them if
custom-job.yaml already derives them) and ensure the invocation matches the
parameter names expected by custom-job.yaml (verify parameter names in
custom-job.yaml like branch/sha/date).
In `@ci/test_cuml_compat.sh`:
- Around line 15-17: The script currently clones cuml at RAPIDS_BRANCH and runs
tests from that head while the installed cuml package may be a different ref;
update ci/test_cuml_compat.sh so after cloning /tmp/cuml it determines the exact
ref of the installed cuML package (e.g., via a python import that reads
cuml.__git_revision__ or another package-provided git/tag identifier) and then
git checkout that specific commit/tag in /tmp/cuml instead of leaving it at the
branch tip referenced by RAPIDS_BRANCH; use the existing RAPIDS_BRANCH and
/tmp/cuml references to locate the clone and perform the checkout so tests run
against the exact package source.
- Around line 24-26: Remove the unconditional "exit 1" statement so the script
proceeds to run the test command; specifically delete the lone exit 1 that
precedes the "timeout 15m python -m pytest" invocation so the cuML compatibility
subset actually executes instead of aborting early.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d16cb9b7-bc75-4a1d-b12c-10d800f1091f
📒 Files selected for processing (4)
.github/workflows/pr.yaml.github/workflows/test.yamlci/test_cuml_compat.shdependencies.yaml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/pr.yaml (1)
597-599:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUndefined
inputs.*context incuml-compat-testsjob.Line 597–599 use
${{ inputs.branch }},${{ inputs.date }}, and${{ inputs.sha }}in a workflow declared withon: push;inputsis not defined in this context and actionlint flags it. Usegithub.*values or omit these keys ifcustom-job.yamlderives them.#!/bin/bash set -euo pipefail # Confirm trigger type and absence of reusable-workflow inputs in this file rg -n '^\s*on:|workflow_call|workflow_dispatch' .github/workflows/pr.yaml -A6 -B1 # Show all unresolved inputs.* references rg -n '\binputs\.(branch|date|sha)\b' .github/workflows/pr.yaml -C2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr.yaml around lines 597 - 599, In the cuml-compat-tests job in the workflow, the YAML is using undefined inputs (inputs.branch, inputs.date, inputs.sha); replace those references with appropriate github context variables (for example github.ref or github.sha and github.event.head_commit.timestamp or github.run_id/date as applicable) or remove these keys if custom-job.yaml derives them; update the entries that currently read `${{ inputs.branch }}`, `${{ inputs.date }}`, and `${{ inputs.sha }}` to use the correct `github.*` context or omit them so actionlint no longer flags undefined `inputs.*`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/pr.yaml:
- Around line 597-599: In the cuml-compat-tests job in the workflow, the YAML is
using undefined inputs (inputs.branch, inputs.date, inputs.sha); replace those
references with appropriate github context variables (for example github.ref or
github.sha and github.event.head_commit.timestamp or github.run_id/date as
applicable) or remove these keys if custom-job.yaml derives them; update the
entries that currently read `${{ inputs.branch }}`, `${{ inputs.date }}`, and
`${{ inputs.sha }}` to use the correct `github.*` context or omit them so
actionlint no longer flags undefined `inputs.*`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 072cae53-4155-4158-9f05-af34a256dc76
📒 Files selected for processing (4)
.github/workflows/pr.yaml.github/workflows/test.yamlci/test_cuml_compat.shdependencies.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/test.yaml
- dependencies.yaml
0f30be2 to
dd51a90
Compare
…n-tests # Conflicts: # .github/workflows/pr.yaml # dependencies.yaml
dd51a90 to
0ba43b8
Compare
|
/ok to test 0ba43b8 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/pr.yaml (2)
778-791:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit permissions block to
cuml-compat-testsjob.All other jobs using
secrets: inherit(e.g.,third-party-integration-tests-cudf-pandasat lines 759-765) explicitly declare apermissionsblock. Thecuml-compat-testsjob should follow the same pattern for consistency and least-privilege security posture.🔐 Proposed fix to add permissions
cuml-compat-tests: needs: [conda-python-build, conda-python-build-noarch, changed-files] + permissions: + actions: read + contents: read + id-token: write + packages: read + pull-requests: read secrets: inherit # zizmor: ignore[secrets-inherit] uses: rapidsai/shared-workflows/.github/workflows/custom-job.yaml@main🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr.yaml around lines 778 - 791, The cuml-compat-tests job uses secrets: inherit but lacks an explicit permissions block, which is inconsistent with other jobs in the workflow that use the same pattern (such as third-party-integration-tests-cudf-pandas). Add a permissions block to the cuml-compat-tests job configuration to match the security pattern used by other similar jobs that inherit secrets. This should be added at the same indentation level as the other top-level job properties like needs, secrets, and uses.
790-790:⚠️ Potential issue | 🟡 MinorUpdate container image to match other conda-based jobs.
The
cuml-compat-testsjob usesrapidsai/ci-conda:26.06-latestat line 790, while all other conda-based jobs (lines 476, 493, 510, 775, 829) use26.08-latest. The test script clones cuML at the matching RAPIDS branch (currentlymain), which aligns with 26.08 development. Update the container image torapidsai/ci-conda:26.08-latestfor consistency unless there is a specific reason to maintain the older version.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr.yaml at line 790, Update the container_image value in the cuml-compat-tests job from rapidsai/ci-conda:26.06-latest to rapidsai/ci-conda:26.08-latest to align with the version used by all other conda-based jobs in the workflow, ensuring consistency with the RAPIDS branch that the test script clones.
♻️ Duplicate comments (1)
.github/workflows/pr.yaml (1)
785-787:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove or replace undefined
inputs.branch,inputs.date,inputs.sha.This workflow uses a
pushtrigger (lines 2-5), notworkflow_call, so theinputsobject is undefined. These parameters should either be removed or replaced with GitHub context variables (e.g.,github.ref,github.sha).🔧 Proposed fix
cuml-compat-tests: needs: [conda-python-build, conda-python-build-noarch, changed-files] secrets: inherit # zizmor: ignore[secrets-inherit] uses: rapidsai/shared-workflows/.github/workflows/custom-job.yaml@main if: fromJSON(needs.changed-files.outputs.changed_file_groups).test_python_conda && fromJSON(needs.changed-files.outputs.changed_file_groups).neither_cudf_polars_nor_dask_cudf with: build_type: pull-request - branch: ${{ inputs.branch }} - date: ${{ inputs.date }} - sha: ${{ inputs.sha }} + branch: ${{ github.ref }} + date: ${{ github.event.head_commit.timestamp }} + sha: ${{ github.sha }} node_type: "gpu-l4-latest-1" continue-on-error: true container_image: "rapidsai/ci-conda:26.06-latest" script: "ci/test_cuml_compat.sh"Note: The same issue exists in
third-party-integration-tests-cudf-pandas(lines 770-772),pandas-tests(lines 806-808), andnarwhals-tests(lines 825-827).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr.yaml around lines 785 - 787, The workflow file uses a push trigger which does not provide an inputs context object, making inputs.branch, inputs.date, and inputs.sha undefined. Replace these references with appropriate GitHub context variables: use github.ref or github.ref_name for branch information and github.sha for the commit SHA. If date is not available from context, remove it or compute it as needed. Apply this fix at all affected locations: the primary site in .github/workflows/pr.yaml (lines 785-787 for the main job), third-party-integration-tests-cudf-pandas job (lines 770-772), pandas-tests job (lines 806-808), and narwhals-tests job (lines 825-827).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/pr.yaml:
- Around line 778-791: The cuml-compat-tests job uses secrets: inherit but lacks
an explicit permissions block, which is inconsistent with other jobs in the
workflow that use the same pattern (such as
third-party-integration-tests-cudf-pandas). Add a permissions block to the
cuml-compat-tests job configuration to match the security pattern used by other
similar jobs that inherit secrets. This should be added at the same indentation
level as the other top-level job properties like needs, secrets, and uses.
- Line 790: Update the container_image value in the cuml-compat-tests job from
rapidsai/ci-conda:26.06-latest to rapidsai/ci-conda:26.08-latest to align with
the version used by all other conda-based jobs in the workflow, ensuring
consistency with the RAPIDS branch that the test script clones.
---
Duplicate comments:
In @.github/workflows/pr.yaml:
- Around line 785-787: The workflow file uses a push trigger which does not
provide an inputs context object, making inputs.branch, inputs.date, and
inputs.sha undefined. Replace these references with appropriate GitHub context
variables: use github.ref or github.ref_name for branch information and
github.sha for the commit SHA. If date is not available from context, remove it
or compute it as needed. Apply this fix at all affected locations: the primary
site in .github/workflows/pr.yaml (lines 785-787 for the main job),
third-party-integration-tests-cudf-pandas job (lines 770-772), pandas-tests job
(lines 806-808), and narwhals-tests job (lines 825-827).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0d554fdd-9dea-4daf-a122-3a7ab39e6250
📒 Files selected for processing (2)
.github/workflows/pr.yaml.github/workflows/test.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yaml
|
/ok to test |
vyasr
changed the title
vyasr
changed the title
|
/merge |
4 participants
Description
NVIDIA/cuml#7924
This runs a subset of the cuml test suite as part of the PR checks. The goal is to give PR authors a signal as to whether or not they are breaking cuml with their changes.
The tests were chosen by looking for test files that import cudf and tests of implementations that use cudf. Locally this selection of tests runs in about 3minutes. This seems like a good trade-off between getting coverage and not taking up too much time.
I copied the job setup from
third-party-integration-tests-cudf-pandas, seemed like the most similar one.Checklist