-
Notifications
You must be signed in to change notification settings - Fork 200
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
Make device_memory_resource::supports_streams() not pure virtual. Remove derived implementations and calls in RMM #1437
Make device_memory_resource::supports_streams() not pure virtual. Remove derived implementations and calls in RMM #1437
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks good to me.
For my own understanding, what does "nonpure" mean in this context (I am assuming it isn't the usual functional mathematical meaning of side-effectful).
@@ -299,7 +299,7 @@ class device_memory_resource { | |||
* | |||
* @returns bool true if the resource supports non-null CUDA streams. | |||
*/ | |||
[[nodiscard]] virtual bool supports_streams() const noexcept = 0; | |||
[[nodiscard]] virtual bool supports_streams() const noexcept { return false; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (non-blocking): This remains virtual so that downstream users don't immediately break? (I must admit that inheritance in C++ is not one of my strong suits)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. And it's being deprecated as soon as the two downstream PRs (already approved) merge. Then removed in the next release. Nobody uses this, and it's not part of cuda::mr::async_memory_resource
"Pure virtual" means that there is no implementation in the base class. Corrected PR title. |
/merge |
Part of rapidsai/rmm#1389. This removes now-optional and soon-to-be deprecated `supports_streams()` from cuDF's custom `device_memory_resource` implementations. Depends on rapidsai/rmm#1437 Authors: - Mark Harris (https://github.com/harrism) - Bradley Dice (https://github.com/bdice) Approvers: - Nghia Truong (https://github.com/ttnghia) - Bradley Dice (https://github.com/bdice) - Yunsong Wang (https://github.com/PointKernel) - Michael Schellenberger Costa (https://github.com/miscco) URL: #14857
Closes #1432. Part of #1389
Checklist