Skip to content

[dev] fix(ssm): handle alignment padding in GDN packed seq + CP - #4230

Merged
yuzhongw-nvidia merged 5 commits into
NVIDIA:devfrom
yxs:fix/gdn-packed-cp-alignment
Apr 13, 2026
Merged

[dev] fix(ssm): handle alignment padding in GDN packed seq + CP#4230
yuzhongw-nvidia merged 5 commits into
NVIDIA:devfrom
yxs:fix/gdn-packed-cp-alignment

Conversation

@yxs

@yxs yxs commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

What does this PR do ?

Fix shape mismatch in GDN packed sequence + CP > 1 when alignment padding is present.

Summary

When cu_seqlens_q_padded is not provided, GDN falls back to cu_seqlens_q (actual token boundaries) for the per-sequence all-to-all unpack. Alignment padding tokens are lost, making the output shorter than hidden_states, crashing at the residual connection.

Fix: Extract cu_seqlens resolution into _resolve_cu_seqlens():

  • cu_seqlens_padded available → use it (unchanged behavior)
  • CP = 1 → use actual (no all-to-all, no issue)
  • Single sequence + padding → auto-extend to cover trailing padding
  • Multi sequence + padding → raise ValueError requesting cu_seqlens_padded

Test results

Unit tests: 5/5 passed

(2× H100, CP=2):

  • Single seq + padding + no cu_seqlens_padded → auto-extend, output shape correct
  • Multi seq + padding + no cu_seqlens_paddedValueError raised correctly
  • With cu_seqlens_padded set → standard path works
  • No padding → passes through unchanged

Fixes #4194

PR for main: #2645

@yxs
yxs requested review from a team as code owners April 9, 2026 11:28
@copy-pr-bot

copy-pr-bot Bot commented Apr 9, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@yxs
yxs force-pushed the fix/gdn-packed-cp-alignment branch from 45d1a49 to 7e32649 Compare April 9, 2026 11:31
@yxs

yxs commented Apr 9, 2026

Copy link
Copy Markdown
Contributor Author

cc @yuzhongw-nvidia

Comment thread megatron/core/ssm/gated_delta_net.py Outdated
Comment thread megatron/core/ssm/gated_delta_net.py Outdated
@yxs
yxs requested a review from yuzhongw-nvidia April 10, 2026 02:11
Comment thread megatron/core/ssm/gated_delta_net.py Outdated
@yuzhongw-nvidia

Copy link
Copy Markdown
Contributor

/claude review

Comment thread megatron/core/ssm/gated_delta_net.py Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall — clean fix with solid test coverage. Left one minor nit on the error message formatting.

@yuzhongw-nvidia

Copy link
Copy Markdown
Contributor

/ok to test 8a5cbc3

@yxs

yxs commented Apr 11, 2026

Copy link
Copy Markdown
Contributor Author

@yuzhongw-nvidia is this PR ready to merge?

yxs and others added 5 commits April 12, 2026 17:52
When cu_seqlens_q_padded is not provided and alignment padding exists,
GDN's packed all-to-all loses padding tokens, causing shape mismatch
at the residual connection.

Fix:
- Extract cu_seqlens resolution into _resolve_cu_seqlens() method
- Single-sequence case: auto-extend cu_seqlens to cover trailing padding
- Multi-sequence case: raise clear error requesting cu_seqlens_padded
- Add unit tests for cu_seqlens validation logic

Validated on 2x H100 (CP=2):
- Single seq with padding + no cu_seqlens_padded: output shape correct
- Multi seq with padding + no cu_seqlens_padded: ValueError raised
- With cu_seqlens_padded set: standard path works
- No padding case: passes through unchanged

Fixes: NVIDIA#4194

Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@yuzhongw-nvidia
yuzhongw-nvidia force-pushed the fix/gdn-packed-cp-alignment branch from 8a5cbc3 to a5b5110 Compare April 13, 2026 00:52
@yuzhongw-nvidia

Copy link
Copy Markdown
Contributor

/ok to test a5b5110

@yuzhongw-nvidia
yuzhongw-nvidia added this pull request to the merge queue Apr 13, 2026
@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🔄 Merge queue validation started!

You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/24326368290

Merged via the queue into NVIDIA:dev with commit 0f6fcb0 Apr 13, 2026
61 checks passed
@yuzhongw-nvidia

Copy link
Copy Markdown
Contributor

@yuzhongw-nvidia is this PR ready to merge?

Merged. Many thanks for your contribution!

@Phlip79

Phlip79 commented Apr 14, 2026

Copy link
Copy Markdown
Member

/claude copy

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🤖 This PR has been cherry-picked to main in #4289.

yuzhongw-nvidia added a commit to yuzhongw-nvidia/Megatron-LM that referenced this pull request Apr 14, 2026
…IA#4230)

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Yuzhong Wang <yuzhongw@nvidia.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
yuzhongw-nvidia added a commit to yuzhongw-nvidia/Megatron-LM that referenced this pull request Apr 15, 2026
…IA#4230)

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Yuzhong Wang <yuzhongw@nvidia.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
yuzhongw-nvidia added a commit to yuzhongw-nvidia/Megatron-LM that referenced this pull request Apr 15, 2026
…IA#4230)

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Yuzhong Wang <yuzhongw@nvidia.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
yuzhongw-nvidia added a commit that referenced this pull request Apr 16, 2026
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Yuzhong Wang <yuzhongw@nvidia.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
yuzhongw-nvidia added a commit to yuzhongw-nvidia/Megatron-LM that referenced this pull request Apr 28, 2026
…IA#4230)

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Yuzhong Wang <yuzhongw@nvidia.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
yuzhongw-nvidia added a commit to yuzhongw-nvidia/Megatron-LM that referenced this pull request Apr 29, 2026
…IA#4230)

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Yuzhong Wang <yuzhongw@nvidia.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
dreamyang-liu added a commit to dreamyang-liu/Megatron-LM that referenced this pull request Jun 26, 2026
…DeltaNet

Port the packed-sequence support from upstream NVIDIA/Megatron-LM PRs NVIDIA#2644
and NVIDIA#4230 into the GatedDeltaNet forward (adapted to this fork's GDN code):

- Gate packed handling on `packed_seq_params.qkv_format == 'thd'` (not just
  `is not None`), so the sbhd PackedSeqParams Megatron passes uniformly is
  ignored and only genuine THD packing is processed.
- Resolve cu_seqlens via `_resolve_cu_seqlens`, preferring *_padded and
  validating cu_seqlens[-1] == total_seq_len (alignment-padding handling).
- Unpack the packed stream per-sequence around both CP all-to-all calls
  (cp2hp / hp2cp) via `_unpack_sequence`, and pass cu_seqlens into the
  causal_conv1d and gated_delta_rule kernels.
- Guard torch_chunk_gated_delta_rule against cu_seqlens (unsupported).
- Assert batch == 1 and non-deterministic mode for packed sequences.

Also keep the independent fp32 numerics fix: cast A_log and dt_bias to fp32
before exp/softplus when computing g, matching the "In fp32" intent.

Verified the cu_seqlens[-1] == seq_len invariant holds for the target
training config (TP=4, sequence-parallel, CP=1, THD): seq_len = (L/tp)*sp*cp
= L and cu_seqlens[-1] = L*cp = L. Note CP>1 + GDN is a known unsupported
edge that would trip the validation.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants