-
Notifications
You must be signed in to change notification settings - Fork 60
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
Handle file descriptor ownership and update documents for GDS #234
Conversation
Signed-off-by: Gigon Bae <gbae@nvidia.com>
8e80b5b
to
8cf2dbc
Compare
@@ -70,6 +74,13 @@ struct EXPORT_VISIBLE CuCIMFileHandle : public std::enable_shared_from_this<CuCI | |||
deleter(this); | |||
deleter = nullptr; | |||
} | |||
|
|||
if (own_fd) |
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.
Should we check fd
too? Is there any risk it is undefined?
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.
Thanks @jakirkham for the feedback!
Will update to make sure to check fd (fd >= 0
. It's default value is -1
)
"fno = os.open(\"output.raw\", os.O_RDWR | os.O_CREAT | os.O_TRUNC)\n", | ||
"fd = CuFileDriver(fno)\n", | ||
"write_count = fd.pwrite(np_arr, 10, 5) # write 10 bytes from np_array to file starting from offset 5\n", | ||
"fd.close()\n", | ||
"os.close(fno)\n", |
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.
Might be worth writing this to use a context manager (with
statement). Just a thought 🙂
Edit: Though I guess we are illustrating this below
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.
Thanks for the review!
This is to showcase the different ways to call the methods.
Here, I couldn't use with statement for os.close(fno)
because Python's built-in os.open() returns int
type and there is no context manager available.
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.
We can always roll our own. Though maybe that's unnecessary for an example.
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.
I am not sure if we want to provide such a wrapper in Python for the file descriptor(int).
with
is supported by our API. the recommended way for using it (without close method) is described in the notebook and a description of the GDS reader page so don't need to change here?
https://github.com/NVIDIA/MagnumIO/tree/main/gds/readers/cucim-gds
with fs.open("output.raw", "w") as fd:
# Write 10 bytes from cp_array to file starting from offset 5
write_count = fd.pwrite(cp_arr, 10, 5)
Thanks Gigon! 😄 Generally looks good. Had a couple questions above 🙂 |
Signed-off-by: Gigon Bae <gbae@nvidia.com>
8cf2dbc
to
1e73270
Compare
@gpucibot merge |
Thanks Gigon! 😄 |
Fixes #233