Skip to content
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

Merged
merged 7 commits into from
May 16, 2024

Conversation

wence-
Copy link
Contributor

@wence- wence- commented May 3, 2024

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 the DeviceBuffer constructor.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@wence- wence- requested a review from a team as a code owner May 3, 2024 14:21
@github-actions github-actions bot added the Python Related to RMM Python API label May 3, 2024
@wence- wence- added doc Documentation non-breaking Non-breaking change improvement Improvement / enhancement to an existing function and removed Python Related to RMM Python API labels May 3, 2024
wence- added 2 commits May 3, 2024 14:41
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
@github-actions github-actions bot added the Python Related to RMM Python API label May 3, 2024
@wence- wence- added non-breaking Non-breaking change and removed non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels May 3, 2024
@harrism
Copy link
Member

harrism commented May 6, 2024

This PR changes the Python code in addition to documenting. Perhaps the PR title and description should cover that?

@wence-
Copy link
Contributor Author

wence- commented May 7, 2024

@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).

@harrism
Copy link
Member

harrism commented May 7, 2024

I think there's no real need to separate it; just to clarify the title and description.

@wence- wence- changed the title Document ownership requirements in Python/C++ interfacing Allow specifying mr in DeviceBuffer construction, and document ownership requirements in Python/C++ interfacing May 7, 2024
@wence-
Copy link
Contributor Author

wence- commented May 7, 2024

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.

Copy link
Member

@harrism harrism left a 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?

@harrism
Copy link
Member

harrism commented May 9, 2024

Can we close #1132 with this change?

@wence-
Copy link
Contributor Author

wence- commented May 9, 2024

Can we close #1132 with this change?

Yes, though I didn't cover as many APIs as that one (exposing everywhere is tracked in #1515).

@wence-
Copy link
Contributor Author

wence- commented May 9, 2024

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?

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.

Copy link
Contributor

@vyasr vyasr left a 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.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
python/rmm/rmm/tests/test_rmm.py Show resolved Hide resolved
@harrism
Copy link
Member

harrism commented May 15, 2024

@vyasr are your concerns assuaged?

Copy link
Contributor

@vyasr vyasr left a 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-!

README.md Show resolved Hide resolved
@wence-
Copy link
Contributor Author

wence- commented May 16, 2024

/merge

@rapids-bot rapids-bot bot merged commit 8ee39ad into rapidsai:branch-24.06 May 16, 2024
57 checks passed
@wence- wence- deleted the wence/fix/1492 branch May 16, 2024 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation non-breaking Non-breaking change Python Related to RMM Python API
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[DOC] Document memory resource lifetime requirements when taking ownership of device buffers in Python
3 participants