[cudf] Adding memory_resources support for column wrappers - #23581
[cudf] Adding memory_resources support for column wrappers#23581nirandaperera wants to merge 20 commits into
memory_resources support for column wrappers#23581Conversation
nirandaperera
added
improvement
7024e91 to
097df15
Compare
097df15 to
380696c
Compare
Adds defaulted stream and memory_resources parameters to cudftestutil column wrappers and helpers so tests can control allocation and stream routing. Includes MR tests for wrappers and timestamp generators. Signed-off-by: niranda perera <niranda.perera@gmail.com>
380696c to
d11a8bb
Compare
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughColumn-wrapper APIs now accept CUDA streams and memory resources across primitive, dictionary, list, struct, timestamp, and comparison paths. Tests validate allocation routing and update empty-wrapper construction. ChangesColumn-wrapper allocation context
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/include/cudf_test/column_wrapper.hpp (1)
1963-1970: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass
streamand the output resource tomake_lists_column.The offsets, the concatenated child data, and the null mask now use the supplied stream and output resource.
make_lists_columnat Line 1969 still uses the default stream and the current device resource.make_lists_columncan allocate when it sanitizes null rows, and it enforces ordering on its own stream. This creates two problems: allocations escape the supplied resource, and work on the caller stream is not ordered against work on the default stream. The same call appears at Line 2004 inbuild_from_non_nestedand at Line 1885 in the private adopting constructor.As per coding guidelines: "Propagate stream and memory-resource parameters through all internal APIs and correctly order operations across streams using events or explicit dependencies."🛠️ Proposed change
// construct the list column - wrapped = make_lists_column( - cols.size(), std::move(offsets), std::move(data), null_count, std::move(null_mask)); + wrapped = make_lists_column(cols.size(), + std::move(offsets), + std::move(data), + null_count, + std::move(null_mask), + stream, + mr.get_output_mr());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/cudf_test/column_wrapper.hpp` around lines 1963 - 1970, Update the list-column construction calls in the current builder, build_from_non_nested, and the private adopting constructor to pass the supplied stream and output memory resource to make_lists_column. Ensure all related allocations and sanitization work use that resource and execute on the caller’s stream, preserving correct ordering.Source: Coding guidelines
🧹 Nitpick comments (1)
cpp/include/cudf_test/timestamp_utilities.cuh (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude the headers that declare the new parameter types.
The signature now uses
rmm::cuda_stream_viewandcudf::memory_resources. Both arrive only throughcudf_test/column_wrapper.hpp. Include them directly.As per coding guidelines: "include headers directly for every used symbol without unused or incorrectly styled includes."♻️ Proposed change
`#include` <cudf_test/column_wrapper.hpp> `#include` <cudf_test/default_stream.hpp> `#include` <cudf/detail/iterator.cuh> `#include` <cudf/utilities/export.hpp> +#include <cudf/utilities/memory_resource.hpp> `#include` <cudf/wrappers/timestamps.hpp> + +#include <rmm/cuda_stream_view.hpp>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/cudf_test/timestamp_utilities.cuh` around lines 8 - 9, Update the includes in timestamp_utilities.cuh to directly include the headers declaring rmm::cuda_stream_view and cudf::memory_resources, rather than relying on cudf_test/column_wrapper.hpp transitively. Preserve the existing includes only if their symbols remain used.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/cudf_test/column_wrapper.hpp`:
- Around line 823-837: Remove the stale preceding Doxygen block above the
constructor `strings_column_wrapper` so only the new documentation block remains
attached to it. Apply the same cleanup to the duplicated documentation blocks
near the other `strings_column_wrapper` constructors identified in the comment,
without changing constructor behavior.
- Around line 1846-1861: Update make_one_empty_row_column so both constructor
calls pass arguments in stream-then-memory-resource order: construct values with
stream and mr, and construct lists_column_wrapper with stream and mr after the
existing data arguments. Preserve the current validity and null-mask behavior.
- Around line 2163-2181: Remove the no-validity structs_column_wrapper
constructors, including the vector and initializer-list overloads, and retain
the overloads that require an explicit validity argument so calls such as
structs_column_wrapper{{child_a, child_b}} are no longer ambiguous.
In `@cpp/tests/utilities_tests/column_wrapper_tests.cpp`:
- Around line 133-171: Update StringsColumnWrapperMemoryResourceTest inputs to
include non-ASCII UTF-8 strings in the relevant strings_column_wrapper overload
cases, while retaining the existing empty values and null validity coverage.
Ensure the added values are used consistently where input vectors, initializer
lists, and pair-based strings are exercised so character-buffer and offset
allocations are tested with multi-byte text.
---
Outside diff comments:
In `@cpp/include/cudf_test/column_wrapper.hpp`:
- Around line 1963-1970: Update the list-column construction calls in the
current builder, build_from_non_nested, and the private adopting constructor to
pass the supplied stream and output memory resource to make_lists_column. Ensure
all related allocations and sanitization work use that resource and execute on
the caller’s stream, preserving correct ordering.
---
Nitpick comments:
In `@cpp/include/cudf_test/timestamp_utilities.cuh`:
- Around line 8-9: Update the includes in timestamp_utilities.cuh to directly
include the headers declaring rmm::cuda_stream_view and cudf::memory_resources,
rather than relying on cudf_test/column_wrapper.hpp transitively. Preserve the
existing includes only if their symbols remain used.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9f5adc43-3fc8-43e3-8862-a51b77cbe169
📒 Files selected for processing (6)
cpp/include/cudf_test/column_wrapper.hppcpp/include/cudf_test/memory_resource_utilities.hppcpp/include/cudf_test/timestamp_utilities.cuhcpp/tests/utilities/column_utilities.cucpp/tests/utilities_tests/column_wrapper_tests.cppcpp/tests/wrappers/timestamps_test.cu
Signed-off-by: niranda perera <niranda.perera@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tests/utilities_tests/column_wrapper_tests.cpp`:
- Around line 416-435: In cpp/tests/utilities_tests/column_wrapper_tests.cpp
lines 416-435 and 520-539, update both comparison tests around match_col, col,
and CUDF_TEST_EXPECT_COLUMNS_EQUAL to assert the combined output allocation size
while both released columns remain alive, using the fixture’s
validate_with_harness() contract. Then explicitly destroy match_col and col so
TearDown() can verify all tracked allocations are released.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 16135ac2-adf3-40cd-ae52-5f8cfa27aec9
📒 Files selected for processing (1)
cpp/tests/utilities_tests/column_wrapper_tests.cpp
Signed-off-by: niranda perera <niranda.perera@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tests/utilities_tests/column_utilities_tests.cpp`:
- Around line 80-86: Update all three cudf::split calls in
cpp/tests/utilities_tests/column_utilities_tests.cpp at lines 80-86, 105-111,
and 307-312 to pass stream as the third argument, while preserving the existing
col and splits arguments.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 378c8a02-4ee3-40d2-8393-7375b50d2186
📒 Files selected for processing (1)
cpp/tests/utilities_tests/column_utilities_tests.cpp
| rmm::device_buffer make_elements( | ||
| InputIterator begin, | ||
| InputIterator end, | ||
| rmm::cuda_stream_view stream = cudf::test::get_default_stream(), |
There was a problem hiding this comment.
I think we typically say that detail functions do not get defaults for stream or mr, to ensure that callers pass the parameters and don’t get the wrong stream/mr by default.
There was a problem hiding this comment.
@bdice I think we can easily do this to make_elements util. But changing this in make_null_mask to accept explicit stream/mr is going to touch many places 😞
https://github.com/rapidsai/cudf/blob/7ec9b08f8791c18a01cd56453575cd1c302f59a2/cpp/include/cudf_test/column_wrapper.hpp#L276
There was a problem hiding this comment.
@nirandaperera and I discussed and that work will go into a follow-up PR.
There was a problem hiding this comment.
#23639 This PR will remove the defaults from the make_null_mask declaration.
Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
| rmm::device_buffer make_elements( | ||
| InputIterator begin, | ||
| InputIterator end, | ||
| rmm::cuda_stream_view stream = cudf::test::get_default_stream(), |
There was a problem hiding this comment.
@nirandaperera and I discussed and that work will go into a follow-up PR.
| : strings_column_wrapper(std::cbegin(strings), std::cend(strings), v) | ||
| template < | ||
| typename ValidityIterator, | ||
| std::enable_if_t<!std::is_convertible_v<ValidityIterator&, cudf::memory_resources>>* = nullptr> |
There was a problem hiding this comment.
Is there some ambiguity in the constructor here? I wonder what else we could do -- do we really need all of these constructors? If we remove parameter defaults, can we reduce ambiguity?
Also let's use requires instead of SFINAE if we can.
There was a problem hiding this comment.
Update: we're going to try cuda::stream_ref instead of rmm::cuda_stream_view. The types are implicitly convertible but cuda::stream_ref isn't default-constructible (or at least that's deprecated).
| template < | ||
| typename InputIterator, | ||
| typename ValidityIterator, | ||
| std::enable_if_t<!std::is_convertible_v<ValidityIterator&, cudf::memory_resources>>* = nullptr> |
There was a problem hiding this comment.
Same question here and elsewhere.
| scoped_current_device_resource(scoped_current_device_resource&&) noexcept; | ||
| scoped_current_device_resource& operator=(scoped_current_device_resource&&) noexcept; |
There was a problem hiding this comment.
Why did we need to make this movable?
There was a problem hiding this comment.
I wanted to be able to move an obj to std::optional. Either way, I think moveable is a correct semantic for scoped_current_device_resource, is it not?
| // TODO: Pass `mr` once two_table_comparator / equality preprocessed_table::create accept | ||
| // memory_resources instead of allocating from the current device resource. |
There was a problem hiding this comment.
It doesn't seem like this is included in #23027. Is this a requirement to get to the cudf::label_bins pilot, or is this additional scope? Either way we should make sure it's tracked in the issue.
There was a problem hiding this comment.
We need the comparators and preprocessed table changes, to fully make the column utils like
CUDF_TEST_EXPECT_COLUMNS_EQUAL free from current resource.
Signed-off-by: niranda perera <niranda.perera@gmail.com>
|
@bdice I simplified this a lot. Waiting for the GH forks to be unblocked |
Signed-off-by: niranda perera <niranda.perera@gmail.com>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
…df into test-column-wrappers-mr
| return rmm::device_buffer{ | ||
| elements.data(), size * sizeof(ElementTo), cudf::test::get_default_stream()}; | ||
| rmm::device_buffer buffer{elements.data(), size * sizeof(ElementTo), stream, mr.get_output_mr()}; | ||
| stream.synchronize(); // wait for async H2D before host source is destroyed |
There was a problem hiding this comment.
I have the impression this stream sync might be too conservative.
Unless we are reading from the device to the host, I don't think we need to insert a stream sync
| return rmm::device_buffer{ | ||
| elements.data(), size * sizeof(RepType), cudf::test::get_default_stream()}; | ||
| rmm::device_buffer buffer{elements.data(), size * sizeof(RepType), stream, mr.get_output_mr()}; | ||
| stream.synchronize(); // wait for async H2D before host source is destroyed |
| cudf::bitmask_allocation_size_bytes(cudf::distance(begin, end)), | ||
| stream, | ||
| mr.get_output_mr()}; | ||
| stream.synchronize(); // wait for async H2D before host source is destroyed |
| 0}); | ||
| rmm::device_buffer data{elements.data(), size * sizeof(Rep), stream, mr.get_output_mr()}; | ||
| wrapped.reset(new cudf::column{data_type, size, std::move(data), rmm::device_buffer{}, 0}); | ||
| stream.synchronize(); // wait for async H2D before host source is destroyed |
4 participants
Description
Depends on #23578
A part of #20780.
Port column wrappers to accept memory_resources and stream
Adds defaulted stream and memory_resources parameters to cudftestutil
column wrappers and helpers so tests can control allocation and stream
routing. Includes MR tests for wrappers and timestamp generators.
Checklist