-
Notifications
You must be signed in to change notification settings - Fork 207
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
[REVIEW] Deprecate CNMeM #466
Conversation
@kkraus14 I'm going to add some more changes -- expose per-device resource APIs in Python since we want to deprecate the old ones (get/set_default_resource) along with deprecating cnmem... |
Can you mark as WIP unless you're planning on doing those in a follow up PR 😄 |
|
[each_mr.c_obj.get() is not NULL | ||
for each_mr in _per_device_mrs.values()] |
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 think we can remove the brackets here as this just unnecessarily creates an intermediate Python list instead of using a generator expression:
[each_mr.c_obj.get() is not NULL | |
for each_mr in _per_device_mrs.values()] | |
each_mr.c_obj.get() is not NULL | |
for each_mr in _per_device_mrs.values() |
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.
closures inside cpdef functions not yet supported
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.
Invalid types for 'is_not' (Python object, void *)
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.
well then, looks like we stay with the Python list for now 😅
I can't get isort to pass -- running it locally on the failing file doesn't do anything. |
Hmm, historically we've allowed importing |
This PR deprecates RMM. Depends on #449 ✔️ and #457 ✔️. Note this PR will be much simpler once those are merged.
Closes #407
Contributes to #459
pool_memory_resource
cnmem_memory_resource
andcnmem_managed_memory_resource
get_default_resource
andset_default_resource
and updates readme.MD to describe the new per-device default resource APIs.get_default_resource
withget_current_device_resource
get_default_resource
andset_default_resource
a wrapper aroundget/set_current_device_resource
rmm.reinitialize()
to create a resource per specified deviceThis PR does not touch cmake -- that will be left for the release where we remove cnmem.
Need to follow this up with PRs for various RAPIDS repos to not use cnmem resources
in test fixtures and benchmarks.