Cudf_streaming dedup tests from rapidsmpf - #22858
Conversation
Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
|
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 with no reviewable changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR removes streaming test files (AllGather and ManyStreams), prunes additional test cases from leaf and partition tests, simplifies StreamingShuffler fixture hierarchy to inherit directly from BaseStreamingFixture, and refactors test_shuffler to call rapidsmpf::shuffler::Shuffler APIs directly instead of accepting callback functors. ChangesTest suite cleanup and refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/libcudf_streaming/tests/test_shuffler.cpp`:
- Around line 234-236: The loop over futures in test_shuffler.cpp currently
calls f.wait(), which won't rethrow exceptions from asynchronous workers;
replace the wait() calls with ASSERT_NO_THROW(f.get()) so any exceptions from
the async tasks (the futures in the futures container) are surfaced and cause
the test to fail. Locate the loop that iterates "for (auto& f : futures)" and
change the body to call get() on each future inside ASSERT_NO_THROW rather than
calling wait().
🪄 Autofix (Beta)
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: e0148017-db23-45d9-94cd-fe61c8d32e23
📒 Files selected for processing (7)
cpp/libcudf_streaming/tests/CMakeLists.txtcpp/libcudf_streaming/tests/streaming/test_allgather.cppcpp/libcudf_streaming/tests/streaming/test_leaf_actor.cppcpp/libcudf_streaming/tests/streaming/test_partition.cppcpp/libcudf_streaming/tests/streaming/test_shuffler.cppcpp/libcudf_streaming/tests/test_shuffler.cppcpp/libcudf_streaming/tests/test_shuffler_many_streams.cpp
💤 Files with no reviewable changes (5)
- cpp/libcudf_streaming/tests/test_shuffler_many_streams.cpp
- cpp/libcudf_streaming/tests/CMakeLists.txt
- cpp/libcudf_streaming/tests/streaming/test_allgather.cpp
- cpp/libcudf_streaming/tests/streaming/test_leaf_actor.cpp
- cpp/libcudf_streaming/tests/streaming/test_partition.cpp
pentschev
left a comment
There was a problem hiding this comment.
There's a lot of cuDF-specific testing being removed here, that's not what we agreed previously. In RapidsMPF we added various generic-tests, but we should keep testing cuDF-specific stuff here, therefore I don't see why several of the tests are being removed, especially not under a test deduplication justification.
pentschev
left a comment
There was a problem hiding this comment.
Alright, if you confirm my comment/question in cpp/libcudf_streaming/tests/streaming/test_shuffler.cpp is correct, then I think we're good. Most of my comments were noise on my part, sorry about that! There's also a CodeRabbit comment that seems relevant, would be good to get addressed. Thanks Niranda.
…edup-tests Signed-off-by: niranda perera <niranda.perera@gmail.com>
Removed 'test_shuffler_many_streams.cpp' from the test list.
|
/ok to test 0e02176 |
|
/merge |
Description
Remove duplicated tests
Checklist