Add pylibcudf bindings for fetch_byte_ranges_to_device - #23543
Open
Matt711 wants to merge 16 commits into
Open
Conversation
Matt711
added
feature request
|
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. |
Matt711
changed the title
fetch_byte_ranges_to_device
github-actions
Bot
added
libcudf
Member
Author
|
/ok to test eab58d0 |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds pylibcudf bindings for Parquet byte-range and page-index fetching. It validates invalid device byte ranges before allocation, adds test coverage, and exposes the module through builds, exports, and API documentation. ChangesParquet I/O bindings
Estimated code review effort: 3 (Moderate) | ~30 minutes Mergeability Score: 🔵 Low · up to The new span bindings may not fully match the underlying C++ API for const spans, which could limit valid calls or cause compatibility issues. This is a localized, mergeable risk with explicit owner follow-up to verify the declarations. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
python/pylibcudf/pylibcudf/io/parquet_io_utils.pyx (1)
66-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the additional failure modes.
The
Raisessection lists only the single-sourceValueError. The native layer now rejects negative and out-of-bounds byte ranges, so invalidbyte_rangesalso raise. Add that case so callers know both validation paths.As per path instructions, "Completeness: Check if API changes (parameters, return values, errors) are documented".
🤖 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/pylibcudf/pylibcudf/io/parquet_io_utils.pyx` around lines 66 - 70, Update the Raises documentation for the relevant parquet I/O utility to include ValueError when byte_ranges contains negative or out-of-bounds ranges, while retaining the existing source_info validation case.Source: Path instructions
python/pylibcudf/pylibcudf/libcudf/io/parquet_io_utils.pxd (1)
41-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
libcudf_exception_handlerfor the shim declaration.Line 47 declares
except +. Line 55 declaresexcept +libcudf_exception_handler. The shim callsfetch_byte_ranges_to_device_asyncandfut.get(), so libcudf exceptions can propagate through it. With plainexcept +,cudf::logic_errorandcudf::cuda_errorare translated to generic Python exceptions instead of the cuDF-specific types.♻️ Proposed change
pair[vector[device_buffer], vector[device_span[const_uint8_t]]] \ cpp_fetch_byte_ranges_to_device( datasource& source, host_span[const_byte_range_info] byte_ranges, cuda_stream_view stream, device_async_resource_ref mr, - ) except + + ) except +libcudf_exception_handlerBased on learnings, "Exceptions not handled correctly across Python/C++ boundary (missing
+libcudf_exception_handlerif notnoexcept)".🤖 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/pylibcudf/pylibcudf/libcudf/io/parquet_io_utils.pxd` around lines 41 - 47, Update the cpp_fetch_byte_ranges_to_device shim declaration to use the libcudf_exception_handler exception policy instead of plain except +, matching the adjacent shim declaration and preserving cuDF-specific exception translation.Source: Learnings
🤖 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 `@cpp/tests/io/experimental/hybrid_scan_filters_test.cpp`:
- Around line 1777-1800: Extend FetchByteRangesInvalidRanges with an
EXPECT_THROW case using a negative byte_range_info size, and add a valid fetch
case whose range ends exactly at byte 1024. Preserve the existing invalid offset
and out-of-bounds end checks while verifying the boundary-ending range is
accepted.
In `@docs/cudf/source/pylibcudf/api_docs/io/index.rst`:
- Line 20: Update the toctree entry in the IO API documentation from
experimental to ../experimental/index.rst, referencing the sibling experimental
document rather than a path under the IO directory.
In `@docs/cudf/source/pylibcudf/api_docs/io/parquet_io_utils.rst`:
- Around line 5-6: Update the module docstring in parquet_io_utils.pyx to change
“IO utilities for the Parquet.” to “IO utilities for Parquet.”, leaving the
automodule directive unchanged.
In `@python/pylibcudf/pylibcudf/io/parquet_io_utils.pxd`:
- Around line 8-18: Update the declaration of fetch_byte_ranges_to_device to use
except +libcudf_exception_handler instead of the generic except +, matching the
throwing cpp_fetch_byte_ranges_to_device wrapper. Leave the existing
synchronization and buffer ownership behavior unchanged.
In `@python/pylibcudf/pylibcudf/io/parquet_io_utils.pyx`:
- Around line 142-151: Update the page-index retrieval flow after the nogil
block to check whether buf is null before accessing data() or size(), and raise
the appropriate Python exception for an empty buffer. Remove the redundant
move() wrapper around cpp_fetch_page_index_to_host while preserving the existing
conversion to bytes for valid buffers.
- Around line 94-108: Update the result-building logic around
cpp_fetch_byte_ranges_to_device to handle empty fetched.first safely, retain
every returned device_buffer with a separate owner, and associate each span in
fetched.second with its owning buffer before calculating offsets. Use the owning
buffer’s base pointer for each gpumemoryview.byte_slice; if exactly one buffer
is guaranteed, validate that assumption explicitly before indexing.
In `@python/pylibcudf/pylibcudf/libcudf/io/datasource.pxd`:
- Around line 15-17: Update the `buffer` declarations in the Cython `cppclass`
to match `datasource::buffer`: make both `data()` and `size()` const-qualified,
remove `noexcept`, and add `except +libcudf_exception_handler` while importing
the exception handler for translation.
In `@python/pylibcudf/pylibcudf/libcudf/utilities/span.pxd`:
- Line 18: Update the device_span declarations in span.pxd to use size_t for the
constructor’s size parameter and size() return type, and mark both data() and
size() as const noexcept. Remove the int32_t sizing declaration to preserve
Python buffer sizes beyond INT32_MAX and align with cuda::std::span.
---
Nitpick comments:
In `@python/pylibcudf/pylibcudf/io/parquet_io_utils.pyx`:
- Around line 66-70: Update the Raises documentation for the relevant parquet
I/O utility to include ValueError when byte_ranges contains negative or
out-of-bounds ranges, while retaining the existing source_info validation case.
In `@python/pylibcudf/pylibcudf/libcudf/io/parquet_io_utils.pxd`:
- Around line 41-47: Update the cpp_fetch_byte_ranges_to_device shim declaration
to use the libcudf_exception_handler exception policy instead of plain except +,
matching the adjacent shim declaration and preserving cuDF-specific exception
translation.
🪄 Autofix
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: a394cc51-3b44-41ab-a4b2-979f46837654
📒 Files selected for processing (13)
cpp/src/io/parquet/io_utils/parquet_io_utils.cppcpp/tests/io/experimental/hybrid_scan_filters_test.cppdocs/cudf/source/pylibcudf/api_docs/io/index.rstdocs/cudf/source/pylibcudf/api_docs/io/parquet_io_utils.rstpython/pylibcudf/pylibcudf/io/CMakeLists.txtpython/pylibcudf/pylibcudf/io/__init__.pypython/pylibcudf/pylibcudf/io/parquet_io_utils.pxdpython/pylibcudf/pylibcudf/io/parquet_io_utils.pyipython/pylibcudf/pylibcudf/io/parquet_io_utils.pyxpython/pylibcudf/pylibcudf/io/parquet_metadata.pyxpython/pylibcudf/pylibcudf/libcudf/io/datasource.pxdpython/pylibcudf/pylibcudf/libcudf/io/parquet_io_utils.pxdpython/pylibcudf/pylibcudf/libcudf/utilities/span.pxd
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/pylibcudf/pylibcudf/libcudf/io/parquet_io_utils.pxd (1)
21-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd Python tests and a benchmark for the new Parquet utility bindings.
Cover multiple and empty ranges, invalid ranges, exception propagation, page-index results, and
gpumemoryviewlifetime. Add a benchmark forfetch_byte_ranges_to_deviceand its synchronousfut.get()path.🤖 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/pylibcudf/pylibcudf/libcudf/io/parquet_io_utils.pxd` around lines 21 - 47, Add Python tests and a benchmark for the `cpp_fetch_byte_ranges_to_device` binding, covering multiple ranges, empty ranges, invalid ranges, exception propagation, page-index results, and `gpumemoryview` lifetime. Benchmark both `fetch_byte_ranges_to_device` and the synchronous `fut.get()` path, reusing existing Parquet test and benchmark utilities where available.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.
Outside diff comments:
In `@python/pylibcudf/pylibcudf/libcudf/io/parquet_io_utils.pxd`:
- Around line 21-47: Add Python tests and a benchmark for the
`cpp_fetch_byte_ranges_to_device` binding, covering multiple ranges, empty
ranges, invalid ranges, exception propagation, page-index results, and
`gpumemoryview` lifetime. Benchmark both `fetch_byte_ranges_to_device` and the
synchronous `fut.get()` path, reusing existing Parquet test and benchmark
utilities where available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6400a10d-1544-4260-8f95-219afe4efd37
📒 Files selected for processing (6)
cpp/tests/io/experimental/hybrid_scan_filters_test.cppdocs/cudf/source/pylibcudf/api_docs/io/index.rstpython/pylibcudf/pylibcudf/io/parquet_io_utils.pyxpython/pylibcudf/pylibcudf/libcudf/io/datasource.pxdpython/pylibcudf/pylibcudf/libcudf/io/parquet_io_utils.pxdpython/pylibcudf/pylibcudf/libcudf/utilities/span.pxd
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/cudf/source/pylibcudf/api_docs/io/index.rst
- python/pylibcudf/pylibcudf/libcudf/io/datasource.pxd
- cpp/tests/io/experimental/hybrid_scan_filters_test.cpp
- python/pylibcudf/pylibcudf/io/parquet_io_utils.pyx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a sync wrapper for fetch_byte_ranges_to_device. Because the way it's used #23317, we get no benefit from resolving the futures later in cudf-polars.
Depends on #23541
Checklist