Add basic slicing support to pylibcudf's gpumemoryview - #23541
Merged
rapids-bot[bot] merged 5 commits intoAug 5, 2026
Conversation
Matt711
added
feature request
Matt711
marked this pull request as ready for review
August 4, 2026 20:58
github-actions
Bot
added
Python
Matt711
changed the title
gpumemoryview
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds ChangesGPU memory view slicing
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🧹 Nitpick comments (1)
python/pylibcudf/tests/test_gpumemoryview.py (1)
77-80: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTest the parent-lifetime contract.
test_slicekeepsgvalive until the child view is consumed. It cannot detect a regression where_slicestops assigningv.obj = parent. Create a child from a temporary parent, release the parent references, force collection, and then consume the child.🤖 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/tests/test_gpumemoryview.py` around lines 77 - 80, Update test_slice to create the sliced child from a temporary parent, remove all parent references, and force garbage collection before calling to_pylist. Ensure the test still compares the child contents with the expected np_array byte slice, thereby validating that the child retains its parent through the _slice lifetime contract.
🤖 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/pylibcudf/pylibcudf/gpumemoryview.pyx`:
- Around line 98-109: Add a complete public-method docstring to
gpumemoryview.__getitem__ describing the slice parameter and byte-based bounds,
the step=1 requirement, the returned |u1 view and parent retention, and the
TypeError and ValueError cases. Keep the existing slicing behavior unchanged.
- Around line 96-109: Update __len__ to return self.nbytes so its unit matches
the byte offsets used by __getitem__ and _slice. Preserve the existing slice
validation and byte-view behavior, and add a regression test using a non-u1
dtype that verifies length and boundary slicing are byte-based.
---
Nitpick comments:
In `@python/pylibcudf/tests/test_gpumemoryview.py`:
- Around line 77-80: Update test_slice to create the sliced child from a
temporary parent, remove all parent references, and force garbage collection
before calling to_pylist. Ensure the test still compares the child contents with
the expected np_array byte slice, thereby validating that the child retains its
parent through the _slice lifetime contract.
🪄 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: 2793d433-4e09-4474-9d21-891928080960
📒 Files selected for processing (3)
python/pylibcudf/pylibcudf/gpumemoryview.pyipython/pylibcudf/pylibcudf/gpumemoryview.pyxpython/pylibcudf/tests/test_gpumemoryview.py
3 tasks
Matt711
marked this pull request as draft
August 4, 2026 21:55
|
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
force-pushed
the
fea/pylibcudf/gpumemoryview-slicing
branch
from
August 4, 2026 22:19
e5698d4 to
e29dffb
Compare
Member
Author
|
/ok to test 2f06c8e |
Matt711
marked this pull request as ready for review
August 5, 2026 01:27
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
python/pylibcudf/pylibcudf/gpumemoryview.pyx (2)
99-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the
byte_slicedocstring.The
Raisessection documents onlyTypeError. Document that only unit steps are supported, that non-unit or zero steps raiseValueError, and that out-of-range or reversed ranges produce zero-length views.Proposed documentation update
gpumemoryview A ``|u1`` view of the requested byte range. The returned view holds a reference to the parent buffer, keeping it alive. Raises ------ TypeError If ``s`` is not a slice. + ValueError + If the slice step is not 1.🤖 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/gpumemoryview.pyx` around lines 99 - 117, Complete the docstring for byte_slice by documenting that only unit slice steps are supported, with non-unit or zero steps raising ValueError, and that out-of-range or reversed ranges return zero-length views.Source: Coding guidelines
12-19: 📐 Maintainability & Code Quality | 🔵 TrivialRerun the Cython build and focused tests.
This
.pyxfile changed. Rerun the repository's standard Python/Cython build andpython/pylibcudf/tests/test_gpumemoryview.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/pylibcudf/pylibcudf/gpumemoryview.pyx` around lines 12 - 19, Rerun the repository’s standard Python/Cython build after the `_slice` change, then execute the focused `python/pylibcudf/tests/test_gpumemoryview.py` test suite and address any failures.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 `@python/pylibcudf/pylibcudf/gpumemoryview.pyx`:
- Around line 12-18: Validate that the parent passed to _slice is C-contiguous
before constructing and publishing the raw byte view. Reject byte slicing for
non-contiguous gpumemoryview instances, using the existing CUDA Array Interface
strides/layout information, while preserving the current contiguous |u1 view
behavior.
- Line 18: Update the CUDA Array Interface construction in gpumemoryview to
preserve the parent interface’s stream metadata: when the parent interface
includes stream, copy that value into the sliced interface, while leaving it
absent when the parent has no stream.
- Line 18: Update the `v.cai` construction in `gpumemoryview` to set the data
read-only flag from `parent.cai["data"][1]` instead of hard-coding `False`,
preserving the parent’s mutability for child views.
---
Nitpick comments:
In `@python/pylibcudf/pylibcudf/gpumemoryview.pyx`:
- Around line 99-117: Complete the docstring for byte_slice by documenting that
only unit slice steps are supported, with non-unit or zero steps raising
ValueError, and that out-of-range or reversed ranges return zero-length views.
- Around line 12-19: Rerun the repository’s standard Python/Cython build after
the `_slice` change, then execute the focused
`python/pylibcudf/tests/test_gpumemoryview.py` test suite and address any
failures.
🪄 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: 0ea183dc-0944-4878-8176-ea54964c1b23
📒 Files selected for processing (3)
python/pylibcudf/pylibcudf/gpumemoryview.pyipython/pylibcudf/pylibcudf/gpumemoryview.pyxpython/pylibcudf/tests/test_gpumemoryview.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/pylibcudf/tests/test_gpumemoryview.py
TomAugspurger
approved these changes
Aug 5, 2026
mroeschke
reviewed
Aug 5, 2026
Member
Author
|
/merge |
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
5 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 support for byte slicing pylibcudf
gpumemoryviews. Used in #23317 where we make a single device allocation for all byte ranges and then slice it into sub-views when passing them to hybrid scan APIs.Checklist