Run all scikit-learn examples under cuml.accel in nightlies - #7960
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds infrastructure to run scikit-learn upstream examples under cuml.accel: updates .gitignore, adds a pytest example-collector plugin, a runner script to checkout and run upstream examples, an xfail rules YAML, and a CI step invoking the runner. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.py`:
- Around line 45-53: Rel.root-level examples produce rel.parent == Path('.')
which yields a module name "."; update the __init__ in example_collector.py to
detect that case before making self.module: after computing rel, compute
parent_str = str(rel.parent).replace(os.sep, ".") and if parent_str in (".", "")
set module_name = "" (or another empty/neutral name expected by the xfail
plugin), otherwise set module_name = parent_str; then set self.module =
_FakeModule(module_name) so root-level examples do not get a "." module name.
- Around line 70-73: The except block currently catches
subprocess.TimeoutExpired and raises ExampleFailed without preserving the
original traceback; modify the handler so you capture the caught exception
(e.g., except subprocess.TimeoutExpired as e:) and re-raise ExampleFailed using
exception chaining (raise ExampleFailed(f"Example timed out after {timeout}s:
{self.path.name}") from e) to retain the original subprocess.TimeoutExpired
context — update the block in the code that raises ExampleFailed from the
TimeoutExpired handler.
🪄 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: Pro
Run ID: 73f88fdf-3309-4667-8c2a-400574662eb6
📒 Files selected for processing (5)
.gitignoreci/test_python_cuml_accel_upstream.shpython/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/run-examples.shpython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-examples.yaml
betatim
approved these changes
Apr 9, 2026
betatim
left a comment
betatim
left a comment
Contributor
There was a problem hiding this comment.
I like it. There are a few more test failures that need looking at and then adding to the xfail list.
I'm in two minds wrt installing some of the missing dependencies to see if those examples would run or not. You decide if you think it is worth it or not.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ci/test_python_cuml_accel_upstream.sh (2)
17-20: Add explicit failure/timeout logging for the examples step.Right now failures rely on the global
trap ERRside effect. Adding a step-local error message makes CI triage faster when the 60-minute timeout is hit.Suggested patch
rapids-logger "scikit-learn examples" -timeout 60m ./python/cuml/cuml_accel_tests/upstream/scikit-learn/run-examples.sh \ - -n 4 --dist worksteal \ - --junitxml="${RAPIDS_TESTS_DIR}/junit-sklearn-examples.xml" +if ! timeout 60m ./python/cuml/cuml_accel_tests/upstream/scikit-learn/run-examples.sh \ + -n 4 --dist worksteal \ + --junitxml="${RAPIDS_TESTS_DIR}/junit-sklearn-examples.xml"; then + rapids-logger "scikit-learn examples failed or timed out (60m)" +fiAs per coding guidelines, for CI/build scripts: “Check for proper error handling and meaningful error messages.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/test_python_cuml_accel_upstream.sh` around lines 17 - 20, The examples step using "timeout 60m ./python/cuml/cuml_accel_tests/upstream/scikit-learn/run-examples.sh -n 4 --dist worksteal --junitxml=\"${RAPIDS_TESTS_DIR}/junit-sklearn-examples.xml\"" lacks a step-local failure/timeout message; wrap the command so you capture its exit status and on non-zero (including timeout) print a clear, timestamped error to stderr (e.g. "Examples step failed or timed out after 60m") before exiting with that status; update the invocation around the run-examples.sh call to check $? and emit the explicit log to aid CI triage.
15-20: Guard the new 60-minute examples stage behind a nightly gate (or confirm this script is nightly-only).Lines 17-19 run unconditionally whenever this script is invoked. If this script is used outside nightly jobs, this will significantly increase CI time and flake surface.
Suggested patch (use your repo-standard nightly flag)
# Run scikit-learn examples under cuml.accel -rapids-logger "scikit-learn examples" -timeout 60m ./python/cuml/cuml_accel_tests/upstream/scikit-learn/run-examples.sh \ - -n 4 --dist worksteal \ - --junitxml="${RAPIDS_TESTS_DIR}/junit-sklearn-examples.xml" +if [[ "${RAPIDS_CI_NIGHTLY:-0}" == "1" ]]; then + rapids-logger "scikit-learn examples" + timeout 60m ./python/cuml/cuml_accel_tests/upstream/scikit-learn/run-examples.sh \ + -n 4 --dist worksteal \ + --junitxml="${RAPIDS_TESTS_DIR}/junit-sklearn-examples.xml" +else + rapids-logger "Skipping scikit-learn examples (nightly-only)" +fi#!/bin/bash # Verify how ci/test_python_cuml_accel_upstream.sh is invoked and which nightly gate variable is used in this repo. fd -e yml -e yaml -e sh . | xargs rg -n -C2 \ 'test_python_cuml_accel_upstream\.sh|cuml_accel_upstream|nightly|RAPIDS_.*NIGHTLY|BUILD_TYPE|TEST_MODE'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/test_python_cuml_accel_upstream.sh` around lines 15 - 20, The scikit-learn examples block (the rapids-logger call and the timeout 60m ./python/.../run-examples.sh invocation with -n 4 --dist worksteal --junitxml=...) must be run only for nightly jobs; wrap this section in a conditional that checks the repo-standard nightly gate variable (e.g., RAPIDS_NIGHTLY true, BUILD_TYPE == "nightly", or whatever test-mode flag your CI uses) and skip or log a message when not set so normal CI runs are not slowed down.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ci/test_python_cuml_accel_upstream.sh`:
- Around line 17-20: The examples step using "timeout 60m
./python/cuml/cuml_accel_tests/upstream/scikit-learn/run-examples.sh -n 4 --dist
worksteal --junitxml=\"${RAPIDS_TESTS_DIR}/junit-sklearn-examples.xml\"" lacks a
step-local failure/timeout message; wrap the command so you capture its exit
status and on non-zero (including timeout) print a clear, timestamped error to
stderr (e.g. "Examples step failed or timed out after 60m") before exiting with
that status; update the invocation around the run-examples.sh call to check $?
and emit the explicit log to aid CI triage.
- Around line 15-20: The scikit-learn examples block (the rapids-logger call and
the timeout 60m ./python/.../run-examples.sh invocation with -n 4 --dist
worksteal --junitxml=...) must be run only for nightly jobs; wrap this section
in a conditional that checks the repo-standard nightly gate variable (e.g.,
RAPIDS_NIGHTLY true, BUILD_TYPE == "nightly", or whatever test-mode flag your CI
uses) and skip or log a message when not set so normal CI runs are not slowed
down.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3ed1ed8-bbf9-4a9a-81ba-cd20bb3181af
📒 Files selected for processing (1)
ci/test_python_cuml_accel_upstream.sh
Contributor
Author
I might install them in a follow-up. We can test locally if any of them not pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.py`:
- Around line 47-48: The code computes rel = self.path.relative_to(examples_dir)
using an unresolved self.path which can raise ValueError for symlinked files;
resolve self.path first before calling relative_to (e.g., use
self.path.resolve() or reassign self.path = self.path.resolve()), so in
ExampleItem (or wherever rel is computed) replace the relative_to call with one
that uses the resolved path to avoid symlink collection crashes during
pytest_collect_file and item construction.
🪄 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: Pro
Run ID: 5468e1ff-6b38-4edd-92f9-cd431a26a3b4
📒 Files selected for processing (2)
python/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-examples.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-examples.yaml
The CI infrastructure is too flaky in that regard. We should investigate timeouts in a follow-up.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
python/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.py (1)
52-53:⚠️ Potential issue | 🟡 MinorResolve
self.pathbefore deriving the relative example ID.Collection filters on
file_path.resolve(), but this recomputes the relative path from the unresolvedself.path. Symlinked example paths can therefore be accepted during collection and then blow up here withValueError.Suggested fix
- rel = self.path.relative_to(examples_dir) + rel = self.path.resolve().relative_to(examples_dir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.py` around lines 52 - 53, The relative path computation uses self.path.relative_to(examples_dir) with examples_dir resolved but self.path possibly unresolved, causing ValueError for symlinked paths; fix by resolving self.path first (e.g., compute resolved_path = self.path.resolve() or similar) and then derive rel = resolved_path.relative_to(examples_dir) so both sides are comparable; update references in this function to use the resolved path variable instead of raw self.path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.py`:
- Around line 114-123: The hook currently silently returns None when
--examples-dir is invalid; update pytest_collect_file to fail loudly: after
obtaining examples_dir via parent.config.getoption("examples_dir"), if
examples_dir is provided verify Path(examples_dir).exists() and is_dir(); if the
check fails raise pytest.UsageError with a clear message (include the provided
examples_dir). Then continue to resolve the path and keep the existing logic
that returns ExampleFile.from_parent(parent, path=file_path) when file_path is
under examples_dir; keep the early return None only when the option was not
supplied.
---
Duplicate comments:
In `@python/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.py`:
- Around line 52-53: The relative path computation uses
self.path.relative_to(examples_dir) with examples_dir resolved but self.path
possibly unresolved, causing ValueError for symlinked paths; fix by resolving
self.path first (e.g., compute resolved_path = self.path.resolve() or similar)
and then derive rel = resolved_path.relative_to(examples_dir) so both sides are
comparable; update references in this function to use the resolved path variable
instead of raw self.path.
🪄 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: Pro
Run ID: 159134c5-f915-4577-b315-e73326ce367a
📒 Files selected for processing (2)
python/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-examples.yaml
✅ Files skipped from review due to trivial changes (1)
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-examples.yaml
Also in test_python_scikit_learn_test.sh
csadorf
force-pushed
the
test-sklearn-examples
branch
from
April 14, 2026 17:48
a44fbc6 to
9adde64
Compare
Contributor
Author
|
I verified that the new test runs are passing on the nightlies configuration in a2e9234 . |
AyodeAwe
approved these changes
Apr 14, 2026
Contributor
Author
|
/merge |
This was referenced Jun 1, 2026
This was referenced Jun 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a pytest-based runner that executes all scikit-learn example scripts under
cuml.accelas part of the upstream acceleration test suite. The goal is to catch regressions wherecuml.accelbreaks example scripts that work with plain scikit-learn.Closes #7961