-
Notifications
You must be signed in to change notification settings - Fork 217
Add Handwritten Bindings for CuFileRead and CuFileWrite #1182
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
base: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
2 files reviewed, no comments
| status = cuFileDriverClose_v2() | ||
| check_status(status) | ||
|
|
||
| cpdef read(intptr_t fh, intptr_t buf_ptr_base, size_t size, off_t file_offset, off_t buf_ptr_offset): |
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.
Read should also have similar documentation as Write.
Can you also updates the tests to check for bytes returned?
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.
Sorry, I missed that this docstring was different when I approved the related cybind MR. We will also need to file another MR to update this over there.
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.
Oh yes whoops added to this repo and corresponding cybind MR :)
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 looks really good, thanks.
I think the remaining work is just to:
- Add the declarations back in the
.pxdfile by hand-writing them in the template (see what @leofang says about that) - Add tests that confirm we are getting the expected integer result when calling read and write.
Yes let's do this. (Sourab requested this too.) |
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.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The developer has addressed previous review comments by:
-
Updated documentation for
readfunction: Thereadfunction now includes a complete docstring matching the style of thewritefunction, documenting parameters, return value, and linking to the C API reference. -
Added test assertions: All three CuFile test functions (
test_cufile_read_write,test_cufile_read_write_host_memory, andtest_cufile_read_write_large) now verify that bytes written and read match expectations. Each test validates thatbytes_writtenequals the requestedwrite_size,bytes_readequalswrite_size, and the two counts match each other.
These changes complete the transition from auto-generated to handwritten bindings for CuFileRead and CuFileWrite, enabling Python users to verify the actual number of bytes transferred in I/O operations. The handwritten implementations capture and return the ssize_t status from the underlying C API calls, which represents the byte count on success. This maintains parity with the native CuFile C API where these functions return the number of bytes transferred.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| cuda_bindings/cuda/bindings/cufile.pyx | 5/5 | Added complete docstring to read function matching write function documentation style |
| cuda_bindings/tests/test_cufile.py | 5/5 | Added assertions in three test functions to verify bytes written and read match expected sizes |
| cuda_bindings/cuda/bindings/cufile.pxd | 5/5 | No new changes since last review - previously removed declarations for handwritten implementations |
Confidence score: 5/5
- This PR is safe to merge with minimal risk
- All previous review comments have been addressed: documentation was added for the
readfunction and comprehensive test assertions were implemented to validate the returned byte counts - No files require special attention - the changes are straightforward documentation and test validation improvements
3 files reviewed, no comments
Description
This PR allows us to specify that the generated Python bindings for CuFileRead and CuFileWrite return bytes read or written, respectively. This is to ensure parity with the CuFile APIs and is helpful for verifying the contents read from a buffer or written from a buffer.