Skip to content

fix(sp): remove ring padding before unpacking model outputs - #10184

Open
Excelius-Wang wants to merge 1 commit into
modelscope:mainfrom
Excelius-Wang:fix/ring-output-unpadding
Open

Excelius-Wang wants to merge 1 commit into
modelscope:mainfrom
Excelius-Wang:fix/ring-output-unpadding

Conversation

@Excelius-Wang

Copy link
Copy Markdown
Contributor

The sequence-parallel output hook gathers Ring outputs with padding between packed samples, then revert_padding_free splits them using the original unpadded position IDs. Later samples can therefore include earlier padding and lose their own trailing tokens, changing pooled representations and downstream classification outputs.

Remove Ring padding from each gathered output using the already-padded position-ID mask before downstream unpacking. Reuse the existing gather, padding layout, and autograd path; leave Ulysses-only behavior unchanged.

Validation:

  • CPU Gloo regression tests cover Ring, Ring + Ulysses, Ulysses-only, and no-SP configurations across four layouts, both output keys, and left/right repadding (64 cases). Use real ModelOutput containers and verify output identity and unrelated attention metadata are preserved.
  • Compare unpacked tensors and local gradients against an independently padded reference, and verify the existing classification head's logits and loss for left-padded inputs.
  • Before: two Ring configurations fail, two controls pass. After: all 4 tests pass.
  • pre-commit run --all-files: passed.

A separate tiny Qwen3 CPU probe with natural dialogue lengths also matches reference hidden states, classification logits/loss, and local output gradients after the fix. Attention in that probe runs per full sequence; no full GPU Ring-attention training run was performed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant