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 libcudf wrappers around current_device_resource functions. #16679

Merged

Conversation

harrism
Copy link
Member

@harrism harrism commented Aug 28, 2024

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 the DEVELOPER_GUIDE.md and *.rst docs. The rest are all replacements of an include and all calls to rmm::get_current_device_resource() with cudf::get_current_device_resource_ref().

Closes #16676

Checklist

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

@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. Java Affects Java cuDF API. labels Aug 28, 2024
@harrism harrism changed the title Fea/cudf_current_resource_wrappers Add libcudf wrappers around current_device_resource functions. Aug 28, 2024
@harrism harrism added feature request New feature or request breaking Breaking change labels Aug 28, 2024
@harrism harrism marked this pull request as ready for review August 28, 2024 21:38
@harrism harrism requested review from a team as code owners August 28, 2024 21:38
@harrism
Copy link
Member Author

harrism commented Aug 28, 2024

@vyasr I could use your help understanding the docs build failure. Thanks!

@vyasr
Copy link
Contributor

vyasr commented Sep 4, 2024

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).

@harrism
Copy link
Member Author

harrism commented Sep 4, 2024

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.

@nvdbaranec
Copy link
Contributor

nvdbaranec commented Sep 5, 2024

This may have been asked already, but why do we have to include <cudf/utilities/default_stream.hpp> as well as <cudf/utilities/memory_resource.hpp> ? Or were the few places where this was done because it wasn't explicitly included for the neighboring cudf::get_default_stream?

Copy link
Contributor

@nvdbaranec nvdbaranec left a 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.

@harrism
Copy link
Member Author

harrism commented Sep 5, 2024

This may have been asked already, but why do we have to include <cudf/utilities/default_stream.hpp> as well as <cudf/utilities/memory_resource.hpp> ? Or were the few places where this was done because it wasn't explicitly included for the neighboring cudf::get_default_stream?

Yes, I fixed IWYU mistakes when I noticed them.

Copy link
Contributor

@nvdbaranec nvdbaranec left a 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>
Copy link
Contributor

@davidwendt davidwendt Sep 9, 2024

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>
Copy link
Contributor

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.

Copy link
Member Author

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.

@harrism
Copy link
Member Author

harrism commented Sep 10, 2024

/merge

@rapids-bot rapids-bot bot merged commit afd3a4b into rapidsai:branch-24.10 Sep 10, 2024
98 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change feature request New feature or request Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API.
Projects
Status: Done
Status: No status
Development

Successfully merging this pull request may close these issues.

[FEA] Add libcudf cudf::get_current_device_resource() wrapper for rmm::get_current_device_resource()
6 participants