CI Switch to strict xfail policy for common estimator checks - #7753
Conversation
|
Is this ready for review? |
|
Ready for review (modulo figuring out the CI failures which seem to be unrelated to the PR content) |
📝 WalkthroughSummary by CodeRabbit
WalkthroughEnabled stricter xfail enforcement in scikit-learn compatibility tests by adding Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@python/cuml/tests/test_sklearn_compatibility.py`:
- Around line 42-44: Remove the blanket skip by deleting
pytest.importorskip("sklearn", minversion="1.8") and instead detect sklearn
version at runtime (using sklearn.__version__ or packaging.version.parse) to set
a module-level flag (e.g., SKLEARN_SUPPORTS_STRICT_XFAIL). Use that flag when
calling or decorating with parameterize_with_checks (or when passing
xfail_strict) so xfail_strict=True is only used when
SKLEARN_SUPPORTS_STRICT_XFAIL is True, while allowing the rest of the sklearn
compatibility tests (parameterize_with_checks) to run on older versions; update
any uses of xfail_strict to reference this flag.
🧹 Nitpick comments (1)
python/cuml/tests/test_sklearn_compatibility.py (1)
596-622: Remove commented-out xfail entries or add a rationale.Leaving disabled keys as comments makes the strict-xfail list harder to audit. Consider removing them entirely (or add an explicit rationale if you want to keep the history in-file).
♻️ Suggested cleanup
- # "check_no_attributes_set_in_init": "GaussianRandomProjection sets attributes during init", ... - # "check_parameters_default_constructible": "GaussianRandomProjection parameters are mutated on init",
|
/merge |
I think what happened is that #7751 fixed something that used to lead to the `check_do_not_raise_errors_in_init_or_set_params` check failing. The reason we ended up seeing it in #7632 is that #7751 was merged before #7753 (and we didnt rerun the CI for that PR). Authors: - Tim Head (https://github.com/betatim) Approvers: - Jim Crist-Harif (https://github.com/jcrist) - Simon Adorf (https://github.com/csadorf) URL: #7768
…7753) This runs the scikit-learn common estimator checks in strict mode. This means a test marked as xfail that passes leads to a failure. This helps us keep the xfail list current and notice flaky checks. fixes NVIDIA#7346 Authors: - Tim Head (https://github.com/betatim) Approvers: - Simon Adorf (https://github.com/csadorf) URL: NVIDIA#7753
I think what happened is that NVIDIA#7751 fixed something that used to lead to the `check_do_not_raise_errors_in_init_or_set_params` check failing. The reason we ended up seeing it in NVIDIA#7632 is that NVIDIA#7751 was merged before NVIDIA#7753 (and we didnt rerun the CI for that PR). Authors: - Tim Head (https://github.com/betatim) Approvers: - Jim Crist-Harif (https://github.com/jcrist) - Simon Adorf (https://github.com/csadorf) URL: NVIDIA#7768
This runs the scikit-learn common estimator checks in strict mode. This means a test marked as xfail that passes leads to a failure. This helps us keep the xfail list current and notice flaky checks.
fixes #7346