[coll] Prevent all-to-all connection.#12075
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the CPU collective TCP backend to avoid an all-to-all connection pattern by using a fixed, sparse topology (ring neighbors + binomial tree rooted at rank 0), while keeping collective semantics intact (including non-zero-root broadcast via relay).
Changes:
- Introduce sparse peer selection (
SparsePeers) and sparse channel population (HasChan+Changuard) for the TCP communicator. - Rework worker-to-worker connection bootstrap to connect only to the sparse peer set, and adjust tests to account for missing channels.
- Update CPU broadcast to use a fixed tree rooted at rank 0 (with relay for non-zero roots) and adjust CPU allgatherV to prefer ring.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/cpp/collective/test_worker.h | Avoids touching non-existent channels in tests by checking HasChan. |
| src/collective/comm.h | Adds sparse topology helper (SparsePeers) and HasChan; makes Chan() enforce topology. |
| src/collective/comm.cc | Changes bootstrap to connect only to sparse peers; stores nullptr channels for non-neighbors. |
| src/collective/coll.h | Copyright year range update. |
| src/collective/broadcast.cc | Implements fixed-tree broadcast rooted at 0 with relay for non-zero roots. |
| src/collective/allgather.h | Selects ring allgatherV for CPU and broadcast-based for CUDA. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use a static topology instead.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
trivialfis
changed the title
RAMitchell
left a comment
RAMitchell
left a comment
There was a problem hiding this comment.
What benefits are you expecting from this? The changes looked clean to me I'm just not familiar with networking code. By the way the only major user of cpu all gather now seems to be cpu histogram column mode split sharing.
Reduced the number of connections. Before this PR, with N nodes, we had O(N^2) connections. Now it's linear. One can reproduce the connection error locally by over-subscribing the machine. For example, my desktop has 24 logical CPUs, and running 32 workers can cause trouble. In practice, this significantly limits the ability to scale. Different implementations use different strategies to avoid all-to-all connections. It might be lazy connection, might be a routing table, might be something more sophisticated. The PR picks the easiest one.
That's great news! Thank you for working on the quantile sketching! |
Use a static topology instead. The topology is a union of a ring and a tree.
Close #10840