Fix the seq-first Ulysses all2all output layout - #8317
Open
vineethsaivs wants to merge 1 commit into
Open
Conversation
vineethsaivs
requested review from
loadams,
tjruwase and
tohtana
as code owners
August 25, 2026 17:39
_generate_layout_params builds the reshape target for every all2all in DistributedAttention. For batch_dim_idx=1 (s, b, n, h) with scatter_idx < 2 it returns [bs, seq_world_size * global_seq_len, num_local_head // seq_world_size, head_dim], which is the batch_dim_idx=0 / scatter_idx >= 2 shape: it puts the batch first, multiplies the sequence and divides the heads, when this direction scatters the sequence and gathers the heads. Before deepspeedai#6750 extracted this function, post_all2all computed [seq_len // seq_world_size, bs, seq_world_size * num_head, head_dim] for that case, so the refactor copied the wrong sibling branch. Restore that shape. The element count still matches whenever num_local_head is divisible by seq_world_size, so the reshape succeeds and silently returns a transposed, mis-strided tensor; when it is not divisible, the floor division makes a dimension 0 and the reshape raises. Both are reachable from DistributedAttention, whose default gather_idx is 0: the output projection all2all and the backward of the q/k/v all2alls both run scatter_idx < 2. The existing coverage misses it. TestUlyssesAll2All only runs batch_dim_idx=0, and TestUlyssesAll2All_odd sets num_kv_heads on its first call so every later call takes uneven_heads_all2all instead of _generate_layout_params. _generate_layout_params is pure, so add TestUlyssesAll2AllLayout, which drives it with an emulated all_to_all_single and checks that both directions land the right (sequence, head) shard of a known tensor. It needs no process group and no accelerator, so it runs in the CPU CI. Against the current code the two batch_dim_idx=1 head-to-sequence cases fail (2 failed, 6 passed: one shape assertion, one reshape RuntimeError) and all 8 pass with the fix. Signed-off-by: Vineeth Sai <vineethsai4444@gmail.com>
vineethsaivs
force-pushed
the
fix/ulysses-seq-first-all2all-layout
branch
from
August 25, 2026 18:40
021970c to
2caa549
Compare
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.
Symptom
_generate_layout_params(deepspeed/sequence/layer.py) builds thereshapetarget for everyall2allinDistributedAttention. For the seq-first layout (batch_dim_idx=1,(s, b, n, h)) withscatter_idx < 2it returnsThat is the
batch_dim_idx=0/scatter_idx >= 2shape: batch first, sequence multiplied, heads divided. This direction does the opposite, it scatters the sequence and gathers the heads, so the result should be[global_seq_len // seq_world_size, bs, seq_world_size * num_local_head, head_dim].The element count still matches whenever
num_local_headis divisible byseq_world_size, so thereshapesucceeds and silently returns a transposed, mis-strided tensor. When it is not divisible the floor division makes a dimension0and the reshape raisesshape '[...]' is invalid for input of size ....Root cause
A copy of the wrong sibling branch during a refactor. Before #6750 extracted this function,
post_all2allcomputed the shape inline and this case read:The permute survived the refactor unchanged; only the reshape target was replaced, with the shape from the
batch_dim_idx=0/scatter_idx >= 2branch.Reachability
DistributedAttention.__init__defaults togather_idx=0, and the output-projectionall2allswaps the two indices:so it runs with
scatter_idx=0._SeqAllToAll.backwardswaps them too, so the backward of the q/k/vall2alls takes the same path. Both are the seq-first(s, b, n, h)layout used by Megatron-DeepSpeed.Why the existing tests miss it
TestUlyssesAll2Allonly runsbatch_dim_idx = 0.TestUlyssesAll2All_odddoes coverbatch_dim_idx = 1, but its first call hasnum_heads % seq_world_size != 0, which callsset_num_kv_heads(...). From then onget_num_kv_heads() is not Noneroutes every later call touneven_heads_all2all, so_generate_layout_paramsis never reached.DistributedTestclasses behindskip_on_arch(min_arch=8), so neither runs in the CPU CI.Fix
One line, restoring the pre-#6750 shape, plus a two-line comment naming the direction.
Test
_generate_layout_params,pre_all2all_funandpost_all2allare pure, soTestUlyssesAll2AllLayoutdrives them with an emulatedall_to_all_single(rankisends chunkjof dim 0 to rankj) and checks that both directions land the right(sequence, head)shard of a known tensor. No process group, no accelerator, so it runs in the CPU CI, where this branch currently has no coverage at all.yapf --style .style.yapfandflake8are clean on both files.