[PyT] Add per-sequence attention policies to packed THD attention - #3274
Conversation
| @pytest.mark.parametrize( | ||
| "sequence_is_causal", | ||
| ( | ||
| (False, True, False, True), |
There was a problem hiding this comment.
The PyTorch QA job explicitly enumerates attention test modules and does not include this new file, so its forward, gradient, validation, and empty-batch coverage never runs in CI and regressions in the new dispatch path go undetected.
Knowledge Base Used: Tests and QA
Greptile SummaryThe PR adds per-sequence attention policies for packed THD attention and exposes policy dispatch through DotProductAttention, MultiheadAttention, and TransformerLayer.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
TL[TransformerLayer] --> MHA[MultiheadAttention]
MHA --> DPA[DotProductAttention]
DPA --> V[Validate per-sequence policies]
V --> D{Dispatch mode}
D -->|grouped| G[Compact policy tokens]
D -->|auto| B{Backend supports padding representation?}
B -->|yes| P[Preserve packed storage and mask inactive lanes]
B -->|no| G
P --> C[Combine policy outputs]
G --> C
Reviews (18): Last reviewed commit: "Merge branch 'main' into desh/mixed-thd-..." | Re-trigger Greptile |
Signed-off-by: Desh Raj <r.desh26@gmail.com>
Signed-off-by: Desh Raj <r.desh26@gmail.com>
Replace the per-policy Q/K/V gather and output scatter dispatcher with the review-suggested inter-sequence-padding design. Keep the original packed Q/K/V storage, derive logical and physical cu-seqlens for each mask policy, run the ordinary scalar-mask DPA path with pad_between_seqs, and sum its disjoint zero-padded outputs. Expose a generic attn_mask_type_per_seq mapping plus window_size_per_mask_type so callers can combine full-context offline attention with bounded causal attention. Preserve the scalar fast path for uniform batches, avoid policy-dependent host synchronization, and cover forward/backward parity, existing physical padding, cross-attention, per-policy windows, bottom-right masks, invalid inputs, empty batches, and CUDA graph replay. NRT H100 validation passed all 16 TransformerEngine cases and 3 focused Megatron integration tests. In an 8-node/64-H100 matched-auto comparison over iterations 7-100, grouped averaged 1802.289 ms and pad-between averaged 1779.596 ms (-1.259%); the paired 95% interval crossed zero, indicating practical parity when the backend is held constant. In the deployable-backend comparison, grouped/Flash averaged 1596.129 ms while pad-between/auto-cuDNN averaged 1766.769 ms: pad-between was 10.691% slower by mean and 11.482% slower by 5%-trimmed mean. The paired penalty was +170.640 ms with a 95% interval of +89.728 to +251.552 ms, with all 16 fully logged workload counters matching across 100 steps. The implementation removes the original data-movement contention, but the current image cannot run full-context inter-sequence padding through FlashAttention 2 and has no FA3, so the path falls back to cuDNN fused attention. Retain grouped/Flash for production until a competitive padding-capable backend is available; this commit keeps the cleaner generic API for review and future backend support. Signed-off-by: Desh Raj <r.desh26@gmail.com>
faea818 to
a88c9b6
Compare
for more information, see https://pre-commit.ci
Mixed-mask dispatch represents sequences owned by other policies as inter-sequence padding. FA3 preserves that layout contract, but its padding lanes are unspecified: focused Hopper testing observed NaNs in inactive forward outputs and tile-spill garbage in inactive dQ/dK/dV lanes. Build a sync-free mask from the physical cu_seqlens, sanitize each policy output with torch.where, and wrap Q/K/V in an identity-forward autograd guard that zeros only inactive gradient lanes. This keeps the pad_between_seqs design, avoids regrouping or copying tokens, and lets disjoint policy results be summed safely across FA2 and FA3. Validation: NRT H100 source-overlay run passed all 16 focused mixed-THD tests for FP16/BF16 forward and backward, pre-existing padding, cross attention, per-policy windows, CUDA graphs, and validation errors. Signed-off-by: Desh Raj <r.desh26@gmail.com>
Keep attn_mask_type_per_seq and window_size_per_mask_type invariant at the caller boundary while selecting the physical implementation inside DotProductAttention. Use inter-sequence padding only on SM90 when FlashAttention 3 is installed and enabled. Fall back to per-policy token compaction, scalar THD attention, and output restoration for FlashAttention 2 and other configurations. Preserve the uniform-policy scalar fast path and restrict CUDA graph coverage to the sync-free FA3 padding path. Add a runtime dispatch matrix covering architecture, FA3 availability, and backend environment controls. Validation: - NRT H100/FA3: 21 mixed-THD tests and 4 Megatron policy/API tests passed - IAD A100/FA2: 20 mixed-THD tests passed, 1 Hopper-only graph test skipped, and 4 Megatron policy/API tests passed - Installed-image five-step training smokes were bit-identical within FA3 across NRT/HEL and within FA2 across IAD/ORD; maximum cross-backend loss delta was 1.56e-5
|
@cyanguwa @KshitijLakhani I made the changes following @cyanguwa's suggestions. The main issue is that the
|
|
Thanks for the pictorial summary :) @KshitijLakhani will help guide you through the rest of the PR. Regarding the benchmarks, should we not expect more savings from |
I previously ran this comparison on H100: |
KshitijLakhani
left a comment
There was a problem hiding this comment.
General comments:
-
The new argument/feature currently appears to be exposed directly through
DotProductAttention; I do not see corresponding plumbing throughMultiheadAttentionorTransformerLayer. Would be good if you could add that too - I'm guessing we wouldn't necessarily want to restrict users of this feature to theDPAAPI only. Ideally, would be good if you could just plumb it in this PR, however, if you are in a rush, I am happy to have this addressed in a separate PR. Please add a TODO so that we do not forget, in case you plan to address it separately -
I might have missed this but if this PR does not explicitly reject CP for this policy based approach, we should. Users should no try to use it with CP. A very small / lightweight CP rejection test would help add confidence to this rejection code.
| mask_type, | ||
| policy["window_size"], | ||
| ) | ||
| policy_bottom_right_diagonal = bottom_right_diagonal |
There was a problem hiding this comment.
bottom_right_diagonal is stored internally per policy, but looks like it is derived from the global argument and mask type and ussers cannot specify it independently per policy. Is my understanding right ?
Now, this may be intentional, particularly because the causal mask names encode much of the alignment behavior. Still, since mask and sliding window are now policy-specific, could you please confirm whether bottom-right alignment can ever legitimately vary between policies. If so, it belongs in the policy object too.
There was a problem hiding this comment.
For our use cases right now, we don't need policy-specific variation in bottom_right_diagonal, but I can imagine in some cross-attention/local attention use cases it may be useful. I will include it as per-policy field in thd_attention_policies.
| interleave sits; must be -3 (e.g. ``bs3hd``) or -2 (e.g. ``bsh3d``, | ||
| Megatron-style). This is an explicit knob rather than shape inference, | ||
| since e.g. ``h == 3`` would make the shapes ambiguous. | ||
| attn_mask_type_and_window_size_per_seq_policies: Optional[List[Dict[str, Any]]], |
There was a problem hiding this comment.
Choose a more generic name like : thd_attention_policies or earlier suggested attn_policies_per_seq so that it is not perceived as a policy for swa and attn masks only
| are supplied as a one-dimensional, strictly ascending CUDA integer tensor; | ||
| all policy tensors must be disjoint and together contain every sequence | ||
| exactly once. A list may contain multiple policies with the same mask type |
There was a problem hiding this comment.
Are we validating/verifying these mentioned conditions in the comments anywhere in code?:
- Strictly ascending IDs within a policy.
- Duplicates within one policy.
There was a problem hiding this comment.
Since policy metadata is expected to be static, could we validate the sequence IDs once before CUDA graph capture and then treat them as immutable?
I'm thinking that for each policy, we should check that IDs are strictly ascending, and the sorted concatenation of all policy IDs should equal arange(batch_size). Now the issues is that these checks will synchronize when their result is converted to a Python boolean IIUC, but that should be acceptable as a one-time preparation/warm-up cost. They should not execute inside the captured forward path. A small validation step (or cached validation before capture though need to think a bit through this one's detils) may be the cleanest approach.
| attention_params_kwargs, | ||
| ) | ||
| padded_output = None | ||
| if padded_policies: |
There was a problem hiding this comment.
The routing currently treats padded-backend availability as a preference: if FA3 or Fused Attention can run the policy with pad_between_seqs=True, padded execution is always selected. This makes sense and I agree with it as an initial policy, but get_attention_backend() only determines backend eligibility for that representation; it does not compare its end-to-end performance against compacting and using FA2/FA4.
It is more of a what is supported call rather than which is more performant call.
So, I'm wondering, to give the users of this feature better guidance, could you please document that mixed THD dispatch is intentionally padded-first .
Users can however, indirectly force grouped execution by disabling FA3 and Fused Attention while leaving FA2/FA4 enabled, but those environment variables affect all attention calls. This is documentation/comment should give enough guidance to the user to maybe experiment the different routes padded and grouped, (assuming they have them available) and then if they find that our approach of choosing padded is less performant for their case they can force TE to use grouped via the env vars.
Note for the future: I have not done any benchamarking on this but @desh2608 if you do any benchmarking and we learn that grouped remains materially faster for important workloads, it may be worth providing a direct padded/grouped preference or override rather than relying solely on global env var backend controls.
There was a problem hiding this comment.
I had benchmarked both in my training setup, and the iteration times are as follows:
| Method | Attn. backend | Iteration time (ms) |
|---|---|---|
| Grouped | FA2 | 1669.9 |
| Grouped | FA3 | 1548.2 |
| Padded | FA3 | 1635.7 |
Given that grouped + FA3 seems faster, I will add an optional override so users can select this implementation.
| bottom_right_diagonal=policy["bottom_right_diagonal"], | ||
| pad_between_seqs=True, | ||
| ) | ||
| *_, available_backends = dpa_utils.get_attention_backend(padded_attention_params) |
There was a problem hiding this comment.
So seems like for the attention params, we have an uncache probe here, which is unlike what we do otherwise in DPA (see lines 2535 in this file)
It looks something like this:
if (
_attention_backends["attention_params"] is None
or attention_params != _attention_backends["attention_params"]
):
_partition_thd_mask_policies() calls get_attention_backend() directly for every policy at line 1359, bypassing the normal _attention_backends cache used by the usual forward path. The subsequent recursive self.forward() can then perform backend selection again when it executes that policy. Was this repeated selection intentional @desh2608 ?
I'm concerned this maybe fine initially, but could you measure the CPU overhead for the latest implementation ? If it is substantial/materical engouth , perhaps the policy probe results could be cached by AttentionParams or reused during execution ? Please evaluate this
There was a problem hiding this comment.
No this was an oversight. Fixed in 0ced9fc. The CPU overhead was ~200us per call.
| @@ -0,0 +1,582 @@ | |||
| # Copyright (c) 2022-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
There was a problem hiding this comment.
Could the new tests reuse more of the existing attention test infrastructur? For e.g. DPA/config construction, THD input generation, backend forcing and comparison helpers?
I am less conflicted about whether the tests remain in a dedicated file or we find an easy/clean way to integrate into the test_attention file instead, however, since the feature now has enough cases to justify a separate file, I'm happy to consider that possibility. My main maintenance concern is avoiding duplicated
setup/reference machinery. Trying to focus on mainainability.
While you perform this restructure, please weight the possibility of just having this as a separate function/class in the fused attention test file itself if it relatively clean to do so, else stick wih a separate file and only address the re-use comment i have bove
There was a problem hiding this comment.
Updated to reuse the existing infrastructure in 9f01ef8. I kept the new tests in a separate file since there are many cases.
Signed-off-by: Desh Raj <r.desh26@gmail.com>
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Added in 2dca008 |
3952732 to
0ced9fc
Compare
Added plumbing in 156069f |
|
|
||
|
|
||
| def _run_dot_product_attention( | ||
| def make_cu_seqlens(seqlens: torch.Tensor) -> torch.Tensor: |
There was a problem hiding this comment.
I extracted these into public methods so they can be re-used in test_mixed_thd_attention.py.
| make_dot_product_attention, | ||
| reset_rng_states, | ||
| run_dot_product_attention, | ||
| ) |
There was a problem hiding this comment.
We are now re-using the same setup as in test_attention.py. This file only contains cases specific to mixed attention.
Signed-off-by: Desh Raj <r.desh26@gmail.com>
|
/te-ci L0 L1 L2 |
Signed-off-by: Desh Raj <r.desh26@gmail.com>
…inimal Signed-off-by: Desh Raj <r.desh26@gmail.com>
|
@KshitijLakhani FYI one of the tests was failing on H100 due to a backend selection bug which is now fixed in 98f375f. |
| if ( | ||
| use_flash_attention | ||
| and (window_size[0] != -1 or window_size[1] not in [-1, 0]) | ||
| and not bottom_right_diagonal | ||
| and max_seqlen_q != max_seqlen_kv | ||
| ): | ||
| logger.debug( | ||
| "Disabling FlashAttention as it only supports sliding window with bottom right" | ||
| " diagonal alignment for cross-attention" | ||
| ) | ||
| use_flash_attention = False |
There was a problem hiding this comment.
This fix follows the existing convention of using max_seqlen_q != max_seqlen_kv for cross-attention-like cases. For variable-length THD, can individual Q/KV lengths differ while their global maxima are equal? @cyanguwa
If so, the selector cannot distinguish that case today. I believe that is a broader existing limitation rather than something this PR needs to solve, but it may be worth a follow-up test or issue.
Not blocking the PR on this for now
|
/te-ci L0 L1 L2 |
KshitijLakhani
left a comment
There was a problem hiding this comment.
LGTM !
Thanks for implementing this feature, Desh and taking it to the finish line :)
…radient The Lint job fails on this branch with dot_product_attention.py:243:4: C0116: Missing function or method docstring dot_product_attention.py:248:4: C0116: Missing function or method docstring _IdentityWithMaskedGradient came from NVIDIA#3274 and is byte-identical to main; the same two errors fail Lint on that PR's own branch (desh/mixed-thd-pr-minimal), which merged before the check went green, so this branch inherited a red Lint by merging main. Not introduced here, but it blocks this PR. Use the same `# pylint: disable=missing-function-docstring` the other autograd Functions in this package use (e.g. context_parallel.py) rather than inventing docstrings. Drop this commit if NVIDIA#3274 is fixed upstream first. Signed-off-by: Nitin Vegesna <nvegesna@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>


