Cudf-free shuffler tests - #1084
Conversation
|
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. |
|
/ok to test |
0ce351e to
fb06ec7
Compare
fb06ec7 to
8820818
Compare
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>
| A ``PackedData`` containing the integer sequence. | ||
| """ | ||
| data = np.arange(offset, offset + n_elements, dtype=_DTYPE).tobytes() | ||
| gpu_data = rmm.DeviceBuffer.to_device(data, stream=stream) |
There was a problem hiding this comment.
Please allocate this test payload through br.device_mr (or a small BR-backed helper) before wrapping it in PackedData. to_device() uses the current/default RMM resource and from_device_buffer() only adopts the allocation, so the replacement tests bypass BufferResource tracking that the old partition_and_pack(..., br=br) path covered.
Signed-off-by: niranda perera <niranda.perera@gmail.com>
| /// Conservation-preserving data model shared by the shuffler round-trip tests. | ||
| /// | ||
| /// The index range `[0, total_num_rows)` is split into `total_num_partitions^2` | ||
| /// contiguous sub-regions via `chunk_indices` (front-loaded, so trailing sub-regions | ||
| /// are empty when `N < P*P`). Sub-region `(local_pidx, split_idx)` is piece | ||
| /// `k = local_pidx * P + split_idx` and is routed to destination partition | ||
| /// `split_idx`. The pieces exactly tile `[0, N)`, so the total shuffled row | ||
| /// count equals `N` regardless of rank or partition counts (conservation). A | ||
| /// per-shuffle `base` offset is added to every value so distinct shuffles carry | ||
| /// distinct data. |
There was a problem hiding this comment.
Is this supposed to be part of make_partition_data docs? Currently looks like a strange placing not really connected to anything.
There was a problem hiding this comment.
I was trying to explain test approach here, especially the P^2 splits because I felt that a new reader might find it a little confusing.
There was a problem hiding this comment.
Since this relates to make_partition_data, validate_partition_data, test_shuffler methods I thought this is the best place to put it, more as a detail rather than a doc string.
There was a problem hiding this comment.
It's a strange placement, but I don't have a better idea where to put it either. Let's leave it there for now.
| async def do_shuffle( | ||
| context: Context, | ||
| comm: Communicator, | ||
| ch_in: Channel[TableChunk], | ||
| ch_out: Channel[TableChunk], | ||
| ch_in: Channel[PartitionMapChunk], | ||
| ch_out: Channel[PartitionVectorChunk], |
There was a problem hiding this comment.
I can't add the comment on the exact lines, but assumes this extends to line 146:
This replacement still bypasses the exported streaming actor wrapper. The removed cudf-based test drove rapidsmpf.streaming.coll.shuffler.shuffler() in the channel pipeline (see that in 26.06 for example), which exercised streaming/coll/shuffler.pyx including the cpp_shuffler call and PartitionAssignment mapping. This new actor instantiates ShufflerAsync directly, so a regression in the public shuffler() wrapper would no longer be caught. Now that PartitionMapChunk.from_packed_data_map() exists, can this test use the actual shuffler() actor between ch_in and ch_shuffled?
There was a problem hiding this comment.
Yes you are right. I was more focused on porting the previous tests as is, and missed this detail. I removed the do_shuffle actor alltogether, because we can strictly replace it from shuffle actor as you said.
|
I have opened #1092 and #1093 adding cuDF-free coverage for the remaining missing tests. Both require changes from this PR (specifically, |
Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
pentschev
left a comment
There was a problem hiding this comment.
Thanks @nirandaperera , one final minor request, otherwise LGTM.
Co-authored-by: Peter Andreas Entschev <peter@entschev.com>
|
Thanks @pentschev |
|
/merge |
Removes the remaining cuDF-specific dependency surface from RapidsMPF after the cuDF-dependent coverage was moved out and replaced with RapidsMPF-native tests. This removes: - `BUILD_CUDF_TESTS` and the `cudf_streaming` CMake fetch path - cuDF/cudf_streaming links from C++ tests, examples, and benchmarks - cuDF-only C++ tests, examples, benchmarks, and NDSH benchmark tooling - Python examples/tests/benchmark code that imports or requires cuDF/pylibcudf - cuDF-related package dependencies from `dependencies.yaml`, generated conda envs, pyproject metadata, and conda recipes - CI smoke-test references to deleted cuDF-specific binaries - documentation and comments that described RapidsMPF APIs in cuDF-specific terms The remaining shuffler, streaming, memory, communicator, and Ray coverage stays library-neutral and is covered by the tests added in the preceding PRs. Depends on #1084 and #1092 . Authors: - Peter Andreas Entschev (https://github.com/pentschev) - Niranda Perera (https://github.com/nirandaperera) Approvers: - Niranda Perera (https://github.com/nirandaperera) - Vyas Ramasubramani (https://github.com/vyasr) URL: #1094
[C++]Make shuffler distributed tests cudf-freeSummary
The shuffler round-trip tests depended on cudf for input generation and result checking (
random_table_with_index,partition_and_pack/unpack_and_concat,CUDF_TEST_EXPECT_TABLES_EQUIVALENT,cudf::test::BaseFixture), which made them heavy and coupled to the cudf integration. This replaces that with a self-contained, conservation-preserving data model built directly fromPackedData, so the shuffler tests no longer depend on cudf and run as part of the default (non-BUILD_CUDF_TESTS) build.Changes
C++ utilities
ceil_divand achunk_indicesview toutils/misc.hpp.chunk_indicestiles[0, count)into exactlynum_chunkscontiguous (possibly empty) ranges whose sizes sum tocount.ceil_divis computed asx / y + (x % y != 0)rather than the naive(x + y - 1) / y, which overflows for large unsigned values and is UB on signed overflow near the type maximum.ceil_divintest_misc.cpp(static_asserts for exact/remainder division, zero numerator, denominator of one, and values atuint64/int64maxima).C++ shuffler tests (now cudf-free)
test_shuffler.cpp,streaming/test_shuffler.cpp) aroundmake_partition_data(produces routed chunks) andvalidate_partition_data(verifies received chunks), dropping allcudf/cudf_testusage and switching tormm::stream/MR helpers and::testing::Test.generate_packed_data/validate_packed_dataintests/utils.hppover the element type and build device buffers via the buffer resource +cuda_memcpy_asyncinstead of cudf.streaming/test_allgather.cppfromcudf_streaming/integrations/partition.hpp.cpp/tests/CMakeLists.txt: movetest_shuffler.cpp,streaming/test_shuffler.cpp, andstreaming/test_allgather.cppout of theBUILD_CUDF_TESTSblock into the default build; the cudf block now only carriestest_partition.cpp,test_shuffler_many_streams.cpp, andstreaming/test_leaf_actor.cpp.Python (now cudf-free)
PackedData.from_device_buffer(gpu_data, metadata, stream, br)factory (plus.pyistub) so packed data can be built from an rmmDeviceBufferwithout cudf.rapidsmpf/testing.py:chunk_indices,generate_packed_data,validate_packed_data,make_partition_data, andvalidate_partition_data.tests/test_shuffler.py,tests/streaming/test_shuffler.py) around those helpers and theShuffler/ShufflerAsyncobject interfaces, removing thecudf/cudf_streaming/pylibcudfdependencies.Depends on #1087