Convert libcudf resource parameters to rmm::device_async_resource_ref #15507

harrism · 2024-04-11T04:27:24Z

Description

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.

bdice · 2024-04-16T12:40:05Z

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

java/src/main/native/src/RmmJni.cpp

java/src/main/native/src/TableJni.cpp

harrism · 2024-04-16T21:20:53Z

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.

bdice · 2024-04-17T00:41:10Z

@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 · 2024-04-17T00:41:41Z

I have added some guidance for reviewers to the description.

bdice

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

bdice · 2024-04-17T00:47:04Z

cpp/include/cudf/detail/search.hpp

 *
 * @param stream CUDA stream used for device memory operations and kernel launches.
 */
 bool contains(column_view const& haystack, scalar const& needle, rmm::cuda_stream_view stream);

 /**
- * @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 )


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)

harrism · 2024-04-17T01:36:45Z

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 · 2024-04-17T02:13:51Z

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 · 2024-04-17T12:20:00Z

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 · 2024-04-17T12:20:08Z

/merge

Convert to device_async_resource_ref

8e152c7

harrism added improvement Improvement / enhancement to an existing function breaking Breaking change labels Apr 11, 2024

github-actions bot added libcudf Affects libcudf (C++/CUDA) code. Java Affects Java cuDF API. labels Apr 11, 2024

harrism added 2 commits April 16, 2024 01:38

Change some JNI refs back to pointers

9ff848f

Merge 24.06

7f49c97

github-actions bot added Python Affects Python cuDF API. CMake CMake build issue conda labels Apr 16, 2024

harrism marked this pull request as ready for review April 16, 2024 10:59

harrism requested review from a team as code owners April 16, 2024 10:59

harrism requested review from bdice, charlesbluca and mythrocks April 16, 2024 10:59

jlowe reviewed Apr 16, 2024

View reviewed changes

java/src/main/native/src/RmmJni.cpp Outdated Show resolved Hide resolved

java/src/main/native/src/TableJni.cpp Outdated Show resolved Hide resolved

harrism added 2 commits April 17, 2024 00:26

Add default alignment to JNI calls to allocate_async/deallocate_async

275a2aa

Merge branch 'branch-24.06' into device_async_resource_ref

f826e45

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

Remove unused header

a1f781b

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

jlowe approved these changes Apr 17, 2024

View reviewed changes

bdice approved these changes Apr 17, 2024

View reviewed changes

Remove extra space in copydoc commands

478651e

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

harrism removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Apr 17, 2024

rapids-bot bot merged commit 9192d25 into rapidsai:branch-24.06 Apr 17, 2024
71 checks passed

This was referenced Apr 17, 2024

[DOC] Update DEVELOPER_GUIDE.md after conversion to rmm::device_async_resource_ref #15561

Closed

Refactor joins for conditional semis and antis #14646

Merged

ttnghia mentioned this pull request Apr 18, 2024

[FEA] Convert rmm resource parameters to rmm::device_async_resource_ref NVIDIA/spark-rapids-jni#1979

Open

harrism mentioned this pull request Jul 2, 2024

[BUG] from_arrow_device and from_arrow_host should take device_async_resource_ref, not device_memory_resource* #16159

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert libcudf resource parameters to rmm::device_async_resource_ref #15507

Convert libcudf resource parameters to rmm::device_async_resource_ref #15507

harrism commented Apr 11, 2024 •

edited

Loading

bdice commented Apr 16, 2024

harrism commented Apr 16, 2024 •

edited

Loading

bdice commented Apr 17, 2024

harrism commented Apr 17, 2024

bdice left a comment •

edited

Loading

bdice Apr 17, 2024

harrism Apr 17, 2024

harrism commented Apr 17, 2024

bdice commented Apr 17, 2024

harrism commented Apr 17, 2024

harrism commented Apr 17, 2024

	* @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)

Convert libcudf resource parameters to rmm::device_async_resource_ref #15507

Convert libcudf resource parameters to rmm::device_async_resource_ref #15507

Conversation

harrism commented Apr 11, 2024 • edited Loading

Description

Checklist

bdice commented Apr 16, 2024

harrism commented Apr 16, 2024 • edited Loading

bdice commented Apr 17, 2024

harrism commented Apr 17, 2024

bdice left a comment • edited Loading

Choose a reason for hiding this comment

bdice Apr 17, 2024

Choose a reason for hiding this comment

harrism Apr 17, 2024

Choose a reason for hiding this comment

harrism commented Apr 17, 2024

bdice commented Apr 17, 2024

harrism commented Apr 17, 2024

harrism commented Apr 17, 2024

harrism commented Apr 11, 2024 •

edited

Loading

harrism commented Apr 16, 2024 •

edited

Loading

bdice left a comment •

edited

Loading