feat(placement): bond parallel RDMA links in JACCL device matrix#2195
Draft
Nottlespike wants to merge 1 commit into
Draft
feat(placement): bond parallel RDMA links in JACCL device matrix#2195Nottlespike wants to merge 1 commit into
Nottlespike wants to merge 1 commit into
Conversation
The JACCL device matrix builder (get_mlx_jaccl_devices_matrix) previously selected a single RDMA interface per device pair and discarded any additional parallel links via an early break. This left extra Thunderbolt (and on Linux, InfiniBand) links unused even when the underlying JACCL ring backend is built to stripe across up to RING_MAX_CONNS (=4) wires per direction. This change collects every RDMA interface per pair and emits the matrix cells as lists of interface names (list[list[list[str] | None]]), which the JACCL backend's JSON parser already accepts (string or array of strings per cell). Link counts are normalised down to the minimum observed across all pairs, so the backend's ring validator (which requires uniform connection counts on every ring edge) never rejects an asymmetric matrix. Bonding is effective for ring/tensor-parallel workloads; the mesh fallback path only consumes the first interface per pair (a JACCL limitation). - Widen MlxJacclInstance.jaccl_devices type to list[list[list[str] | None]] - Collect all RDMAConnection edges per pair instead of breaking on the first - Normalise to uniform per-pair count for ring validator compatibility - Update existing tensor-parallel matrix test assertions to list cells - Add test_jaccl_devices_matrix_bonds_parallel_rdma_links - Add test_jaccl_devices_matrix_normalises_asymmetric_link_counts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
get_mlx_jaccl_devices_matrixselected a single RDMA interface per device pair and discarded any additional parallel links via an earlybreak:This left extra Thunderbolt links unused even when two Macs are connected by more than one cable. The underlying JACCL ring backend is already built to bond:
RING_MAX_CONNS = 4,RingGroupacceptsleft_devices/right_devicesas vectors, andRingImplstripes acrossn_conns_wires with per-wire buffers. The backend's JSON parser (parse_devices_json) also already accepts either a single string or an array of strings per cell. exo was the bottleneck — it threw away links the backend was designed to consume.Changes
src/exo/master/placement_utils.py—get_mlx_jaccl_devices_matrixnow:RDMAConnectioninterface per pair instead of breaking on the first.list[list[list[str] | None]](a list of interface names per cell, orNoneon the diagonal).is_valid_ringatjaccl.cpp:130-141) never rejects an asymmetric matrix.src/exo/shared/types/worker/instances.py—MlxJacclInstance.jaccl_deviceswidened fromlist[list[str | None]]tolist[list[list[str] | None]].src/exo/master/tests/test_placement.py— updated the existing tensor-parallel matrix assertions to expect list cells, plus two new tests:test_jaccl_devices_matrix_bonds_parallel_rdma_links— multiple parallel links survive into the cell.test_jaccl_devices_matrix_normalises_asymmetric_link_counts— asymmetric link counts are trimmed to a uniform minimum.Why It Works
The JACCL backend (the forked
mlx-jaccl-fix-small-recvwheels exo pins) consumesMLX_IBV_DEVICESas a JSON matrix. Its parser handles both forms per cell:RingGroupthen receivesvector<string> left_devices/right_devices, callscreate_connections(plural), andRingImplactively stripes sends/recvs acrossn_conns_wires. So emitting arrays was always supported downstream — exo just wasn't feeding the data through.The uniform-count normalisation is required because
is_valid_ringrejects a matrix where ring edges have differing connection counts (jaccl.cpp:136). Trimming to the minimum is the conservative choice — bonding only helps when it's consistent across the ring, and a partially-bonded ring would be rejected anyway.Known limitation: the mesh path (
get_mesh_connectivity,jaccl.cpp:152) takes onlydevices_[rank][dst][0], so bonding is effective for ring/tensor-parallel all-reduce (where the heavy traffic is) but not for the mesh fallback. That's a JACCL limitation, not addressable from exo.Test Plan
Manual Testing
Hardware available: Two Thunderbolt 5-capable Macs, connected by two Thunderbolt 4 cables (TB4 cables between TB5 ports — links negotiate to TB4 signalling). This is exactly the multi-link scenario this change targets: two parallel cables between one pair, which the matrix builder now feeds to the backend as a two-element cell instead of discarding one.
End-to-end throughput benchmarking on this 2×TB4-cable setup is the planned validation. The change is already verified at the configuration/data level (both interface names reach the backend in the cell) and by reading the JACCL ring striping code. A bandwidth measurement on the dual-cable pair vs single-cable would confirm the gain and is the concrete next step I can run with the hardware above.
Automated Testing
uv run ruff check— cleanuv run basedpyright— 0 errors, 0 warningsuv run pytest src/exo/master/tests/test_placement.py src/exo/shared/tests/ src/exo/utils/info_gatherer/— 79 passed, 1 skipped (macOS-onlytest_tb_parsing)Formatting note
CONTRIBUTING.mdspecifiesnix fmt. I don't have Nix available in my environment, so I ranruff formatinstead. If CI'streefmtconfig wants something different, a maintainer can re-runnix fmton this branch.Draft — opening for review, not ready to merge. Happy to adjust naming, test coverage, or the normalisation strategy based on feedback.