-
Notifications
You must be signed in to change notification settings - Fork 912
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 libcudf wrappers around current_device_resource functions. #16679
Add libcudf wrappers around current_device_resource functions. #16679
Conversation
@vyasr I could use your help understanding the docs build failure. Thanks! |
You'll need to add a new docs page for memory_resource.hpp to https://github.com/rapidsai/cudf/tree/branch-24.10/docs/cudf/source/libcudf_docs/api_docs, and then add it either to index.rst or to one of the subpages that it lists in its ToC (or their subpages, etc). |
Thanks! Made an attempt. |
This may have been asked already, but why do we have to include |
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.
There are a handful of places where the default stream header is included by ultimately unused. Example:
#include <cudf/utilities/default_stream.hpp> |
Edit: Actually I think this may be the only one.
Yes, I fixed IWYU mistakes when I noticed them. |
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.
Just that one lingering unnecessary header - not a blocker.
@@ -17,8 +17,7 @@ | |||
#pragma once | |||
|
|||
#include <cudf/utilities/export.hpp> | |||
|
|||
#include <rmm/resource_ref.hpp> | |||
#include <cudf/utilities/memory_resource.hpp> |
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.
Looks like this include can be removed.
@@ -19,11 +19,11 @@ | |||
#include <cudf/detail/utilities/vector_factories.hpp> | |||
#include <cudf/io/text/detail/multistate.hpp> | |||
#include <cudf/utilities/export.hpp> | |||
#include <cudf/utilities/memory_resource.hpp> |
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.
Looks like this include can be removed.
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.
Correct, as well as device_memory_resource.hpp, but need to add rmm/resource_ref.hpp
.
/merge |
Description
Merge after rapidsai/rmm#1661
Creates and uses CUDF internal wrappers around RMM
current_device_resource
functions.I've marked this PR as breaking because it breaks the ABI, however the API is compatible.
For reviewers, the most substantial additions are in the new file
<cudf/utilities/memory_resource.hpp>
, and in theDEVELOPER_GUIDE.md
and*.rst
docs. The rest are all replacements of an include and all calls tormm::get_current_device_resource()
withcudf::get_current_device_resource_ref()
.Closes #16676
Checklist