Skip to content

Support learnable softmax (attention sink) with cp_comm_type="p2p" - #3551

Open
0z5a wants to merge 3 commits into
NVIDIA:mainfrom
0z5a:cp-p2p-learnable-softmax
Open

0z5a wants to merge 3 commits into
NVIDIA:mainfrom
0z5a:cp-p2p-learnable-softmax

Conversation

@0z5a

@0z5a 0z5a commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

Incremental review: Files changed against main.

Addresses #2541.

What was broken

softmax_type="learnable" (the learnable attention sink) with cp_comm_type="p2p" was rejected by an assertion. Behind that assertion, the p2p fused-attention helpers never passed softmax_type to fused_attn_fwd / fused_attn_bwd, so the kernels fell back to "vanilla" and ignored softmax_offset.

What changed

  • cp_p2p_fwd_fused_attn / cp_p2p_bwd_fused_attn take softmax_type and softmax_offset. The offset joins the auxiliary tensor pack in the order the forward returns it ([stats, rng, (bias), (offset)]). The previously unconditional attn_bias = rest[0] is now guarded by whether a bias was passed, because with a sink the pack would otherwise return the offset as the bias.
  • The sink enters each query row's softmax exactly once. cuDNN counts the sink as one more softmax column, so the softmax stats of a tile include e^offset. The p2p forward merges the tiles' stats, so passing the offset to every tile would add the sink once per tile that a row visits. Only step 0 gets the sink: it is the local KV tile, and it covers every local query row (the diagonal tile under a causal mask, the full tile without one). The other tiles run a plain softmax. In the backward, only the mirror tile passes the sink and returns its gradient. The other tiles' dq/dk/dv use the merged stats, which already include the sink.
  • Sink gradient reduction. Each rank's sink gradient covers only its own query rows. It is all-reduced over the CP group, so every rank returns the full-sequence gradient. This matches the a2a path, which all-gathers the per-head gradients, and it is what run_attention_with_cp.py compares against.
  • utils.py and attn_forward_func_with_cp: fused attention is no longer disabled for non-vanilla softmax with p2p, except with FP8 attention. That case is unverified, and the FP8 branch of the per-tile forward unpacks a two-entry aux pack, so it stays off FusedAttention.
  • Tests:
    • test_attention_with_cp.py now runs the non-vanilla softmax cases (cp_4_1 off-by-one, cp_4_2 learnable) for p2p as well as a2a.
    • run_attention_with_cp.py kept the reference sink gradient as an alias of softmax_offset.grad, then zeroed it before the CP run accumulated into the same tensor. The sink-gradient check therefore compared the CP result with itself, for a2a as well. It now clones the reference.
    • The learnable offsets are drawn as randn * 5 from a fixed CPU generator. With the default zero offset, the sink is almost invisible next to thousands of keys.

An earlier version of this PR passed the sink to every tile and did not reduce the gradient. Both errors cancel without a mask at cp=2: every row was counted twice, and each rank held half the rows. Under a causal mask they don't cancel: the sum of the per-rank sink gradients was off by up to 0.84 (reference max 6.36). The CP test skipped non-vanilla softmax for p2p, and its sink-gradient check compared against an alias, so neither caught it.

Verification

Two GH200 nodes (sm_90), one GPU each, cross-node over InfiniBand, cp=2, BF16, FusedAttention. The run used TE main fac8d4a9 with this PR's Python files on top, cuDNN 9.24.0, and torch 2.14.1+cu130. run_attention_with_cp.py was run with the changes above:

softmax comm layout TE Python result
learnable (cp_4_2) p2p bshd this PR pass
learnable (cp_4_2) p2p sbhd this PR pass
off-by-one (cp_4_1) p2p bshd this PR pass
vanilla (cp_4_0) p2p bshd this PR pass
learnable (cp_4_2) a2a bshd main pass
off-by-one (cp_4_1) a2a bshd main pass
learnable (cp_4_2) p2p bshd previous PR head 649969bb fail: sink gradient, 12/64 heads, max abs diff 1.55

