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

Add python wrapper for system memory resource #1605

Merged
merged 16 commits into from
Jul 25, 2024

Conversation

rongou
Copy link
Contributor

@rongou rongou commented Jul 10, 2024

Description

Follow up on #1581 to add access to the system memory resource in python.

Fixes #1622

Checklist

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

@rongou rongou requested a review from a team as a code owner July 10, 2024 21:09
@rongou rongou self-assigned this Jul 10, 2024
@github-actions github-actions bot added the Python Related to RMM Python API label Jul 10, 2024
@rongou rongou added feature request New feature or request 3 - Ready for review Ready for review by team non-breaking Non-breaking change labels Jul 10, 2024
@rongou rongou requested review from harrism, wence- and vyasr July 11, 2024 23:34
@rongou rongou requested a review from jakirkham July 18, 2024 23:35
Copy link

copy-pr-bot bot commented Jul 19, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@rongou rongou force-pushed the sam-cupy-allocator branch from 2f0ae85 to 352ab6d Compare July 19, 2024 17:44
@rongou
Copy link
Contributor Author

rongou commented Jul 19, 2024

@wence- @vyasr can you take a look? Thanks!

@rongou
Copy link
Contributor Author

rongou commented Jul 22, 2024

ping @harrism @wence- @vyasr

@vyasr
Copy link
Contributor

vyasr commented Jul 22, 2024

Apologies, Lawrence and are swamped with multiple large P0 efforts going into this release. I'll try to get to this today.

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.

Looks fine, but I question whether we want to add more options to initialize.

python/rmm/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
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.

Thanks for compromising, @rongou ! As part of our upcoming Python refactoring, we should consider how to make it easier to handle setting MRs more generically.

@rongou rongou requested a review from bdice July 24, 2024 18:34
pass


cdef class SamHeadroomResourceAdaptor(DeviceMemoryResource):
Copy link
Contributor

@bdice bdice Jul 24, 2024

Choose a reason for hiding this comment

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

An adaptor accepts another resource as an argument (e.g. a logging adaptor adds logging, but passes through to the underlying MR). This is not an adaptor, it is implemented like an entirely new resource.

Adaptors must be composable. It cannot be specific to the System MR and must accept other MRs if it is to be an adaptor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we redesign this so that the SystemMemoryResource has an argument headroom and we don't need the "adaptor" at all? Or rename this so it's not an adaptor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The C++ side has both the system mr and the adaptor, but I guess we don't have to strictly mirror it in python. I'd be happy to have a single SystemMemoryResource with an argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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.

I think there's room for future improvement here, but we definitely can't do that in this PR so I don't want to hold things up for this release.

python/rmm/rmm/_lib/memory_resource.pxd Outdated Show resolved Hide resolved
python/rmm/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved
@rongou rongou requested a review from a team as a code owner July 24, 2024 22:02
@rongou rongou requested a review from miscco July 24, 2024 22:02
@github-actions github-actions bot added the cpp Pertains to C++ code label Jul 24, 2024
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.

Approving again post the final split of classes on the Python side.

@harrism
Copy link
Member

harrism commented Jul 25, 2024

/merge

@rapids-bot rapids-bot bot merged commit 67a78d6 into rapidsai:branch-24.08 Jul 25, 2024
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for review Ready for review by team cpp Pertains to C++ code feature request New feature or request non-breaking Non-breaking change Python Related to RMM Python API
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FEA] Python bindings for system_memory_resource and sam_headroom_memory_resource
4 participants