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

[REVIEW] Deprecate CNMeM #466

Merged
merged 30 commits into from
Aug 12, 2020
Merged

Conversation

harrism
Copy link
Member

@harrism harrism commented Aug 6, 2020

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

  • Removes the dependency on cnmem memory resources in RMM Python. Pool mode switched to pool_memory_resource
  • removes cnmem from all tests
  • removes cnmem from all benchmarks
  • adds C++ deprecation keywords to cnmem_memory_resource and cnmem_managed_memory_resource
  • Updates the readme to mention that cnmem resources are deprecated
  • adds C++ deprecation keywords to get_default_resource and set_default_resource and updates readme.MD to describe the new per-device default resource APIs.
  • replaces usage of C++ get_default_resource with get_current_device_resource
  • Makes C++ get_default_resource and set_default_resource a wrapper around get/set_current_device_resource
  • Updates Python rmm.reinitialize() to create a resource per specified device
  • Updates Python API to expose current device and per-device resource APIs rather than default resource APIs
  • Updates readme

This 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.

@harrism harrism added 3 - Ready for review Ready for review by team 5 - Merge After Dependencies Depends on another PR: do not merge out of order labels Aug 6, 2020
@harrism harrism requested review from a team as code owners August 6, 2020 06:07
@harrism harrism self-assigned this Aug 6, 2020
@harrism harrism changed the title Deprecate CNMeM [REVIEW] Deprecate CNMeM Aug 6, 2020
python/rmm/rmm.py Outdated Show resolved Hide resolved
kkraus14
kkraus14 previously approved these changes Aug 7, 2020
@harrism
Copy link
Member Author

harrism commented Aug 7, 2020

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

@kkraus14
Copy link
Contributor

kkraus14 commented Aug 7, 2020

@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 😄

@kkraus14 kkraus14 dismissed their stale review August 7, 2020 00:37

More changes incoming

@harrism
Copy link
Member Author

harrism commented Aug 7, 2020

Changed my mind, should be a follow up PR.

python/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
Comment on lines +427 to +428
[each_mr.c_obj.get() is not NULL
for each_mr in _per_device_mrs.values()]
Copy link
Contributor

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:

Suggested change
[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()

Copy link
Member Author

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

Copy link
Member Author

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 *)

Copy link
Contributor

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 😅

python/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
@harrism
Copy link
Member Author

harrism commented Aug 11, 2020

I can't get isort to pass -- running it locally on the failing file doesn't do anything.

@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge Python Related to RMM Python API and removed 3 - Ready for review Ready for review by team 5 - Merge After Dependencies Depends on another PR: do not merge out of order labels Aug 11, 2020
@kkraus14
Copy link
Contributor

Hmm, historically we've allowed importing rmm / cudf / etc. on machines without a GPU. With the change to RMM initialization we now call a cudaGetDevice that will throw if there's no GPU available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge Python Related to RMM Python API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Deprecate CNMeM
7 participants