Pin pyarrow<24 in the cudf and pylibcudf conda recipes - #23319
Conversation
|
/okay to test 9a3a288 |
📝 WalkthroughWalkthroughChangesPyArrow runtime bounds
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/cudf/cudf/io/feather.py (1)
22-26: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider early column projection via
IpcReadOptionsinstead of post-read selection.
read_featherreads all columns then filters withpa_table.select(columns). Usingpa.ipc.IpcReadOptions(included_columns=columns)passed topa.ipc.open_filewould avoid reading unnecessary columns, improving I/O efficiency for files with many columns.🤖 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/io/feather.py` around lines 22 - 26, The read_feather flow should project requested columns during IPC reading instead of loading the full table and calling pa_table.select(columns) afterward. Create IpcReadOptions with included_columns=columns when columns is provided, pass it to pa.ipc.open_file, and preserve full-column reads when columns is None; remove the post-read selection.
🤖 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/cudf/cudf/io/feather.py`:
- Around line 29-44: Restore compatibility between DataFrame.to_feather and the
feather.to_feather wrapper by updating the to_feather signature to accept and
forward *args/**kwargs, or update the DataFrame.to_feather wrapper consistently
after verifying its signature. Preserve support for existing extra arguments
such as compression, and add the required deprecation warning if narrowing the
public API is intentional.
---
Nitpick comments:
In `@python/cudf/cudf/io/feather.py`:
- Around line 22-26: The read_feather flow should project requested columns
during IPC reading instead of loading the full table and calling
pa_table.select(columns) afterward. Create IpcReadOptions with
included_columns=columns when columns is provided, pass it to pa.ipc.open_file,
and preserve full-column reads when columns is None; remove the post-read
selection.
🪄 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: 4b7c89a3-363d-4a23-9956-bacc0b814cb5
📒 Files selected for processing (5)
ci/test_narwhals.shpython/cudf/cudf/io/feather.pypython/cudf/cudf/tests/input_output/test_feather.pypython/cudf/cudf/tests/input_output/test_orc.pypython/pylibcudf/tests/test_quantiles.py
| @ioutils.doc_to_feather() | ||
| def to_feather(df, path, *args, **kwargs): | ||
| def to_feather(df, path): | ||
| """{docstring}""" | ||
| warnings.warn( | ||
| "Using CPU via PyArrow to write Feather dataset, this may " | ||
| "be GPU accelerated in the future" | ||
| ) | ||
| # Feather doesn't support using an index | ||
| pa_table = df.to_arrow(preserve_index=False) | ||
| feather.write_feather(pa_table, path, *args, **kwargs) | ||
| # Feather v2 is the Arrow IPC file format; pyarrow.feather is | ||
| # deprecated as of pyarrow 24. Keep write_feather's default lz4 | ||
| # compression. | ||
| options = pa.ipc.IpcWriteOptions(compression="lz4") | ||
| with pa.OSFile(str(path), "wb") as sink: | ||
| with pa.ipc.new_file(sink, pa_table.schema, options=options) as writer: | ||
| writer.write_table(pa_table) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
to_feather signature narrowing breaks DataFrame.to_feather wrapper.
The new to_feather(df, path) no longer accepts *args/**kwargs, but DataFrame.to_feather (in python/cudf/cudf/core/dataframe.py) still forwards them: feather.to_feather(self, path, *args, **kwargs). Any user passing extra arguments (e.g., compression) will now get a TypeError instead of the previous behavior. As per coding guidelines, API breaking changes to public methods should include deprecation warnings.
Please verify whether DataFrame.to_feather needs updating:
#!/bin/bash
# Check the DataFrame.to_feather wrapper signature
rg -n -A5 'def to_feather' python/cudf/cudf/core/dataframe.py🤖 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/io/feather.py` around lines 29 - 44, Restore compatibility
between DataFrame.to_feather and the feather.to_feather wrapper by updating the
to_feather signature to accept and forward *args/**kwargs, or update the
DataFrame.to_feather wrapper consistently after verifying its signature.
Preserve support for existing extra arguments such as compression, and add the
required deprecation warning if narrowing the public API is intentional.
Source: Coding guidelines
dependencies.yaml pins pyarrow>=19.0.0,<24 for the conda, requirements, and pyproject outputs, but the hand-maintained conda recipes only declare pyarrow>=19.0.0 with no upper bound: cudf's run dependency and pylibcudf's run constraint. The conda test environments don't list pyarrow directly (only the oldest-deps matrix pins pyarrow==19.*), so the solver takes the bound from the built packages and the latest-deps CI jobs picked up pyarrow 25.0.0, which fails the feather, ORC, and pylibcudf quantiles tests as well as the narwhals suite. Mirror the dependencies.yaml bound into both recipes.
9a3a288 to
4c05dcc
Compare
|
/okay to test 4c05dcc |
|
/merge |
d7a32b1
into
NVIDIA:release/26.08
The latest-deps conda CI jobs started failing on every PR (e.g. [this run on NVIDIA#23272](https://github.com/rapidsai/cudf/actions/runs/29585048860)) with pyarrow 25.0.0 in the environment — feather tests/doctests (`pyarrow.feather` deprecated as of 24), pylibcudf quantiles (`SortOptions(null_placement=)` deprecated in 25), ORC tests (out-of-ns-range timestamps now raise `ArrowInvalid` instead of silently overflowing), and the narwhals suite. cudf pins `pyarrow>=19.0.0,<24` (NVIDIA#22229) in `dependencies.yaml` for the conda, requirements, and pyproject outputs — but the hand-maintained conda recipes only declare `pyarrow>=19.0.0` with **no upper bound** (`conda/recipes/cudf/recipe.yaml` run dependency and `conda/recipes/pylibcudf/recipe.yaml` run constraint). The conda test environments don't list pyarrow directly (only the oldest-deps matrix pins `pyarrow==19.*`), so the env solve takes the bound from the built packages, and with the recipes unbounded the solver picked pyarrow 25.0.0. This is also how the narwhals job got pyarrow 25: its env installs the built cudf conda package in the same solve. This mirrors the `dependencies.yaml` bound into both recipes, which constrains every conda test environment that installs the built packages. Note: the wheel jobs were unaffected because the pip/pyproject metadata carries the `<24` bound. The remaining failure in the linked run (`conda-python-other-tests`) was a runner infra flake (`nvidia-smi`: "No devices were found") — retry only. For whenever the pin is actually lifted (NVIDIA#22229): the test-suite adaptations needed for pyarrow 24/25 (feather→`pyarrow.ipc` migration, per-sort-key `null_placement`, ORC timestamp-range handling, narwhals deselects) were worked out and verified in [9a3a288](galipremsagar@9a3a288019) (previous head of this branch). Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: NVIDIA#23319
Description
The latest-deps conda CI jobs started failing on every PR (e.g. this run on #23272) with pyarrow 25.0.0 in the environment — feather tests/doctests (
pyarrow.featherdeprecated as of 24), pylibcudf quantiles (SortOptions(null_placement=)deprecated in 25), ORC tests (out-of-ns-range timestamps now raiseArrowInvalidinstead of silently overflowing), and the narwhals suite.cudf pins
pyarrow>=19.0.0,<24(#22229) independencies.yamlfor the conda, requirements, and pyproject outputs — but the hand-maintained conda recipes only declarepyarrow>=19.0.0with no upper bound (conda/recipes/cudf/recipe.yamlrun dependency andconda/recipes/pylibcudf/recipe.yamlrun constraint). The conda test environments don't list pyarrow directly (only the oldest-deps matrix pinspyarrow==19.*), so the env solve takes the bound from the built packages, and with the recipes unbounded the solver picked pyarrow 25.0.0. This is also how the narwhals job got pyarrow 25: its env installs the built cudf conda package in the same solve.This mirrors the
dependencies.yamlbound into both recipes, which constrains every conda test environment that installs the built packages.Note: the wheel jobs were unaffected because the pip/pyproject metadata carries the
<24bound. The remaining failure in the linked run (conda-python-other-tests) was a runner infra flake (nvidia-smi: "No devices were found") — retry only.For whenever the pin is actually lifted (#22229): the test-suite adaptations needed for pyarrow 24/25 (feather→
pyarrow.ipcmigration, per-sort-keynull_placement, ORC timestamp-range handling, narwhals deselects) were worked out and verified in 9a3a288019 (previous head of this branch).Checklist