Reduce verbosity of cudf-polars-polars tests - #22543
Conversation
GitHub Actions's UI doesn't load the entire logs for the cudf-polars polars tests job (e.g. https://github.com/rapidsai/cudf/actions/runs/26022392040/job/76487038645). This removes the verbose flag we pass to pytest.
|
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:
📝 WalkthroughWalkthroughAdds pytest-timeout to test dependencies and pytest config (timeout=45), appends pytest duration-reporting flags to Polars CI test invocations, updates an SPDX year, and removes ChangesCI test runner and environments
🎯 2 (Simple) | ⏱️ ~10 minutes
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 |
|
I'll check to ensure that we still get a feedback on which test is hanging test if it's killed by the |
- add pytest-timeout - add default per-test timeout - print durations
|
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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ci/test_wheel_cudf_polars.sh (1)
78-78: 💤 Low valueConsider using consistent pytest flag syntax across CI scripts.
This line uses space-separated syntax (
--durations 10 --durations-min 10), whileci/test_python_other.sh:53uses equals syntax (--durations=10 --durations-min=10). Both are valid pytest syntax, but consistency improves maintainability across the CI infrastructure.♻️ Proposed fix for consistency
- --durations 10 --durations-min 10 \ + --durations=10 --durations-min=10 \🤖 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_wheel_cudf_polars.sh` at line 78, The pytest flags in ci/test_wheel_cudf_polars.sh use space-separated syntax for --durations and --durations-min; make them consistent with the other CI script by switching to equals syntax. Update the invocation that currently contains "--durations 10 --durations-min 10" to "--durations=10 --durations-min=10" (referencing the flags --durations and --durations-min in ci/test_wheel_cudf_polars.sh) so both CI scripts use the same pytest flag style.
🤖 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/test_wheel_cudf_polars.sh`:
- Line 78: The pytest flags in ci/test_wheel_cudf_polars.sh use space-separated
syntax for --durations and --durations-min; make them consistent with the other
CI script by switching to equals syntax. Update the invocation that currently
contains "--durations 10 --durations-min 10" to "--durations=10
--durations-min=10" (referencing the flags --durations and --durations-min in
ci/test_wheel_cudf_polars.sh) so both CI scripts use the same pytest flag style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4360b778-c6f5-41da-8648-53cda8706d4b
📒 Files selected for processing (3)
ci/run_cudf_polars_polars_tests.shci/test_python_other.shci/test_wheel_cudf_polars.sh
✅ Files skipped from review due to trivial changes (1)
- ci/run_cudf_polars_polars_tests.sh
|
It seems like some of the polars tests take at least 60 seconds to run. At least we get to see what timeouts look like: https://github.com/rapidsai/cudf/actions/runs/26164254515/job/76969242214?pr=22543#step:13:3647 In our tests, we'd be able to increase the timeout on that test specifically in the source code. For now, bumping the timeout on the CLI is probably our best option. |
|
Yeah, that one test actually tests 11250 different cases... |
|
/merge |
…_polars tests (#22669) A complement to #22543, this PR explicitly passes the `timeout` parameter to subprocess/threading/Future APIs that accept one, using a global value of 120 seconds Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) - Lawrence Mitchell (https://github.com/wence-) - Tom Augspurger (https://github.com/TomAugspurger) URL: #22669
GitHub Actions's UI doesn't load the entire logs for the cudf-polars polars tests job (e.g.
https://github.com/rapidsai/cudf/actions/runs/26022392040/job/76487038645).
This PR adjusts how we run cudf-polars tests (our own, and the polars test suite).
timeouton the command is good to have as a backup, in case the signal sent by pytest-timeout is unable to interrupt the test. This is configured through ourpyproject.tomlfor cudf-polars' tests, and on the command line for the polars tests.--durationsto print the 10 longest test durations. This will help with tuning the timeout for individual tests.-vvto reduce the verbosity of the test output-rato print the reasons for xfails / skips, in addition to failures and errors (see the pytest docs)Together, these changes should provide similar output before aside from printing the name of each passing test.