"pass" means out, dq, dk, dv and the sink gradient are all within the runner's tolerances. All configs are causal GQA with s=4096. The two a2a rows show that the runner changes keep the existing a2a cases passing.

Sink gradient, per rank, max abs diff against the non-CP reference (cp_4_2, offsets randn * 5):

mask reference max previous PR head this PR
causal 6.36 0.84 (sum over ranks) 0.0032
no_mask 2.55 0.51 (sum over ranks) 0.0030

An FP64 emulation of the p2p tile schedule shows the same pattern. With the sink on every tile, the ratio of the CP sum to the reference ranges from 0.5× to 2× per head at cp=2, and from 0.25× to 4× at cp=4. With the sink on step 0 only, it matches to 1e-15 at both sizes.

Not covered

The test changes enable more CI cases than I ran. I ran BF16 bshd/sbhd for p2p and BF16 bshd for a2a, on sm_90 with cuDNN 9.24. These newly enabled or changed cases were not run here:

  • p2p with qkv_format="thd": main requires cuDNN ≥ 9.26 for a ragged layout with a sink in the backward.
  • p2p and a2a with FP16.
  • a2a sbhd/thd and cp_4_3 (sliding window, learnable) with the new non-zero offsets.
  • FP8 a2a with a sink, which needs sm100 and cuDNN ≥ 9.21.
  • cp_size > 2 on GPUs (only the emulation covers cp=4), and the flash-attention backend.

cp_comm_type="a2a+p2p" is still rejected for non-vanilla softmax.

Question for reviewers

Both a2a and p2p now return the full-sequence sink gradient on every CP rank. Other parameters get only their own tokens' gradient on each CP rank. Megatron-LM scales the loss by the CP size and scales dense gradients by 1 / dp_cp_group.size() before the DP×CP reduction. As far as I can tell, it treats softmax_offset like any other parameter. In that setup, the sink gradient would come out cp_size times larger than the weights' gradients. Is the full gradient on every rank the intended contract, or should both paths return the per-rank partial sum? I kept the existing a2a contract here so that the two paths agree.

@github-actions github-actions Bot added the community-contribution PRs from external contributor outside the core maintainers, representing community-driven work. label Sep 20, 2026
@0z5a
0z5a marked this pull request as ready for review September 21, 2026 04:51
@0z5a
0z5a requested a review from cyanguwa as a code owner September 21, 2026 04:51
@greptile-apps

greptile-apps Bot commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 4/5

[Medium impact] The change reaches P2P context-parallel attention users who choose non-vanilla softmax.

The PR still needs the existing bias-gradient shape bug fixed before merging.

Findings

  1. P1 Offset Corrupts Bias Gradient Shape ▶

Summary

Adds learnable-softmax support to fused P2P attention.

  • P2P context-parallel attention now applies the learnable sink.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Local tile with sink] --> C[Merge tile outputs and statistics]
  B[Other tiles without sink] --> C
  C --> D[Backward with merged output and statistics]
  D --> E[Local tile returns sink gradient]
  E --> F[Sum sink gradients across CP ranks]
Loading

Reviews (4) · Last reviewed commit: "Test learnable and off-by-one softmax wi..." · Reviewed by Greptile

Comment thread transformer_engine/pytorch/attention/dot_product_attention/utils.py
Comment on lines +1386 to +1389
# Aux pack order is [softmax_stats, rng_state, (bias), (softmax_offset)],
# matching what the forward returned.
if softmax_offset is not None:
aux_tensors += [softmax_offset]

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.

P1 Offset Corrupts Bias Gradient Shape

With post_scale_bias and learnable softmax, P2P reconstructs the backward auxiliary pack as [stats, rng, bias, offset]. The fused backward binding allocates dBias using the shape of the last auxiliary tensor, which is now the [1, h, 1, 1] offset rather than the tile bias. This configuration cannot return a correctly shaped bias gradient and may fail backward shape validation.

