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.

Sign up for GitHub

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

Jump to bottom

Enable constructing DeviceBuffers using a non-default memory resource and correctly manage memory resource lifetime in DeviceBuffer #953

Closed
wants to merge 2 commits into from
Closed

Enable constructing DeviceBuffers using a non-default memory resource and correctly manage memory resource lifetime in DeviceBuffer #953

wants to merge 2 commits into from

Conversation

shwina
Copy link
Contributor

@shwina shwina commented Jan 20, 2022

Closes #944

@shwina shwina changed the base branch from branch-22.02 to branch-22.04 January 20, 2022 18:31
@github-actions github-actions bot added the Python Related to RMM Python API label Jan 20, 2022
jakirkham
jakirkham reviewed Jan 20, 2022
View reviewed changes
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ashwin! 😄

Had a couple suggestions below

Also should we add this to functions like to_device, etc.?

python/rmm/_lib/device_buffer.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/device_buffer.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/device_buffer.pyx Show resolved Hide resolved
python/rmm/_lib/device_buffer.pyx Show resolved Hide resolved
python/rmm/_lib/device_buffer.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/device_buffer.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/device_buffer.pyx
@@ -47,7 +47,8 @@ cdef class DeviceBuffer:
def __cinit__(self, *,
uintptr_t ptr=0,
size_t size=0,
Stream stream=DEFAULT_STREAM):
Stream stream=DEFAULT_STREAM,
DeviceMemoryResource mr=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May need to cimport this and device_memory_resource above

jakirkham
jakirkham reviewed Jan 20, 2022
View reviewed changes
python/rmm/_lib/device_buffer.pyx

with nogil:
c_ptr = <const void*>ptr
c_stream_view = stream.view()
c_mr = mr.c_obj.get()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a side note, it may be possible to use get_mr here, but that method would need to be marked nogil

rmm/python/rmm/_lib/memory_resource.pyx

Lines 167 to 168 in d94bdfd

cdef device_memory_resource* get_mr(self):
return self.c_obj.get()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's probably what we should do

@jakirkham jakirkham added feature request New feature or request non-breaking Non-breaking change labels Jan 20, 2022
@github-actions
Copy link

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

@harrism
Copy link
Member

harrism commented Mar 21, 2022

@shwina can we bump this to next release?

@shwina
Copy link
Contributor Author

shwina commented Mar 21, 2022

Thanks - did so.

@github-actions
Copy link

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

@github-actions
Copy link

This PR has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates.

@jakirkham jakirkham mentioned this pull request Oct 12, 2022
Merged
3 tasks
@shwina
Copy link
Contributor Author

shwina commented Sep 13, 2023

I'm going to go ahead and close this one out as it's beyond stale.

@shwina shwina closed this Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakirkham jakirkham jakirkham left review comments

Assignees
No one assigned
Labels
feature request New feature or request inactive-30d inactive-90d non-breaking Non-breaking change Python Related to RMM Python API
Projects
RMM Project Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Potentially unsafe releasing of the GIL in DeviceBuffer
3 participants
@shwina @harrism @jakirkham