Add cuFile wheel dependencies for Python tests - #23392
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesCUDA cuFile requirements
Estimated code review effort: 3 (Moderate) | ~15 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)dependencies.yamlTraceback (most recent call last): Comment |
79bc5de to
7675c05
Compare
github-actions
Bot
added
libcudf
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cudf/cudf/core/groupby/groupby.py (1)
1338-1343: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not flatten the source MultiIndex restored by these branches.
DataFrameGroupBy.agginpython/cudf/cudf/core/groupby/groupby.pyLines 4297-4319 still rewrites every result withresult._data.nlevels > 1to its second level. These branches now intentionally returnmultiindex=Truefor single aggregations and empty outputs, soagg("sum")on MultiIndex columns is flattened again, while empty results can lose their empty MultiIndex metadata. The new test inpython/cudf/cudf/tests/groupby/test_agg.pyLines 809-821 will expose this regression. Restrict the collapse to aggregation-generated multi-level outputs, such as by guarding it with_is_multi_agg(func).Proposed fix
- if result._data.nlevels > 1: + if _is_multi_agg(func) and result._data.nlevels > 1:Also applies to: 1349-1368
🤖 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/core/groupby/groupby.py` around lines 1338 - 1343, In DataFrameGroupBy.agg, restrict the result-level collapse for result._data.nlevels > 1 to aggregation-generated multi-level outputs by guarding it with _is_multi_agg(func). Preserve the multiindex metadata and source MultiIndex returned by the restoration branches, including single-aggregation and empty-result paths.
🧹 Nitpick comments (1)
python/cudf/cudf/tests/groupby/test_agg.py (1)
809-821: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for empty outputs and typed MultiIndex levels.
These tests cover non-empty string-labeled MultiIndexes and flat relabeling, but not the
len(data) == 0branch ingroupby.pyLines 1329-1343 or preservation of non-objectlevel_dtypes. Add a case with only the grouping column so aggregation returns zero data columns, plus a MultiIndex containing an integer level, and compare both values and column metadata with pandas.As per coding guidelines, Python test files should cover edge cases such as empty and mixed-type inputs.
Also applies to: 824-835
🤖 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/tests/groupby/test_agg.py` around lines 809 - 821, Add tests alongside test_agg_multiindex_columns_preserved for a groupby aggregation with only the grouping column, producing zero data columns, and assert pandas and cuDF values plus column metadata match. Add a separate MultiIndex-column case containing an integer level, exercise the same aggregation, and verify non-object level_dtypes and labels are preserved against pandas; ensure both empty-output and mixed-type cases cover the relevant groupby paths.Source: Coding guidelines
🤖 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 `@dependencies.yaml`:
- Around line 986-989: Update the wildcard CUDA dependency entry for
nvidia-cufile-cu12 so the cuda: "12.*" path uses a compatible pinned version, or
change the matrix configuration to select an exact CUDA minor version; ensure
the run_cudf integration path does not install the unpinned package when invoked
with cuda=12.
---
Outside diff comments:
In `@python/cudf/cudf/core/groupby/groupby.py`:
- Around line 1338-1343: In DataFrameGroupBy.agg, restrict the result-level
collapse for result._data.nlevels > 1 to aggregation-generated multi-level
outputs by guarding it with _is_multi_agg(func). Preserve the multiindex
metadata and source MultiIndex returned by the restoration branches, including
single-aggregation and empty-result paths.
---
Nitpick comments:
In `@python/cudf/cudf/tests/groupby/test_agg.py`:
- Around line 809-821: Add tests alongside test_agg_multiindex_columns_preserved
for a groupby aggregation with only the grouping column, producing zero data
columns, and assert pandas and cuDF values plus column metadata match. Add a
separate MultiIndex-column case containing an integer level, exercise the same
aggregation, and verify non-object level_dtypes and labels are preserved against
pandas; ensure both empty-output and mixed-type cases cover the relevant groupby
paths.
🪄 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: 4561047b-f968-442d-a866-6a89dd1d73d7
📒 Files selected for processing (7)
cpp/src/strings/regex/glushkov_regcomp.cppcpp/src/strings/regex/glushkov_regcomp.hppcpp/tests/strings/split_tests.cppdependencies.yamlpython/cudf/cudf/core/groupby/groupby.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/groupby/test_agg.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
dependencies.yaml (1)
986-987: 🗄️ Data Integrity & Integration | 🟠 MajorPin the wildcard CUDA 12 cuFile dependency.
Line 987 still emits bare
nvidia-cufile-cu12forcuda: "12.*", while the pins at Lines 777-797 only apply to exact CUDA minor selectors. Wildcardrun_cudfconsumers can therefore resolve an arbitrary cuFile version, defeating the intended version-specific pinning. Add a compatible pin here or select an exact CUDA minor version.🤖 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 `@dependencies.yaml` around lines 986 - 987, Pin the nvidia-cufile-cu12 dependency in the wildcard CUDA 12 configuration instead of leaving it unversioned. Update the dependency entry adjacent to the cuda: "12.*" selector to use a compatible explicit version, while preserving the existing compatibility note and wildcard configuration.
🤖 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 `@dependencies.yaml`:
- Around line 986-987: Pin the nvidia-cufile-cu12 dependency in the wildcard
CUDA 12 configuration instead of leaving it unversioned. Update the dependency
entry adjacent to the cuda: "12.*" selector to use a compatible explicit
version, while preserving the existing compatibility note and wildcard
configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 232f8195-37a6-4575-9893-ddabc85ef193
📒 Files selected for processing (1)
dependencies.yaml
|
The cudf-polars-polars-test job failed with Assuming we need cuda-toolkit 12.0, maybe we can patch that be something like (maybe Alternatively, I suspect we're falling back for the relevant tests anyway, and so we could pass |
Co-authored-by: Gil Forsyth <gforsyth@users.noreply.github.com>
|
/merge |
This fixes some wheel packaging issues in libcudf / cudf. The main goal is to declare the CUDA runtime libraries loaded by `libcudf` wheels: NVRTC and nvJitLink. Previously we included an `nvcc` component which was needed by `numba-cuda`, but that is now handled through our dependency on `numba-cuda[cu12]` which includes the necessary CUDA wheels. In #23392, we added a dependency on cuFile, but this was incorrect. cuDF doesn't use cuFile directly, it is only used through KvikIO. rapidsai/kvikio#1013 supplies the corresponding cuFile metadata for KvikIO, so we don't need that here. This PR also adds an isolated `libcudf.load_library()` wheel smoke test before `pylibcudf` test extras are installed, so that test dependencies cannot mask missing `libcudf` wheel requirements. This kind of test revealed the issue in KvikIO's packaging, so we should add similar tests here. Authors: - Bradley Dice (https://github.com/bdice) Approvers: - James Lamb (https://github.com/jameslamb) URL: #23414
6 participants
Description
The
ci-imgschange in rapidsai/ci-imgs#408 switchedcitestwheelimages from CUDAdeveltobase, which no longer bundleslibcufile.sowith the container. Now we must install cuFile wheels in Python test environments that use CUDA wheels.CUDA 12 uses
nvidia-cufile-cu12with some caveats in the pinnings, because cuFile wheels weren't available until 12.6.3 so the[cufile]extra isn't reliable.CUDA 13 uses the
cuda-toolkit[cufile]extra.Checklist