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

Add get_upstream_resource to resource adaptors #1456

Merged
merged 8 commits into from
Feb 29, 2024
Merged

Add get_upstream_resource to resource adaptors #1456

merged 8 commits into from
Feb 29, 2024

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Feb 2, 2024

We want to get rid of the raw get_upstream method. In order to achieve that we need to add an alternative that is based on our new rmm::device_async_resource_ref

I am a stickler to consistency so I also cleaned the documentation of get_upstream up and added [[nodiscard]]

closes #1455

@miscco
Add get_upstream_resource to resource adaptors
dbfadc5 
We want to get rid of the raw `get_upstream` method. In order to achieve that we need to add an alternative that is based on our new `rmm::device_async_resource_ref`

I am a stickler to consistency so I also cleaned the documentation of `get_upstream` up and added `[[nodiscard]]`

closes rapidsai#1455
@miscco miscco requested a review from a team as a code owner February 2, 2024 09:22
@miscco miscco requested review from harrism and cwharris February 2, 2024 09:22
@github-actions github-actions bot added the cpp Pertains to C++ code label Feb 2, 2024
@miscco miscco added feature request New feature or request non-breaking Non-breaking change and removed cpp Pertains to C++ code labels Feb 2, 2024
harrism
harrism approved these changes Feb 6, 2024
View reviewed changes
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 good, thank you. Should we standardize the interface of resource adaptors somehow? I've often felt that MRs would be more composable if there were some common interface for accessing upstream MRs.

include/rmm/mr/device/binning_memory_resource.hpp Show resolved Hide resolved
@miscco
Copy link
Contributor Author

miscco commented Feb 6, 2024

Looks good, thank you. Should we standardize the interface of resource adaptors somehow? I've often felt that MRs would be more composable if there were some common interface for accessing upstream MRs.

I believe we are on a good way. When moving everything towards async_device_memory_resource we can completely get rid of Upstream altogether and only over communicate throught the resource_ref

@harrism
Copy link
Member

harrism commented Feb 6, 2024

But adaptors, suballocators, etc. will always have upstream resource refs. Just wondering if we need a legislated way to get the ref, or if we just stick with a de facto interface.

@miscco
Copy link
Contributor Author

miscco commented Feb 9, 2024

But adaptors, suballocators, etc. will always have upstream resource refs. Just wondering if we need a legislated way to get the ref, or if we just stick with a de facto interface.

I mean my thought was that we will just keep going with get_upstream_resource or get_upstream

@github-actions github-actions bot added the cpp Pertains to C++ code label Feb 9, 2024
@jrhemstad
Copy link
Contributor

I mean my thought was that we will just keep going with get_upstream_resource or get_upstream

I believe @harrism is asking about adding a resource_adaptor concept and/or CRTP helper that codifies get_upstream_resource() as the definitive way to access an upstream resource in an adaptor.

@miscco
Copy link
Contributor Author

miscco commented Feb 22, 2024

Coming back to this, I am all for providing a well defined way of extracting the upstream resource

However, we want to make sure that we have the right one. Currently we have get_upstream and this PR introduces the slightly longer get_upstream_resource

If we want to introduce a concept that checks whether an upstream resource is provided we need to decide whether we want get_upstream_resource going forward or whether we want to move back to get_upstream once the old one has been removed

@harrism
Copy link
Member

harrism commented Feb 22, 2024

My 2c: I think get_upstream_resource is very clearly named. So it would be fine to switch to that if we have to change the API from pointers to refs anyway.

miscco and others added 2 commits February 22, 2024 13:01
@miscco @harrism
Update include/rmm/mr/is_resource_adaptor.hpp
7cf473f 
Co-authored-by: Mark Harris <[email protected]>
@miscco
Use get_upstream_resource universally to get the upstream resource_ref
af53698 
I also adopted `pool_memory_resource` and `thrust_allocator_adaptor` to use the new interface.

We only added it this release AFAIK so it should not be used yet and we should settle on a universal design
miscco
miscco commented Feb 22, 2024
View reviewed changes
include/rmm/mr/device/thrust_allocator_adaptor.hpp
@@ -121,9 +121,12 @@ class thrust_allocator : public thrust::device_malloc_allocator<T> {
}

/**
* @briefreturn{The resource used to allocate and deallocate}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has just been added in the recent push so I am pretty sure it is safe to rename the method so that it is consistent with the other adaptors

@harrism harrism requested a review from wence- February 22, 2024 19:51
@harrism harrism removed the request for review from cwharris February 27, 2024 09:17
@miscco
Copy link
Contributor Author

miscco commented Feb 29, 2024

/merge

@rapids-bot rapids-bot bot merged commit f132d4b into rapidsai:branch-24.04 Feb 29, 2024
55 checks passed
@harrism
Copy link
Member

harrism commented Feb 29, 2024

I generally like to get 2 C++ codeowner approvals before merging. Just ping the auto-assigned reviewers if it has been a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harrism harrism harrism approved these changes

@wence- wence- Awaiting requested review from wence-

Assignees
No one assigned
Labels
cpp Pertains to C++ code feature request New feature or request non-breaking Non-breaking change
Projects
RMM Project Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add get_upstream_resource method to resource adaptors to eventually replace raw get_upstream
3 participants
@miscco @harrism @jrhemstad