Various cuDF CI fixes - #22677
Conversation
These tests marked as xfails recently started passing, causing CI failures in jobs like https://github.com/rapidsai/cudf/actions/runs/26497271908/job/78028482500#step:13:1669
|
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. |
github-actions
Bot
added
Python
|
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:
📝 WalkthroughWalkthroughRelax vendored Pandas pytest config by setting ChangesPytest xfail_strict configuration update
Host memory aligned allocation helpers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
TomAugspurger
added
improvement
bdice
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cudf/cudf/pandas/scripts/run-pandas-tests.sh (1)
57-58: 🏗️ Heavy liftConsider removing xfail markers if tests consistently pass.
The comment references "flaky xpasses," but the PR description states tests "have recently started passing," suggesting consistent passing rather than intermittent behavior. If these tests now pass reliably, removing their
xfailmarkers is the recommended approach per the project's testing guidelines (docs/cudf/source/cudf/developer_guide/testing.md:213-217), which documentxfail_strict=trueas the standard practice.Setting
xfail_strict = falseis appropriate for genuinely flaky tests, but if the tests pass consistently, cleaning up the markers provides clarity and maintains strict xfail handling across the test suite.🤖 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/cudf/cudf/pandas/scripts/run-pandas-tests.sh` around lines 57 - 58, The tests referenced as "flaky xpasses" now appear to be consistently passing, so update the test configuration by setting xfail_strict = true (replace the current xfail_strict = false) and remove any individual xfail markers for the tests that are now stable; also update or remove the "flaky xpasses tracked..." comment to reflect the new status. Locate the xfail_strict variable and the related xfail markers in the test config/script and adjust them accordingly so strict xfail handling is enforced and stale comments/markers are cleaned up.
🤖 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 `@python/cudf/cudf/pandas/scripts/run-pandas-tests.sh`:
- Around line 57-58: The tests referenced as "flaky xpasses" now appear to be
consistently passing, so update the test configuration by setting xfail_strict =
true (replace the current xfail_strict = false) and remove any individual xfail
markers for the tests that are now stable; also update or remove the "flaky
xpasses tracked..." comment to reflect the new status. Locate the xfail_strict
variable and the related xfail markers in the test config/script and adjust them
accordingly so strict xfail handling is enforced and stale comments/markers are
cleaned up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 73d837bd-714f-4946-b9e4-757d91293f8e
📒 Files selected for processing (1)
python/cudf/cudf/pandas/scripts/run-pandas-tests.sh
TomAugspurger
changed the title
This vendors some private rmm methods that were recently moved in rapidsai/rmm#2416. The implementations are *mostly* the same, but they're not templated (generic?) over `Alloc`.
|
Adding a "DO NOT MERGE" since I left a |
There was a problem hiding this comment.
Thanks for the fix. @TomAugspurger could you please update the PR title and description to reflect the changes?
| // https://github.com/rapidsai/rmm/pull/2416 | ||
| void* aligned_host_allocate(std::size_t bytes, std::size_t alignment) | ||
| { | ||
| assert(rmm::is_supported_alignment(alignment)); |
There was a problem hiding this comment.
I'm not sure, but the version in RMM has it: https://github.com/rapidsai/rmm/blob/039bdc51137d701337f1ff0cb447a3dc71cfcc7d/cpp/include/rmm/detail/aligned.hpp#L47
Signed-off-by: niranda perera <niranda.perera@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cudf_polars/cudf_polars/engine/core.py (1)
550-599: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAdd targeted unit coverage and a benchmark for this collective-path change.
all_gather_host_datachangedAllGatherconstruction behavior; please add a unit test (ordered bytes round-trip across ranks) and a unit benchmark to prevent regressions on this hot path.As per coding guidelines,
**/*.{cpp,cu,py,pyx}: Add unit tests and unit benchmarks for code contributions.🤖 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/cudf_polars/cudf_polars/engine/core.py` around lines 550 - 599, Add a unit test and a micro-benchmark that exercise the changed AllGather construction path via the all_gather_host_data function: write a test that creates a mock or test Communicator with multiple ranks (or uses the test harness communicator), a BufferResource, and calls all_gather_host_data with distinct per-rank bytes, then asserts the returned list length equals world size and that element i equals the bytes sent by rank i (use PackedData.from_host_bytes indirectly by calling all_gather_host_data). Also add a small unit benchmark that times repeated calls to all_gather_host_data with a representative payload size to detect regressions on the hot path; place both files alongside existing test/benchmark suites following the repository naming conventions and import AllGather, Communicator, BufferResource, and PackedData to set up the scenario.
🤖 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.
Outside diff comments:
In `@python/cudf_polars/cudf_polars/engine/core.py`:
- Around line 550-599: Add a unit test and a micro-benchmark that exercise the
changed AllGather construction path via the all_gather_host_data function: write
a test that creates a mock or test Communicator with multiple ranks (or uses the
test harness communicator), a BufferResource, and calls all_gather_host_data
with distinct per-rank bytes, then asserts the returned list length equals world
size and that element i equals the bytes sent by rank i (use
PackedData.from_host_bytes indirectly by calling all_gather_host_data). Also add
a small unit benchmark that times repeated calls to all_gather_host_data with a
representative payload size to detect regressions on the hot path; place both
files alongside existing test/benchmark suites following the repository naming
conventions and import AllGather, Communicator, BufferResource, and PackedData
to set up the scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cdaae984-cc6c-4129-8ee0-c7f6d3cecd20
📒 Files selected for processing (1)
python/cudf_polars/cudf_polars/engine/core.py
TomAugspurger
changed the title
|
/merge |
|
@TomAugspurger thanks for tying up the loose ends. |
Reverts the temporary `xfail_strict=false` setting introduced in #22677. If flaky xpasses surface, we should add them to the skip list and investigate individually rather than globally disabling strict xfail. Leaving `xfail_strict = false` silently allows tests that should be failing to pass without anyone noticing, and then they could start failing again later. We want to make sure we're always trending upwards in our pass rate. Closes #22681 Authors: - Vyas Ramasubramani (https://github.com/vyasr) - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) URL: #22913
7 participants
Description
This has several fixes for cudf CI
detailfunctions from RMM that were changed in Move public memory resource definitions to source files rapidsai/rmm#2416StatisticsInitial stats cleanup rapidsai/rapidsmpf#1009.