-
Notifications
You must be signed in to change notification settings - Fork 198
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
Allow specifying mr in DeviceBuffer construction, and document ownership requirements in Python/C++ interfacing #1552
Conversation
In c_from_unique_ptr we should not just rely on get_current_device_resource, but rather allow the user to pass in the memory resource they _know_ was used to allocate the buffer we are taking ownership of. So that we are backwards-compatible we default, as before, to the current device resource.
On the C++ side, device_buffers store raw pointers for the memory resource that was used in their allocation. Consequently, it is unsafe to take ownership of a device_buffer in Python unless we controlled the provenance of the memory resource that was used in its allocation. The only way to do that is to pass the memory resource from Python into C++ and then use it when constructing the DeviceBuffer. Add discussion of this with some examples and a section on pitfalls if only relying on get_current_device_resource and set_current_device_resource. - Closes rapidsai#1492
This PR changes the Python code in addition to documenting. Perhaps the PR title and description should cover that? |
@harrism good point, let me pull that out into a separate change (I wanted these so I could actually make the recommendation to do the right thing in the docs). |
I think there's no real need to separate it; just to clarify the title and description. |
Ok, done. I also added a test of the new functionality. |
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 all looks great. My only question is whether README.md
is getting too big. Maybe this belongs in the Python docs instead? e.g. guide.md
?
Can we close #1132 with this change? |
Arguably yes, a question then arises as to whether we want to do a split properly. Right now, some of the information in README is replicated in the python guide. AFAICT, the README is the "canonical" source of user documentation for the C++ API, I don't know if we would rather incorporate it into the doxygen docs. |
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.
Overall looks great, just a few small nits.
@vyasr are your concerns assuaged? |
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.
Yup all good. Thanks @wence-!
/merge |
Description
On the C++ side, device_buffers store raw pointers for the memory
resource that was used in their allocation. Consequently, it is unsafe
to take ownership of a device_buffer in Python unless we controlled
the provenance of the memory resource that was used in its allocation.
The only way to do that is to pass the memory resource from Python
into C++ and then use it when constructing the DeviceBuffer.
Add discussion of this with some examples and a section on pitfalls
if only relying on get_current_device_resource and
set_current_device_resource.
To allow Python users of
DeviceBuffer
objects to follow best practices,introduce explicit (defaulting to the current device resource)
mr
arguments in both
c_from_unique_ptr
and theDeviceBuffer
constructor.Checklist