-
Notifications
You must be signed in to change notification settings - Fork 230
Design proposal: Buffer.fill() redesign #1357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
9afc9d2 to
39351ec
Compare
| ### Option C: Hybrid Approach | ||
|
|
||
| **`Buffer.fill()`** - Flexible: | ||
| ```python | ||
| def fill(self, value, *, stream: Stream | GraphBuilder): | ||
| """Fill buffer with value pattern. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| value : int, numpy scalar, or bytes | ||
| - int: 1-byte fill (0-255) | ||
| - numpy.{u}int{8,16,32}: corresponding width fill | ||
| - bytes (1-4 bytes): raw byte pattern fill | ||
| """ | ||
| ``` | ||
|
|
||
| **`StridedMemoryView.fill()`** - Uses dtype: | ||
| ```python | ||
| def fill(self, value, *, stream: Stream | GraphBuilder): | ||
| """Fill view elements with value (uses view's dtype).""" | ||
| ``` | ||
|
|
||
| **Pros**: | ||
| - Buffer API is flexible and discoverable | ||
| - StridedMemoryView uses its natural dtype context | ||
| - Consistent with launch() scalar handling | ||
|
|
||
| **Cons**: | ||
| - More complex implementation in Buffer | ||
|
|
||
| ## Recommendation | ||
|
|
||
| **Option C (Hybrid Approach)** provides the best balance: | ||
|
|
||
| 1. **Buffer.fill()** accepts: | ||
| - `int` → 1-byte fill (value 0-255) | ||
| - `numpy.int8/uint8` → 1-byte fill | ||
| - `numpy.int16/uint16` → 2-byte fill | ||
| - `numpy.int32/uint32` → 4-byte fill | ||
| - `bytes` (length 1, 2, or 4) → raw pattern fill | ||
|
|
||
| 2. **StridedMemoryView.fill()** (new): | ||
| - Uses the view's dtype to determine width and signedness | ||
| - Value is validated against the dtype's range | ||
|
|
||
| 3. **Remove `width` parameter** from current API (breaking change, but before release) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the main part of the proposal.
| 1. **Should plain Python `int` default to 1-byte or 4-byte?** | ||
| - Proposal: 1-byte (consistent with "Buffer is untyped bytes") | ||
| - Alternative: Error if int > 255, requiring explicit dtype for larger values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python integers have a built-in to_bytes function that errors if you try to call it with something larger than 1-byte where it makes sense to limit it to 1-byte in my opinion.
|
@Andy-Jost I'm aligned to the approach you've detailed out here. My only suggested change is related to the use of Python buffer protocol instead of specifically handling bytes and numpy scalar objects. The rest looks good to me! |
Thanks for the quick feedback, Keith. I'll follow up with a code change based on these suggestions. |
|
Question:
|
Let me update the proposal and to strip it down (e.g., Option C is off the table now). If we go with Keith's suggestion to rely on the buffer protocol, we can sidestep type promotion for the basic Is the fill value Given the added complexity, I'm inclined to:
I don't have any sense of how the buffer protocol will compare with the |
Design document addressing feedback from PR NVIDIA#1318 that was prematurely merged. Proposes Option C (Hybrid Approach): - Buffer.fill() infers width from value type (int, numpy scalar, bytes) - StridedMemoryView.fill() uses view's dtype for width/signedness - Removes explicit width parameter Related to NVIDIA#1345
39351ec to
279f90c
Compare
|
New (much shorter) proposal doc is live. |
kkraus14
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach looks good to me!
| ---------- | ||
| value : int or buffer-protocol object | ||
| - int: Must be in range [0, 256). Converted to 1 byte. | ||
| - buffer-protocol object: Must be 1, 2, or 4 bytes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nitpick, but there's the collections.abc.Buffer type that exists specifically to indicate objects that support buffer protocol
I think we could special case and fast path certain object types and then fall back to buffer protocol if desired? I.E. support |
|
There is one thing that came to my mind last night. @Andy-Jost you saw @chloechia4's design doc on pythonic cuFILE support. There is one bit of cuFILE that I like very much (at least the concept of it), which is they support deferred C scalar values by accepting pointers in their read/write APIs. Our current |
Discussed: not something we can support due to driver API limitations |
Summary
This PR contains a design document proposing changes to
Buffer.fill()to address feedback from PR #1318 that was prematurely merged.Please review the design document and provide feedback before implementation proceeds.
Related
Buffer.fill()#1345cc @leofang @kkraus14 @rparolin