[cudf] Adding memory_resources support for test utils - #23578
Conversation
Signed-off-by: niranda perera <niranda.perera@gmail.com>
nirandaperera
added
improvement
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe test utility APIs now accept optional CUDA streams and memory resources. Column and table comparison, host conversion, formatting, nested-column handling, and validation propagate these resources. Tests verify distinct memory-resource usage. ChangesResource-aware test utilities
Estimated code review effort: 4 (Complex) | ~45 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: 1
🧹 Nitpick comments (4)
cpp/tests/utilities/debug_utilities.cu (1)
149-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
streamandmras the last two parameters.
nested_offsets_to_stringplaces the defaulteddelimiteraftermr. The coding guidelines requirestreambeforemras the final two parameters. Movedelimiterbeforestreamand pass it explicitly at the single call site on Line 387.♻️ Proposed signature reorder
template <typename NestedColumnView> std::string nested_offsets_to_string(NestedColumnView const& c, + std::string const& delimiter, rmm::cuda_stream_view stream, - cudf::memory_resources mr, - std::string const& delimiter = ", ") + cudf::memory_resources mr)Then update the call site:
- "Offsets : " + (lcv.size() > 0 ? nested_offsets_to_string(lcv, stream, mr) : "") + "\n" + + "Offsets : " + (lcv.size() > 0 ? nested_offsets_to_string(lcv, ", ", stream, mr) : "") + + "\n" +As per coding guidelines: "Keep stream and MR as the final two parameters, with stream before MR; public APIs provide defaults while detail APIs do not."
🤖 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/tests/utilities/debug_utilities.cu` around lines 149 - 152, Reorder the parameters of nested_offsets_to_string so delimiter precedes stream, with stream and mr remaining the final two parameters in that order. Update its single call site to pass the delimiter explicitly, preserving the existing delimiter behavior.Source: Coding guidelines
cpp/include/cudf_test/column_utilities.hpp (1)
222-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the missing
@param streamdocumentation.Both overloads now take a
streamparameter, but the doc blocks only documentmr. The//!@condDoxygen_Suppressregion hides this from the doxygen build, so it does not fail docs. Add the line for consistency with the genericto_hostat Line 195.📝 Proposed doc fix
* `@param` c the `column_view` to copy from + * `@param` stream CUDA stream used for device memory operations * `@param` mr Memory resources used for temporary device allocationsAlso applies to: 239-247
🤖 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_utilities.hpp` around lines 222 - 230, Add the missing `@param` stream documentation to the fixed-point to_host overload and the corresponding overload near the additional referenced range, matching the wording and placement used by the generic to_host documentation. Keep the existing mr documentation unchanged.cpp/tests/utilities_tests/column_utilities_tests.cpp (1)
149-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the resource assertions to a nested column.
The test covers fixed-width, string, and decimal columns. The nested paths carry most of the new propagation code:
generate_child_row_indices, the recursivecolumn_property_comparator, the recursivecolumn_comparator_impl<list_view>, and the list and struct printers. None of them run in this test, so a missedstreamormrin those paths stays undetected by the harness.Add one
lists_column_wrappercase and onestructs_column_wrappercase to the same assertions.💚 Proposed additional coverage
cudf::test::fixed_point_column_wrapper<int32_t> decimals({123, 456}, numeric::scale_type{-2}); + cudf::test::lists_column_wrapper<int32_t> lists{{1, 2}, {3}, {4, 5, 6}}; + cudf::test::fixed_width_column_wrapper<int32_t> struct_member{1, 2, 3}; + cudf::test::structs_column_wrapper structs{{struct_member}};CUDF_TEST_EXPECT_COLUMNS_EQUAL(lhs, rhs, cudf::test::debug_output_level::QUIET, stream, mr); + CUDF_TEST_EXPECT_COLUMNS_EQUAL(lists, lists, cudf::test::debug_output_level::QUIET, stream, mr); + CUDF_TEST_EXPECT_COLUMNS_EQUAL( + structs, structs, cudf::test::debug_output_level::QUIET, stream, mr);static_cast<void>(cudf::test::to_strings(sliced, stream, mr)); + static_cast<void>(cudf::test::to_strings(lists, stream, mr)); + static_cast<void>(cudf::test::to_strings(structs, stream, 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/tests/utilities_tests/column_utilities_tests.cpp` around lines 149 - 199, Extend DistinctMemoryResources with lists_column_wrapper and structs_column_wrapper fixtures, then pass each through the same relevant property, equality, equivalence, conversion, and printing assertions used for existing columns. Ensure nested list and struct paths exercise stream and memory-resource propagation, and retain the existing resource-usage and no-live-allocation checks.cpp/tests/utilities/column_utilities.cu (1)
835-948: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRemove defaults from the detail debug helpers.
cudf::test::detail::to_stringandto_stringsstill default to the default stream and current memory resource. Keep defaults only on public overloads, so omitted arguments cannot bypass the caller’s stream and memory resource.🤖 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/tests/utilities/column_utilities.cu` around lines 835 - 948, Remove the default stream and memory-resource arguments from the detail helpers cudf::test::detail::to_string and to_strings. Require callers of these detail functions to pass both explicitly, while preserving defaults only on the corresponding public overloads so omitted arguments cannot bypass the caller’s resources.
🤖 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 339-340: Update all make_null_mask calls in the column utilities
tests to pass only the validity range arguments, removing
cudf::get_current_device_resource_ref() and any other third argument while
preserving the existing begin/end iterators.
---
Nitpick comments:
In `@cpp/include/cudf_test/column_utilities.hpp`:
- Around line 222-230: Add the missing `@param` stream documentation to the
fixed-point to_host overload and the corresponding overload near the additional
referenced range, matching the wording and placement used by the generic to_host
documentation. Keep the existing mr documentation unchanged.
In `@cpp/tests/utilities_tests/column_utilities_tests.cpp`:
- Around line 149-199: Extend DistinctMemoryResources with lists_column_wrapper
and structs_column_wrapper fixtures, then pass each through the same relevant
property, equality, equivalence, conversion, and printing assertions used for
existing columns. Ensure nested list and struct paths exercise stream and
memory-resource propagation, and retain the existing resource-usage and
no-live-allocation checks.
In `@cpp/tests/utilities/column_utilities.cu`:
- Around line 835-948: Remove the default stream and memory-resource arguments
from the detail helpers cudf::test::detail::to_string and to_strings. Require
callers of these detail functions to pass both explicitly, while preserving
defaults only on the corresponding public overloads so omitted arguments cannot
bypass the caller’s resources.
In `@cpp/tests/utilities/debug_utilities.cu`:
- Around line 149-152: Reorder the parameters of nested_offsets_to_string so
delimiter precedes stream, with stream and mr remaining the final two parameters
in that order. Update its single call site to pass the delimiter explicitly,
preserving the existing delimiter behavior.
🪄 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: 2ebfc3c2-4d89-44b1-b1b9-939cd9f9cced
📒 Files selected for processing (7)
cpp/include/cudf_test/column_utilities.hppcpp/include/cudf_test/debug_utilities.hppcpp/include/cudf_test/table_utilities.hppcpp/tests/utilities/column_utilities.cucpp/tests/utilities/debug_utilities.cucpp/tests/utilities/table_utilities.cucpp/tests/utilities_tests/column_utilities_tests.cpp
The validation-utils port incorrectly passed a memory resource to make_null_mask, which still only accepts begin/end on this branch.
|
/ok to test d32da77 |
|
/ok to test 4f0d6db |
Signed-off-by: niranda perera <niranda.perera@gmail.com>
|
/ok to test cfe3cf5 |
| requires(is_numeric<Element>()) | ||
| { | ||
| auto h_data = cudf::test::to_host<Element>(col); | ||
| auto h_data = cudf::test::to_host<Element>(col, stream, mr); |
There was a problem hiding this comment.
A general design question: should we remove the default arguments for stream and mr to help prevent misuse? One common way to end up with the wrong stream or mr is that the code builds and runs fine with the defaults, so users may not stop to consider whether the appropriate values are being passed. Requiring them explicitly would force users to make that decision at each call site and could significantly reduce accidental misuse.
There was a problem hiding this comment.
@PointKernel I thought about this too. But this will be a major breaking change. Maybe once we merge this, I can do this as a follow up.
There was a problem hiding this comment.
Makes sense to me. This isn’t a change request for the current effort. I’m just putting the idea out there as something that may be worth considering and getting feedback on from others.
There was a problem hiding this comment.
Actually, for this PR, we could simply add new overloads that take stream and mr, and deprecate the existing APIs. Not a hard request though.
There was a problem hiding this comment.
I think cudf_test isnt really public, isnt it? So, this will all be impl detail anyways. Following would be the scope of changes. I will follow it up once this PR goes in.
| Macro | Call sites | Files |
|---|---|---|
| CUDF_TEST_EXPECT_COLUMNS_EQUAL | 3317 | 208 |
| CUDF_TEST_EXPECT_COLUMNS_EQUIVALENT | 1132 | 100 |
| CUDF_TEST_EXPECT_TABLES_EQUAL | 934 | 64 |
| CUDF_TEST_EXPECT_TABLES_EQUIVALENT | 239 | 40 |
| CUDF_TEST_EXPECT_EQUAL_BUFFERS | 25 | 6 |
| CUDF_TEST_EXPECT_COLUMN_PROPERTIES_EQUAL | 9 | 3 |
| CUDF_TEST_EXPECT_COLUMN_PROPERTIES_EQUIVALENT | 5 | 2 |
| Total | ~5661 | ~284 |
| requires(is_numeric<Element>()) | ||
| { | ||
| auto h_data = cudf::test::to_host<Element>(col); | ||
| auto h_data = cudf::test::to_host<Element>(col, stream, mr); |
There was a problem hiding this comment.
Makes sense to me. This isn’t a change request for the current effort. I’m just putting the idea out there as something that may be worth considering and getting feedback on from others.
|
/merge |
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. Authors: - Niranda Perera (https://github.com/nirandaperera) - Bradley Dice (https://github.com/bdice) Approvers: - Basit Ayantunde (https://github.com/lamarrr) - Bradley Dice (https://github.com/bdice) URL: #23581
3 participants
Description
A part of #20780.
Adds explicit output/temporary memory-resource routing and a CUDA stream parameter to the
cudftestutilvalidation and debug helpers, so tests can verify (or override) where these utilities allocate. This is part of the ongoingcudf::memory_resourcesmigration (issue #20780) and follows the pattern established by PR #23028 (which addedmemory_resourcesto the library-side APIs and thememory_resource_test_harness).Changes
Public test-utility APIs — new trailing parameters (defaulted for full source-compat):
expect_column_properties_equal/expect_column_properties_equivalentexpect_columns_equal/expect_columns_equivalentexpect_equal_buffersexpect_tables_equal/expect_tables_equivalentbitmask_to_host,to_host<T>(numeric, fixed-point, andstd::stringspecialization)to_string,to_strings,printEach gains:
cpp rmm::cuda_stream_view stream = cudf::test::get_default_stream(), cudf::memory_resources mr = cudf::get_current_device_resource_ref()Notes
fixed_width_column_wrapperetc.) migration is intentionally deferred to a follow-up.Checklist