Description
Add per-sequence attention policies to packed THD attention.
A scalar
attn_mask_type,window_size, andbottom_right_diagonalapply one policy to an entire packed invocation. Mixed offline/causal training instead needs individual packed sequences to use different policies while preserving a single caller-facing API.The new
thd_attention_policiesAPI accepts an ordered list of complete policies:Every policy must contain
sequence_ids,mask_type, andwindow_size.bottom_right_diagonalis optional and falls back to the forward-level or module-level setting. Mask types with explicit causal alignment retain their existing semantics:padding_causalforces top-left alignment andpadding_causal_bottom_rightforces bottom-right alignment.Sequence IDs are one-dimensional, strictly ascending CUDA
int32orint64tensors. The policy tensors must be disjoint and together cover every packed sequence exactly once. Repeated mask types are allowed, so separate sequence subsets can use the same mask with different windows or diagonal alignment. Policy IDs are validated once before CUDA graph capture and then treated as immutable.The mixed-policy argument is mutually exclusive with scalar
attn_mask_typeandwindow_sizearguments.API accessibility
Both mixed-policy options are exposed at every relevant PyTorch layer:
thd_attention_policiesthd_attention_policy_dispatchCallers using
TransformerLayerorMultiheadAttentiontherefore have the same automatic/grouped control as direct DPA callers.Runtime dispatch
thd_attention_policy_dispatch="auto"is intentionally padded-first. For each nonempty policy, Transformer Engine queries its ordinary backend selector with the complete runtimeAttentionParamsandpad_between_seqs=True:thd_attention_policy_dispatch="grouped"explicitly compacts every policy and bypasses padded-backend probing. The override also applies to a uniform one-policy batch; the scalar fast path does not bypass the requested dispatch.Inter-sequence-padding representation
The padding path:
pad_between_seqs=True.Grouped-compaction representation
The grouped path:
pad_between_seqs=False.Backend-selection caching
Mixed policies alternate among a small set of
AttentionParams. Their complete backend-selection results are stored in a bounded cache and reused by the recursive scalar attention calls. The cache accounts for backend-control environment state and is invalidated when ordinary DPA backend selection is marked for refresh.In a controlled same-node H100 microbenchmark, the steady-state selector calls per mixed forward changed from 4 to 0 for automatic dispatch and from 2 to 0 for grouped dispatch. The directly measured selector CPU time removed was 242.0 us/forward and 169.6 us/forward, respectively. Total host submission time decreased by 170.6 us (8.1%) for automatic dispatch and 109.5 us (5.3%) for grouped dispatch. These are DPA microbenchmark results, not full-model throughput estimates.
Supported scope
The new API supports:
MultiheadAttention, andTransformerLayercallers.It is currently restricted to padding mask types and does not support:
Type of change
Validation
Focused tests cover:
The A100 skip is the Hopper/FA3-only CUDA graph case. Python compilation, the repository's Black configuration, and whitespace checks pass for all changed files.
End-to-end 4B ASR profile
A dedicated one-node/eight-GPU profile ran the 4B/A770M model on ASR-only data for 100 optimizer steps per mode. All-causal, all-full-attention, and 50%-causal modes ran sequentially in the same exclusive allocation on each architecture. The table reports the steady-state mean over iterations 7-100 (
n=94).All-causal and all-full attention are within 1% end to end on both systems. The heterogeneous path is slower because policy grouping and multiple backend invocations are paid in each audio layer. Since A100 and H100 used different cluster-local containers, the cross-architecture ratio should not be interpreted as a pure silicon comparison.
Checklist
The focused GPU suites, backend-selection tests, end-to-end 4B profiles, Python compilation, formatting, and whitespace checks pass as described above.