Remove sklearn example xfails - #8090
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
This comment has been minimized.
This comment has been minimized.
7c809eb to
6a242b9
Compare
This comment has been minimized.
This comment has been minimized.
6a242b9 to
6102eb7
Compare
This comment has been minimized.
This comment has been minimized.
6102eb7 to
f08b9ed
Compare
This comment has been minimized.
This comment has been minimized.
1b86a4c to
af5f4d4
Compare
This comment has been minimized.
This comment has been minimized.
af5f4d4 to
c740c0f
Compare
This comment has been minimized.
This comment has been minimized.
|
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:
📝 WalkthroughWalkthroughThe PR narrows the active CI workflow, updates the scikit-learn examples runner and xfail configuration, changes result summarization logic, and rewrites the upstream tests README. ChangesCI narrowing and sklearn example test hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
6044b55 to
352fc89
Compare
352fc89 to
9d73463
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cuml/cuml_accel_tests/upstream/summarize-results.py (1)
309-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the JUnit counting logic
A small fixture test would lock in mixedpytest.xfail, regular skip, and error/failure cases so future changes don’t regress the pass-rate math.🤖 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 `@python/cuml/cuml_accel_tests/upstream/summarize-results.py` around lines 309 - 360, Add a regression test for the JUnit counting logic in summarize-results.py that exercises mixed testcase outcomes and verifies the pass-rate math stays correct. Use the existing counting flow around the testcase loop and passed calculation to cover regular failures, regular errors, regular skips, pytest.xfail skips, and XPASS(strict)/XPASS cases so future changes don’t double-count or misclassify outcomes. Keep the fixture small but assert the final tallies and pass rate reflect the intended behavior of the summarize-results logic.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@python/cuml/cuml_accel_tests/upstream/summarize-results.py`:
- Around line 309-360: Add a regression test for the JUnit counting logic in
summarize-results.py that exercises mixed testcase outcomes and verifies the
pass-rate math stays correct. Use the existing counting flow around the testcase
loop and passed calculation to cover regular failures, regular errors, regular
skips, pytest.xfail skips, and XPASS(strict)/XPASS cases so future changes don’t
double-count or misclassify outcomes. Keep the fixture small but assert the
final tallies and pass rate reflect the intended behavior of the
summarize-results logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8c838d75-b74e-48b5-bb44-f7e18ab4fa2d
📒 Files selected for processing (6)
.github/workflows/pr.yamlci/test_python_sklearn_examples.shpython/cuml/cuml_accel_tests/upstream/README.mdpython/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-examples.yamlpython/cuml/cuml_accel_tests/upstream/summarize-results.py
💤 Files with no reviewable changes (1)
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-examples.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pr.yaml
|
The CI runtime concerns are addressed now with a 300s per-example timeout cap. The latest sklearn examples jobs complete under 50 minutes, and both reported Like @betatim , I was not able to locally reproduce the extremely long runtimes for most of the examples; the exception was Last step is to revert the temporary CI config changes used for the test run. After that, I think this is good to merge. |
|
/merge |
Validates the upstream
scikit-learnexamples suite undercuml.accelwith the staticcuml.accelexample xfail list removed.The examples job now installs the optional dependencies needed by the upstream examples and enforces a pass-rate gate on the JUnit summary. The one remaining static example xfail is
applications::wikipedia_principal_eigenvector, which also fails withoutcuml.accelon this dependency stack.Changes
scikit-learnexample dependencies inci/test_python_sklearn_examples.sh:plotly,polars,pooch, andscikit-imagecuml.accelexample xfails fromxfail-examples.yamlValidation
The latest targeted PR CI run with the sklearn examples job completed under 50 minutes on both H100 jobs:
266 passed,14 xfailed,0 failed,0 errors,95.00%pass rate266 passed,14 xfailed,0 failed,0 errors,95.00%pass rateThe temporary
.github/workflows/pr.yamlchanges used to run the sklearn examples job in PR CI have been reverted from the final branch diff.Closes #8180