Handle sklearn example OpenML network failures - #8205
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR expands the network-error substring list, adds a helper to find the first matching pattern in process output, and applies detection to stdout+stderr combined to emit ExampleNetworkError and xfail with the matched pattern. ChangesEnhanced network error detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@python/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.py`:
- Around line 115-126: The current failure path truncates and raises only
stderr, losing stdout diagnostics; modify the block in ExampleCollector (where
stderr/result and pattern = _network_error_pattern(result.stderr +
result.stdout) are used) to preserve stdout in the raised ExampleFailed message:
construct a combined diagnostics string that includes both stdout and stderr
(apply the same 4000-char truncation to the tail of the combined text or to
stderr and stdout individually as desired), and raise ExampleFailed with that
combined text while keeping the existing network-error xfail branch that raises
pytest.xfail(…) for ExampleNetworkError.
🪄 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: 32b108a0-a1d0-42ef-b3e4-6152bde1d7e4
📒 Files selected for processing (1)
python/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.py
9a059d1 to
21055f6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
python/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.py (1)
124-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
stdoutin the non-xfail failure path.Line 116 correctly inspects both streams for network errors, but lines 124-126 still truncate and raise only
stderr. When failures appear instdoutbut aren't network-related, their diagnostics are lost.Suggested fix
if result.returncode != 0: - stderr = result.stderr - pattern = _network_error_pattern(result.stderr + result.stdout) + output = result.stderr + result.stdout + pattern = _network_error_pattern(output) if pattern: warnings.warn( f"Example {self.path.name} failed due to network error" f" ({pattern})", ExampleNetworkError, ) pytest.xfail(reason=f"Network error: {pattern}") - if len(stderr) > 4000: - stderr = "...\n" + stderr[-4000:] - raise ExampleFailed(stderr) + if len(output) > 4000: + output = "...\n" + output[-4000:] + raise ExampleFailed(output)🤖 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/scikit-learn/example_collector.py` around lines 124 - 126, When raising ExampleFailed currently only stderr is included, lose diagnostics from stdout; update the failure path that raises ExampleFailed to include stdout as well (e.g., combine truncated stdout and stderr into the error message) using the same truncation logic you apply to stderr (keep last 4000 chars, prefix with "...\n" if truncated). Modify the block around the raise so ExampleFailed receives both streams (reference symbols: ExampleFailed, stdout, stderr) while leaving the earlier network-error inspection intact.
🤖 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 `@python/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.py`:
- Around line 124-126: When raising ExampleFailed currently only stderr is
included, lose diagnostics from stdout; update the failure path that raises
ExampleFailed to include stdout as well (e.g., combine truncated stdout and
stderr into the error message) using the same truncation logic you apply to
stderr (keep last 4000 chars, prefix with "...\n" if truncated). Modify the
block around the raise so ExampleFailed receives both streams (reference
symbols: ExampleFailed, stdout, stderr) while leaving the earlier network-error
inspection intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d4a696b9-d944-4271-86de-01d3eb46b044
📒 Files selected for processing (1)
python/cuml/cuml_accel_tests/upstream/scikit-learn/example_collector.py
Treats `sklearn.datasets._openml.OpenMLError` as an external OpenML failure in sklearn example collection so missing OpenML datasets xfail instead of failing the examples job. Follow-up to #8205
Treat additional connection, HTTP, requests, socket, and SSL failures from sklearn example runs as network xfails, and scan stdout as well as stderr so OpenML fetch failures do not fail the examples job.
Closes #8204