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

Make device_memory_resource::do_get_mem_info() and supports_get_mem_info() not pure virtual. Remove derived implementations and calls in RMM #1430

Merged
merged 4 commits into from
Jan 23, 2024
Merged

Make device_memory_resource::do_get_mem_info() and supports_get_mem_info() not pure virtual. Remove derived implementations and calls in RMM #1430

merged 4 commits into from
Jan 23, 2024

Conversation

harrism
Copy link
Member

@harrism harrism commented Jan 18, 2024 •
edited
Loading

Description

Closes #1426

As part of #1388, this PR contributes to deprecating and removing all get_mem_info functionality from memory resources. This first PR makes these methods optional without deprecating them.

  • Makes rmm::mr::device_memory_resource::supports_get_mem_info() nonvirtual (and always return false)
  • Makes rmm::mr::device_memory_resource::do_get_mem_info() nonvirtual (and always return {0, 0}).
  • Removes all derived implementations of the above.
  • Removes all calls to the above.

Checklist

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

@harrism harrism requested a review from a team as a code owner January 18, 2024 04:35
@harrism harrism requested review from vyasr and bdice January 18, 2024 04:35
@github-actions github-actions bot added the cpp Pertains to C++ code label Jan 18, 2024
@harrism harrism self-assigned this Jan 18, 2024
@harrism harrism added feature request New feature or request breaking Breaking change labels Jan 18, 2024
bdice
bdice approved these changes Jan 19, 2024
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

One comment: I'd prefer to remove the commented deprecations and add them later when you're ready to actually deprecate the methods. I'm approving now because I think the current state is okay to merge if you disagree with my preference.

include/rmm/mr/device/device_memory_resource.hpp Outdated
@@ -306,7 +306,8 @@ class device_memory_resource {
*
* @return bool true if the resource supports get_mem_info, false otherwise.
*/
[[nodiscard]] virtual bool supports_get_mem_info() const noexcept = 0;
//[[deprecated("Use rmm::available_device_memory())")]] //
Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify my understanding, a later PR will uncomment this and deprecate this method?

If so, can we add the deprecation notice in that second PR rather than leaving a commented deprecation notice in this PR? If I came across this code I would think it was previously deprecated and is now un-deprecated by the commenting.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bdice yes, see the plan in #1388 (checklist item 3 is what you're looking for)

I agree with him that we should do this later

Copy link
Contributor

Choose a reason for hiding this comment

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

Related, but possible a comment more relevant to a subsequent PR if we do remove the deprecation notice in this PR: how should we document the difference between querying for the total device memory available and the amount provided by the memory resource? The difficulty in accurately providing the latter is one reason that this API is being removed, but prior users may have expected it to be working. Should we include that explanation somewhere? I think #1388 explains this fairly well, maybe adding it to a PR description (perhaps in the PR actually adding the deprecation) would be sufficient so that the explanation is immortalized in the changelog (i.e. in the git repo rather than only on GH).

Copy link
Member Author

Choose a reason for hiding this comment

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

We document it, yes. FWIW, at least within RAPIDS, there are no uses of get_mem_info.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed comments. Those were from my initial testing of deprecation. That was intended to surface needed changes in other RAPIDS libraries. "Allowed" deprecation warnings in the other libraries makes this difficult, and a textual search turns out to be easier.

vyasr
vyasr approved these changes Jan 22, 2024
View reviewed changes
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, with a pair of non-blocking comments. Feel free to address how you best see fit based on Bradley/my input.

include/rmm/mr/device/device_memory_resource.hpp Outdated
@@ -306,7 +306,8 @@ class device_memory_resource {
*
* @return bool true if the resource supports get_mem_info, false otherwise.
*/
[[nodiscard]] virtual bool supports_get_mem_info() const noexcept = 0;
//[[deprecated("Use rmm::available_device_memory())")]] //
Copy link
Contributor

Choose a reason for hiding this comment

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

@bdice yes, see the plan in #1388 (checklist item 3 is what you're looking for)

I agree with him that we should do this later

include/rmm/mr/device/device_memory_resource.hpp Outdated
@@ -306,7 +306,8 @@ class device_memory_resource {
*
* @return bool true if the resource supports get_mem_info, false otherwise.
*/
[[nodiscard]] virtual bool supports_get_mem_info() const noexcept = 0;
//[[deprecated("Use rmm::available_device_memory())")]] //
Copy link
Contributor

Choose a reason for hiding this comment

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

Related, but possible a comment more relevant to a subsequent PR if we do remove the deprecation notice in this PR: how should we document the difference between querying for the total device memory available and the amount provided by the memory resource? The difficulty in accurately providing the latter is one reason that this API is being removed, but prior users may have expected it to be working. Should we include that explanation somewhere? I think #1388 explains this fairly well, maybe adding it to a PR description (perhaps in the PR actually adding the deprecation) would be sufficient so that the explanation is immortalized in the changelog (i.e. in the git repo rather than only on GH).

@harrism
Copy link
Member Author

harrism commented Jan 22, 2024

/merge

@rapids-bot rapids-bot bot merged commit 6c904f7 into rapidsai:branch-24.02 Jan 23, 2024
48 checks passed
@harrism harrism mentioned this pull request Jan 24, 2024
Merged
3 tasks
@harrism harrism changed the title Make device_memory_resource::do_get_mem_info() and supports_get_mem_info() nonvirtual. Remove derived implementations and calls in RMM Make device_memory_resource::do_get_mem_info() and supports_get_mem_info() not pure virtual. Remove derived implementations and calls in RMM Jan 25, 2024
rapids-bot bot pushed a commit that referenced this pull request Jan 26, 2024
@harrism
Deprecate rmm::mr::device_memory_resource::get_mem_info() and support…
e374a27 
…s_get_mem_info(). (#1436)

Closes #1427 . Part of #1388.

#1430 made these functions non-virtual and removed them from all MRs and tests. This PR completes the next step of deprecating them.  

Merge after 
 - rapidsai/raft#2108
 - rapidsai/cudf#14832

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Lawrence Mitchell (https://github.com/wence-)
  - Michael Schellenberger Costa (https://github.com/miscco)
  - Bradley Dice (https://github.com/bdice)

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

@bdice bdice bdice approved these changes

@vyasr vyasr vyasr approved these changes

Assignees

@harrism harrism

Labels
breaking Breaking change cpp Pertains to C++ code feature request New feature or request
Projects
RMM Project Board
Status: Done
Milestone
No milestone
3 participants
@harrism @vyasr @bdice