@0z5a
0z5a force-pushed the cp-p2p-learnable-softmax branch from 69fe4a3 to b752897 Compare September 24, 2026 01:11
The p2p fused-attention helpers never passed softmax_type to fused_attn_fwd or
fused_attn_bwd, so the kernels fell back to their "vanilla" default and discarded
softmax_offset on the floor. Nothing failed loudly: the forward returned the
vanilla answer and the sink gradient came back as an exact zero. The a2a path has
always passed it; p2p never did.

Thread softmax_type through both helpers, add the offset to the auxiliary tensor
pack in the order the forward returns it, and accumulate the per-tile sink gradient
into the p2p backward's return tuple. The offset is replicated across CP ranks and
every rank computes all query rows, so each already holds the full gradient and the
outer framework's DDP average reproduces it; no extra reduction is needed here.

The aux pack is [stats, rng, (bias), (offset)], so the previously unconditional
attn_bias = rest[0] is now guarded -- otherwise it would hand the offset back as the
attention bias once a sink is active.

Also bind d_sink_ before the section dispatch: it is assigned only in the fused
backward branch, so a vanilla run on the flash backend would have raised
UnboundLocalError at the accumulation.

Verified against an FP64 oracle at cp=2, BF16, 1024 tokens: the p2p forward matches
to 9.5e-3 relative error and dSink to 1.7e-3, on par with the already-supported a2a
path. The oracle's own autograd and finite-difference answers agree to 2.3e-08.
Not covered: qkv_format="thd", cp_size > 2, and the flash backend.

Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
@0z5a
0z5a force-pushed the cp-p2p-learnable-softmax branch from b752897 to 649969b Compare September 24, 2026 05:52
0z5a added 2 commits October 27, 2026 11:26
cuDNN treats the sink as one more softmax column, so the softmax stats of
every p2p tile included it. Passing the offset to every tile made the LSE
merge add e^offset once per tile that a query row visits: once for the
first chunk on rank 0 under a causal mask, cp_size times without a mask.
The output error was hidden because it is large only where the sink
dominates, and there the output is small. The sink gradient was wrong,
and each rank held only the gradient of its own query rows.

Pass the sink only to step 0, the local KV tile that covers every local
query row, and to its mirror tile in the backward. The other tiles run a
plain softmax against the merged stats, which already include the sink.
All-reduce the sink gradient over the CP group so every rank returns the
full gradient, as the a2a path does.

FP8 attention with a sink on p2p is unverified, and the FP8 branch of the
per-tile forward unpacks a two-entry aux pack. Keep that combination off
FusedAttention for now.

Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
Run the non-vanilla softmax CP cases for p2p as well as a2a. The runner
kept the reference sink gradient as an alias of the parameter's .grad,
zeroed it, and then let the CP run accumulate into the same tensor, so
the comparison checked the CP result against itself; clone it instead.
Draw the learnable offsets around the scale of the attention logsumexp
so that the sink changes the output and its gradient.

Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
@0z5a

0z5a commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

I pushed a correction to this PR: 462355e ("Count the attention sink once per query row in CP p2p") and aa5a177 (tests).

The earlier version was wrong. cuDNN counts the sink as one more softmax column, so it is included in every p2p tile's softmax stats. Passing the offset to every tile made the LSE merge count the sink once per tile that a query row visits. I also claimed that every rank already holds the full sink gradient. That is not true for p2p, where each rank computes only its own query rows. Without a mask at cp=2, the two errors cancel, which is what my original check measured. Under a causal mask they don't.

The sink now goes only to the local KV tile (step 0), and the sink gradient is all-reduced over the CP group to match a2a. The CP test now covers p2p for learnable and off-by-one softmax. It also fixes an aliased reference that made the sink-gradient check compare the CP result with itself. I updated the description with the cross-node GH200 results, and the updated test fails on the previous head.

There is one open question at the end of the description, about whether every CP rank should return the full sink gradient (the current a2a contract) or only its partial sum.

This branch has not been deployed

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

Labels

attention community-contribution PRs from external contributor outside the core maintainers, representing community-driven work.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants