Don't setup managed memory if rmm isn't using default settings#6700
Merged
rapids-bot[bot] merged 3 commits intorapidsai:branch-25.06from May 16, 2025
Merged
Conversation
jcrist
added
non-breaking
jcrist
commented
May 13, 2025
csadorf
approved these changes
May 13, 2025
csadorf
requested changes
May 14, 2025
Contributor
csadorf
left a comment
csadorf
left a comment
There was a problem hiding this comment.
Just explicitly acknowledging the outcome from this thread: we should update our docs about our behavior when we can't enable UVM, otherwise good.
Previously `cuml.accel` would always install a new `ManagedMemoryResource` on for the current device when installed. We now only do this if rmm is using the default resource. If a user or library has already configured rmm, we continue using their settings. This lets `cuml.accel` compose better with `cudf.pandas`, since we won't clobber their memory resource setup after loading ours. Also added tests for the `cuml.accel` IPython magics, including tests loading `cudf.pandas` before and after `cuml.accel`.
jcrist
force-pushed
the
cuml-accel-dont-modify-setup-mr
branch
from
37a97ed to
6981ec5
Compare
jcrist
commented
May 15, 2025
Member
Author
jcrist
left a comment
jcrist
left a comment
There was a problem hiding this comment.
Docs are updated, ready for another review.
Contributor
|
/merge |
betatim
approved these changes
May 16, 2025
Member
|
I restarted the CI jobs. There was no outout in the failed build job, so not sure what went wrong but probably transient. |
viclafargue
approved these changes
May 16, 2025
Contributor
viclafargue
left a comment
viclafargue
left a comment
There was a problem hiding this comment.
Thanks @jcrist, LGTM
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously
cuml.accelwould always install a newManagedMemoryResourceon the current device when installed. We now only do this if rmm is using the default resource. If a user or library has already configured rmm, we continue using their settings.This lets
cuml.accelcompose better withcudf.pandas, since we won't clobber their memory resource setup after loading ours.Also added tests for the
cuml.accelIPython magics, including tests loadingcudf.pandasbefore and aftercuml.accel.