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

Replace all internal usage of get_upstream with get_upstream_resource #1491

Merged
merged 5 commits into from
Mar 14, 2024

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Mar 1, 2024

We want to get away from raw resources, so prepare deprecation of it by replacing all internal usages

This PR relies on preparation in downstream repositories

…rce`

We want to get away from raw resources, so prepare deprecation of it by replacing all internal usages
@miscco miscco requested a review from a team as a code owner March 1, 2024 13:49
@miscco miscco requested review from wence- and vyasr March 1, 2024 13:49
@github-actions github-actions bot added the cpp Pertains to C++ code label Mar 1, 2024
@miscco miscco added feature request New feature or request 3 - Ready for review Ready for review by team breaking Breaking change labels Mar 1, 2024
@miscco miscco requested review from harrism and bdice March 1, 2024 13:50
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.

Requesting changes to bake sure this doesn't merge before the downstream PRs are merged.

@harrism
Copy link
Member

harrism commented Mar 1, 2024

@miscco if you would like to merge this before downstream PRs, you could move the deprecations to a separate PR that won't be merged yet.

@bdice
Copy link
Contributor

bdice commented Mar 1, 2024

I'd favor splitting this PR into the new functions and then the deprecations, it does seem to help with moving the work along.

@miscco
Copy link
Contributor Author

miscco commented Mar 3, 2024

As far as I can see, there are two occurrences of get_upstream outside of rmm

rapidsai/cudf#15203
rapidsai/raft#2207

The only one I am not 100% sure how to get rid of is one other in raft where it explicitly checks whether a resource of of a given type, which is neither possible with the current resource_ref not is it desired

Given that those PRs are very small and limited in scope I believe we can wait a few days getting them in

@harrism
Copy link
Member

harrism commented Mar 4, 2024

Also the RMM Cython needs to be updated, and all uses of its current method.

@harrism
Copy link
Member

harrism commented Mar 6, 2024

@miscco does this PR also result in changing the semantics of MR equality as discussed in #1402 ?

Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Tiny doc nitpick.

include/rmm/mr/device/binning_memory_resource.hpp Outdated Show resolved Hide resolved
@wence-
Copy link
Contributor

wence- commented Mar 8, 2024

Also the RMM Cython needs to be updated, and all uses of its current method.

We decided that this didn't seem to be necessary, since, for now, the Cython get_upstream function doesn't call into C++ (it returns a member variable of the cython class).

@miscco
Copy link
Contributor Author

miscco commented Mar 14, 2024

/merge

@rapids-bot rapids-bot bot merged commit a98931b into rapidsai:branch-24.04 Mar 14, 2024
53 checks passed
@miscco miscco deleted the deprecate_get_upstream branch March 14, 2024 16:13
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 breaking Breaking change cpp Pertains to C++ code feature request New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants