xfail test_umap_outliers - #7938
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded a Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cuml/tests/test_umap.py (1)
992-994: Consider addingstrict=Trueto the xfail marker since globalxfail_strictis not enabled.Without
strict=Trueor a globalxfail_strict=truesetting in pytest config, if this test begins passing, CI will silently mark it XPASS without failing, potentially masking that the underlying issue has been resolved. Addingstrict=Trueensures CI will fail when the test passes, alerting developers to the fix.Suggested patch
`@pytest.mark.parametrize`("random_state", [None, 42]) `@pytest.mark.parametrize`("force_serial_epochs", [True, False]) `@pytest.mark.xfail`( - reason="With current heuristics, UMAP may produce outliers on GPUs with high SM counts." + reason="With current heuristics, UMAP may produce outliers on GPUs with high SM counts.", + strict=True, ) def test_umap_outliers(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/tests/test_umap.py` around lines 992 - 994, The xfail marker on the UMAP test uses pytest.mark.xfail without strict mode, so a now-passing test would be silently marked XPASS; update the decorator in python/cuml/tests/test_umap.py (the pytest.mark.xfail applied to the UMAP test function) to include strict=True while preserving the existing reason string so CI will fail if the test unexpectedly passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/cuml/tests/test_umap.py`:
- Around line 992-994: The xfail marker on the UMAP test uses pytest.mark.xfail
without strict mode, so a now-passing test would be silently marked XPASS;
update the decorator in python/cuml/tests/test_umap.py (the pytest.mark.xfail
applied to the UMAP test function) to include strict=True while preserving the
existing reason string so CI will fail if the test unexpectedly passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42da62ac-beb5-411e-b96b-afe1f3abb38c
📒 Files selected for processing (1)
python/cuml/tests/test_umap.py
|
/merge |
Related issue: #7930
xfailing this test to unblock CI for release.
This is not a major bug but something with our algo and heuristics that makes it more "prone to failing" with good compute-power GPUs (hence the flaky failure appearing only recently after rtx pro 6000 joined the CI list).
Working on finding good heuristics by running a bunch of benchmarks on different GPUs.