Flatten hierarchical collective payloads#7370
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe collectives module introduces flattened payload storage and utilities, extends gather, scatter, and all-to-all communication paths, refactors hierarchical subtree exchanges, and adds coverage for flattened data, payload types, and uneven locality topologies. ChangesFlattened hierarchical collectives
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LocalSite
participant SubtreeRepresentative
participant CollectiveCommunicator
LocalSite->>SubtreeRepresentative: gather or scatter flattened payload
SubtreeRepresentative->>CollectiveCommunicator: exchange flattened blocks
CollectiveCommunicator-->>SubtreeRepresentative: gathered or selected flattened data
SubtreeRepresentative-->>LocalSite: scatter and unwrap row
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
|
Can one of the admins verify this patch? |
Replace nested intermediate vectors with serializable flat buffers and offset tables for hierarchical gather, scatter, and all-to-all. Keep all-to-all diagonal blocks local and add coverage for uneven trees, bool payloads, strings, serialization, and non-default-constructible values. Signed-off-by: iemAnshuman <asquare567@gmail.com>
iemAnshuman
force-pushed
the
feat/flatten-hierarchical-collectives
branch
from
c69e7cb to
b77a379
Compare
|
@hkaiser sorry I ran into a git conflict...will resolve it soon. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
libs/full/collectives/tests/unit/hierarchical_flat_fallback.cpp (1)
120-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting a generic payload-verification helper to avoid triplicated blocks.
The uint32/string/bool verification blocks (fb_clients and tree_clients legs) are structurally identical aside from the payload type and generation number. A small templated helper (e.g.,
verify_all_to_all<T>(clients, send, expected, generation)) would remove the duplication and make adding a 4th payload type (or extending coverage) less error-prone.Also applies to: 187-207
🤖 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 `@libs/full/collectives/tests/unit/hierarchical_flat_fallback.cpp` around lines 120 - 161, Extract the repeated all_to_all result-size and element verification logic into a templated helper such as verify_all_to_all, parameterized by payload type, clients, send data, expected data, and generation. Replace the duplicated uint32, string, and bool checks in both fb_clients and tree_clients sections with this helper while preserving each payload’s generation value and expected ordering.libs/full/collectives/include/hpx/collectives/detail/flattened_data.hpp (1)
189-304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the destructive move semantics of
merge_flattened_data/slice_flattened_data/select_flattened_column.These take mutable references (
flattened_data<T>&/std::vector<flattened_data<T>>&) and, for non-boolT, move-construct elements out of the source viaappend_flattened_range's move-iterator branch (Line 139-141). This leaves the caller's input in a moved-from (unspecified but valid) state, e.g. strings become empty. Callers within this PR are safe today (disjoint-segment access pattern and single-use inputs), but the API doesn't signal this destructive behavior, which is a foot-gun for future maintainers reusing an input after calling one of these.Consider documenting this explicitly (e.g., a
// NOTE: moves elements out of value/valuescomment), or making it explicit at the type level (accept&&).🤖 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 `@libs/full/collectives/include/hpx/collectives/detail/flattened_data.hpp` around lines 189 - 304, Document that merge_flattened_data, slice_flattened_data, and select_flattened_column destructively move non-bool elements from their mutable input value or values, leaving sources valid but unspecified. Add clear comments at these APIs, or change their parameters to rvalue references if the surrounding callers support that contract; preserve the existing behavior and avoid implying inputs remain reusable.
🤖 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.
Nitpick comments:
In `@libs/full/collectives/include/hpx/collectives/detail/flattened_data.hpp`:
- Around line 189-304: Document that merge_flattened_data, slice_flattened_data,
and select_flattened_column destructively move non-bool elements from their
mutable input value or values, leaving sources valid but unspecified. Add clear
comments at these APIs, or change their parameters to rvalue references if the
surrounding callers support that contract; preserve the existing behavior and
avoid implying inputs remain reusable.
In `@libs/full/collectives/tests/unit/hierarchical_flat_fallback.cpp`:
- Around line 120-161: Extract the repeated all_to_all result-size and element
verification logic into a templated helper such as verify_all_to_all,
parameterized by payload type, clients, send data, expected data, and
generation. Replace the duplicated uint32, string, and bool checks in both
fb_clients and tree_clients sections with this helper while preserving each
payload’s generation value and expected ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c368f2fa-3009-4b3b-a121-0b617facf702
📒 Files selected for processing (26)
libs/full/collectives/CMakeLists.txtlibs/full/collectives/include/hpx/collectives/all_gather.hpplibs/full/collectives/include/hpx/collectives/all_reduce.hpplibs/full/collectives/include/hpx/collectives/all_to_all.hpplibs/full/collectives/include/hpx/collectives/barrier.hpplibs/full/collectives/include/hpx/collectives/broadcast.hpplibs/full/collectives/include/hpx/collectives/create_communicator.hpplibs/full/collectives/include/hpx/collectives/detail/communicator.hpplibs/full/collectives/include/hpx/collectives/detail/flattened_data.hpplibs/full/collectives/include/hpx/collectives/detail/hierarchical_all_to_all_helpers.hpplibs/full/collectives/include/hpx/collectives/detail/hierarchical_helpers.hpplibs/full/collectives/include/hpx/collectives/detail/hierarchical_scan_helpers.hpplibs/full/collectives/include/hpx/collectives/gather.hpplibs/full/collectives/include/hpx/collectives/reduce.hpplibs/full/collectives/include/hpx/collectives/scatter.hpplibs/full/collectives/src/barrier.cpplibs/full/collectives/src/create_communicator.cpplibs/full/collectives/src/detail/hierarchical_helpers.cpplibs/full/collectives/tests/unit/CMakeLists.txtlibs/full/collectives/tests/unit/barrier.cpplibs/full/collectives/tests/unit/barrier_hierarchical.cpplibs/full/collectives/tests/unit/gather_hierarchical.cpplibs/full/collectives/tests/unit/hierarchical_flat_fallback.cpplibs/full/collectives/tests/unit/hierarchical_helpers.cpplibs/full/collectives/tests/unit/inclusive_scan_hierarchical.cpplibs/full/collectives/tests/unit/subtree_gather_scatter.cpp
3 participants
Fixes #
Proposed Changes
gather,scatter, andall_to_allAny background context you want to provide?
Hierarchical gather/scatter/all-to-all previously shuttled nested
vectorpayloads between tree levels. Flattening to a contiguous buffer plus offset table reduces allocation churn and simplifies serialization across the communicator tree while preserving existing hierarchical generation semantics.This PR is based directly on current
masterand does not include the collectives hardening work from #7359.Checklist
Not all points below apply to all pull requests.