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 get_upstream_resource method to stream_checking_resource_adaptor #15203

Merged

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Mar 1, 2024

Also remove get_upstream as we want to get away from raw upstreams

Also deprecate `get_upstream` as we want to get away from raw upstreams
@miscco miscco requested a review from a team as a code owner March 1, 2024 13:47
@miscco miscco requested review from davidwendt and nvdbaranec March 1, 2024 13:47
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 1, 2024
@miscco
Copy link
Contributor Author

miscco commented Mar 1, 2024

@harrism can you set the labels etc, I do not have any permissions in cudf

* @brief Return pointer to the upstream resource.
*
* @return Pointer to the upstream resource.
* @briefreturn{rmm::device_async_resource_ref to the upstream resource}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can yeah, I would note that this is what we settled on in rmm

Copy link
Member

Choose a reason for hiding this comment

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

This macro was created by @vyasr to clean up repetitive documentation in exactly this situation of documenting getters. Why did you want it changed back, @davidwendt ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not a standard doxygen tag. This macro is implemented in rmm but not cudf.
This header is not included in the published docs (and not actually processed by doxygen) so I'd rather it be more decipherable to humans in the source text here.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I thought the macro was centralized for RAPIDS. I'm guessing @vyasr will add it once cuDF moves to unified C++/Python docs.

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 1, 2024
}

/**
* @briefreturn{Upstream* to the upstream memory resource}
Copy link
Contributor

Choose a reason for hiding this comment

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

This one too.

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.

Maybe just delete get_upstream now if it's not used anywhere. There is no downstream usage of this.

@@ -73,11 +73,7 @@ class stream_checking_resource_adaptor final : public rmm::mr::device_memory_res
*
* @return Pointer to the upstream resource.
*/
[[deprecated("Use get_upstream_resource instead")]] [[nodiscard]] Upstream* get_upstream()
Copy link
Member

Choose a reason for hiding this comment

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

In the cudf case this is only used in tests. As long as all the tests are updated to not call this method it's perfectly fine to deprecate, and probably even to remove it without deprecation (since it's internal to libcudf unit tests).

@miscco
Copy link
Contributor Author

miscco commented Mar 6, 2024

@davidwendt I removed the get_upstream method alltogether.

From my point this is ready to be merged

@davidwendt
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit bb0e4fd into rapidsai:branch-24.04 Mar 6, 2024
73 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 improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants