Remove cudf-polars CI timeouts - #22768
Conversation
We're investigating some persistent CI failures, many of which are made harder to debug by the timeouts in our CI scripts. For example, https://github.com/rapidsai/cudf/actions/runs/26805071862/job/79020475939#step:13:3529 has a run where the output was swallowed and the cause of the error is unclear. To ensure we get the full pytest output, this PR removes those timeouts
|
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. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR removes hardcoded timeout limits from four CI test scripts that run cudf-polars tests and adds extra-verbose ( ChangesCI test verbosity and timeout removal
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ci/run_cudf_polars_polars_tests.sh (1)
64-64: ⚡ Quick winConsider adding
-vvto the second pytest invocation for consistency.The first pytest invocation now includes
-vvfor extra verbosity, but the second invocation (line 84) does not. For consistent debugging output across both test runs, consider adding-vvto the second invocation as well.📋 Suggested addition
At line 84, add the
-vvflag to match the first invocation:python -m pytest \ + -vv \ --import-mode=importlib \🤖 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 `@ci/run_cudf_polars_polars_tests.sh` at line 64, The second pytest invocation in ci/run_cudf_polars_polars_tests.sh should include the same extra-verbosity flag as the first one; locate the second call to pytest (the later "pytest" command in the script) and add the `-vv` option so both invocations run with consistent verbosity.ci/test_cudf_polars_polars_tests.sh (1)
65-65: Timeout removal accepted; ensure CI platform has job-level timeouts.The removal of the 50-minute script-level timeout aligns with the PR objective to capture complete test output. The existing error handling (trap on line 61) will still propagate test failures correctly.
Verify that the CI platform has its own job-level timeout configured to prevent indefinite hangs if tests deadlock. This provides a safety net while allowing legitimate long-running tests to complete and produce full 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 `@ci/test_cudf_polars_polars_tests.sh` at line 65, The removal of the 50-minute timeout from the test script means you must ensure the CI platform enforces a job-level timeout; check the CI job that runs ./ci/run_cudf_polars_polars_tests.sh and either configure a job-level timeout there or add a CI-level wrapper timeout so builds cannot hang indefinitely, and keep the existing trap (the failure propagation logic set around the script) intact so test failures still propagate correctly.
🤖 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 `@ci/run_cudf_polars_polars_tests.sh`:
- Line 64: The second pytest invocation in ci/run_cudf_polars_polars_tests.sh
should include the same extra-verbosity flag as the first one; locate the second
call to pytest (the later "pytest" command in the script) and add the `-vv`
option so both invocations run with consistent verbosity.
In `@ci/test_cudf_polars_polars_tests.sh`:
- Line 65: The removal of the 50-minute timeout from the test script means you
must ensure the CI platform enforces a job-level timeout; check the CI job that
runs ./ci/run_cudf_polars_polars_tests.sh and either configure a job-level
timeout there or add a CI-level wrapper timeout so builds cannot hang
indefinitely, and keep the existing trap (the failure propagation logic set
around the script) intact so test failures still propagate correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 78443f8b-142a-4171-86e1-e5fefe991fba
📒 Files selected for processing (4)
ci/run_cudf_polars_polars_tests.shci/test_cudf_polars_polars_tests.shci/test_python_other.shci/test_wheel_cudf_polars.sh
|
/merge |
Description
We're investigating some persistent CI failures, many of which are made harder to debug by the timeouts in our CI scripts. For example, https://github.com/rapidsai/cudf/actions/runs/26805071862/job/79020475939#step:13:3529 has a run where the output was swallowed and the cause of the error is unclear.
To ensure we get the full pytest output, this PR removes those timeouts
Checklist