Skip to content

Convert libcudf resource parameters to rmm::device_async_resource_ref - #15507

Merged
rapids-bot[bot] merged 7 commits into
NVIDIA:branch-24.06from
harrism:device_async_resource_ref
Apr 17, 2024
Merged

Convert libcudf resource parameters to rmm::device_async_resource_ref#15507
rapids-bot[bot] merged 7 commits into
NVIDIA:branch-24.06from
harrism:device_async_resource_ref

Conversation

@harrism

@harrism harrism commented Apr 11, 2024

Copy link
Copy Markdown
Contributor

Description

Closes #15498

For reviewers:
Almost all of the thousands of changes are simple textual replace of rmm::mr::device_memory_resource * with rmm::device_async_resource_ref.

I think the only substantial changes that are different are in contiguous_split.cu (which was assigning nullptr to the MR pointer -- I have changed these cases to use a std::optional<rmm::device_async_resource_ref>), and in JNI code.

I still need to figure out how to build and test the JNI bindings. And figure out necessary Cython changes.

JNI is passing CI now. Cython required no changes.

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 added improvement Improvement / enhancement to an existing function breaking Breaking change labels Apr 11, 2024
@github-actions github-actions Bot added libcudf Affects libcudf (C++/CUDA) code. Java Affects Java cuDF API. labels Apr 11, 2024
@github-actions github-actions Bot added Python Affects Python cuDF API. CMake CMake build issue conda labels Apr 16, 2024
@harrism
harrism marked this pull request as ready for review April 16, 2024 10:59
@harrism
harrism requested review from a team as code owners April 16, 2024 10:59
@bdice

bdice commented Apr 16, 2024

Copy link
Copy Markdown
Contributor

It looks like there are some unrelated CI changes in the diff. Could you take a peek?

Comment thread java/src/main/native/src/RmmJni.cpp Outdated
Comment thread java/src/main/native/src/TableJni.cpp Outdated
@harrism

harrism commented Apr 16, 2024

Copy link
Copy Markdown
Contributor Author

It looks like there are some unrelated CI changes in the diff. Could you take a peek?

Definitely are. I have no idea how they got them. Any idea how to remove them, or do I have to start over for the third time?

Note that a local git diff device_async_resource_ref branch-24.06 --stat lists only C++ source and header files, no CI files.

Fixed with a merge.

@github-actions github-actions Bot removed Python Affects Python cuDF API. CMake CMake build issue conda labels Apr 17, 2024
@bdice

bdice commented Apr 17, 2024

Copy link
Copy Markdown
Contributor

@harrism Thanks. I think the problem with the diff was that commit 7f49c97 wasn't a normal merge commit that included the history (which is then excluded from the diff, because the branches have common history).

@harrism

harrism commented Apr 17, 2024

Copy link
Copy Markdown
Contributor Author

I have added some guidance for reviewers to the description.

@harrism
harrism requested review from jlowe and removed request for a team April 17, 2024 00:42

@bdice bdice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I read the first ~200 files and spot-checked the rest. I have one comment on spacing in docstrings. Otherwise, this looks good to me.

(I also looked at contiguous_split.cu specifically as requested.)

Comment thread cpp/include/cudf/detail/search.hpp Outdated

/**
* @copydoc cudf::contains(column_view const&, column_view const&, rmm::mr::device_memory_resource*)
* @copydoc cudf::contains(column_view const&, column_view const&, rmm::device_async_resource_ref )

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A lot of these docstrings have an extra space at the end after rmm::device_async_resource_ref. Can we fix that? e.g.

Suggested change
* @copydoc cudf::contains(column_view const&, column_view const&, rmm::device_async_resource_ref )
* @copydoc cudf::contains(column_view const&, column_view const&, rmm::device_async_resource_ref)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@harrism harrism added the DO NOT MERGE Hold off on merging; see PR for details label Apr 17, 2024
@harrism

harrism commented Apr 17, 2024

Copy link
Copy Markdown
Contributor Author

Thank you for the reviews. @bdice thoughts on whether we should proceed with merging this out of sync with other repos? I kind of want to wait until I have a bit more confident progress on other repos, especially RAFT, which is proving kind of tricky.

@bdice

bdice commented Apr 17, 2024

Copy link
Copy Markdown
Contributor

Thank you for the reviews. @bdice thoughts on whether we should proceed with merging this out of sync with other repos? I kind of want to wait until I have a bit more confident progress on other repos, especially RAFT, which is proving kind of tricky.

I'm somewhat inclined to merge sooner, given the scope of this change and its potential for conflicts. It would also be nice to have a large RAPIDS repository using the new code paths, to ensure that it's working as intended. However, if you want to wait, I'm okay with that.

@harrism

harrism commented Apr 17, 2024

Copy link
Copy Markdown
Contributor Author

I got the RAFT changes working. Whether they like them is a different matter. But I agree, this PR looks pretty clean and safe, it is reviewed, and at risk of a lot of conflicts, so let's go ahead and merge

@harrism

harrism commented Apr 17, 2024

Copy link
Copy Markdown
Contributor Author

/merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaking change improvement Improvement / enhancement to an existing function Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace device_memory_resource* with device_async_resource_ref

3 participants