Fix flash/sage varlen prep under torch.compile with dynamic shapes - #14568
ShivamShrirao wants to merge 10 commits into
Conversation
|
Hi @ShivamShrirao, thanks for the PR! It does not appear to link an issue it fixes. If this PR addresses an existing issue, please add a closing keyword (e.g. Please note that PRs without a linked issue are likely to be automatically closed 10 days after this notice. Once the PR links an issue (or gets the |
|
Can you provide a minimal reproducer with the diffusers functions included? |
|
Reproducer with diffusers varlen backends call: import torch
from diffusers.models.attention_dispatch import _prepare_for_flash_attn_or_sage_varlen
@torch.compile(dynamic=True, fullgraph=True)
def prep(hidden_states):
batch_size, seq_len, _ = hidden_states.shape
return _prepare_for_flash_attn_or_sage_varlen(
batch_size, seq_len, seq_len, attn_mask=None, device=hidden_states.device
)
prep(torch.randn(2, 77, 64, device="cuda"))On main: torch._inductor.exc.InductorError: TypeError: unsupported operand type(s) for *: 'FakeTensor' and 'Node'With the fix: ((tensor([77, 77], device='cuda:0', dtype=torch.int32),
tensor([77, 77], device='cuda:0', dtype=torch.int32)),
(tensor([ 0, 77, 154], device='cuda:0', dtype=torch.int32),
tensor([ 0, 77, 154], device='cuda:0', dtype=torch.int32)),
(77, 77))Tested with torch 2.12.1 and 2.13.0+cu129. |
|
Sorry, but this is bit vague to determine which attention backend is causing this, though. |
There was a problem hiding this comment.
🤗 Serge says:
Solid, well-scoped fix — the arange construction is numerically identical to cumsum(full(...)) for uniform lengths (0, s, 2s, …, batch_size*s, batch_size+1 elements, same int32 dtype), and returning seq_len_q / seq_len_kv matches what seqlens.max().item() produced while removing the sync/graph break.
Correctness
- All five call sites were checked: the masked callers (
_flash_attn_varlen_*,_flash_varlen_attention_hub) already discard the helper'smax_seqlen_qor recompute it asseq_len_q, so the returned values are unchanged in every path.lru_cache_unless_exporton the no-mask helper is unaffected since the new returns derive purely from the cache-key arguments. - Only nit:
torch.arange(..., step=seq_len_*)raises on a zero step, whereas the oldcumsumpath degenerated gracefully to all-zeros for a zero-length sequence. Almost certainly unreachable in practice, but it is a behavioral difference worth being aware of.
Tests
- The verification described (bit-identical outputs, compile with
dynamic=True, fullgraph=True) is not reflected in the repo — no test exercises these helpers directly. A small unit test comparing the newcu_seqlensagainst the cumsum reference for a few(batch_size, seq_len)combos would lock this in cheaply, since the helpers are shared by every flash/sage varlen backend.
Minor
- In the no-mask helper,
seqlens_q/seqlens_kare still materialized viatorch.fullbut every caller discards them (the unmasked branches immediately setseqlens_k = None). Out of scope for this PR, but they are dead allocations on the hot path if you ever want to trim the return tuple.
serge v0.1.0 · model: claude-opus-5 · 6 LLM turns · 9 tool calls · 68.4s · 93703 in / 5078 out tokens
| # Built with arange instead of cumsum(full(...)): inductor rewrites that pattern into | ||
| # `arange * fill_value`, which raises under dynamic shapes because the fill value is a | ||
| # symbolic sequence length. The lengths are uniform here, so arange is also cheaper. | ||
| cu_seqlens_q = torch.arange(0, (batch_size + 1) * seq_len_q, seq_len_q, dtype=torch.int32, device=device) |
There was a problem hiding this comment.
Behavioral edge case worth noting: torch.arange with step=0 raises RuntimeError: step must be nonzero, so a zero-length query/key sequence now errors where the previous cumsum(zeros) path returned an all-zero cu_seqlens. I don't think any dispatcher path can reach here with seq_len == 0, so this is informational rather than blocking — just flagging it since the PR claims strict numerical/behavioral equivalence.
There was a problem hiding this comment.
@sayakpaul Thanks, fixed in 0ac4fbf. The sequence length now scales a unit-step arange instead of being the step itself, so seq_len == 0 returns all-zero offsets like the cumsum path it replaced, instead of raising. Verified it still matches cumsum(full(...)) across batch/seq combinations and still compiles under dynamic=True.
|
The helper is shared by every varlen backend, so it's all of them rather than one:
All of them go through |
|
Hi @sayakpaul , any update on this ? How can I help to get this merged ? |
|
Hi @sayakpaul bumping this up! |
10bcd5c to
0ac4fbf
Compare
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
sayakpaul
left a comment
There was a problem hiding this comment.
Thanks! Sorry about the delay. Can we also add some lightweight tests, keeping the convention similar to tests/models/testing_utils/attention.py?
| offsets = torch.arange(0, batch_size + 1, dtype=torch.int32, device=device) | ||
| cu_seqlens_q = offsets * seq_len_q | ||
| cu_seqlens_k = offsets * seq_len_kv | ||
| return (seqlens_q, seqlens_k), (cu_seqlens_q, cu_seqlens_k), (seq_len_q, seq_len_kv) |
There was a problem hiding this comment.
Why are we changing the return signature? The caller can reuse the seq_len_q and seq_len_kv right?
There was a problem hiding this comment.
The shape is unchanged, still three pairs. The third pair was seqlens.max().item(), which for uniform lengths is the input by construction, so this just skips a device sync per call. It also matters for compile: full((b,), n, dtype=int32).max() with a symbolic n fails inductor codegen under dynamic=True on torch 2.12 (int32/int64 loop-carried type mismatch in the reduction kernel).
|
No worries! Thanks for getting back. I have added |
| with ( | ||
| torch._inductor.utils.fresh_inductor_cache(), | ||
| torch._dynamo.config.patch(error_on_recompile=True), | ||
| ): | ||
| for seq_len in (128, 256, 256): | ||
| q, k, v = make_qkv(seq_len) | ||
| torch.testing.assert_close( | ||
| compiled_attention(q, k, v), dispatch_attention_fn(q, k, v), atol=1e-3, rtol=1e-3 | ||
| ) |
| def test_zero_length_sequence(self): | ||
| _, (cu_seqlens_q, cu_seqlens_k), max_seqlens = _prepare_for_flash_attn_or_sage_varlen( | ||
| 2, 0, 0, device=torch_device | ||
| ) | ||
| assert cu_seqlens_q.tolist() == cu_seqlens_k.tolist() == [0, 0, 0] | ||
| assert max_seqlens == (0, 0) |
There was a problem hiding this comment.
We can remove this test because it tests a private function.
There was a problem hiding this comment.
Removed, thanks!
What does this PR do?
Fixes #11957.
That report is the
.item()graph break on the no-mask path (Flux passes no attention mask), whichchange 2 below removes entirely — the no-mask helper no longer calls
.item()at all. #11970 was anearlier attempt at the same issue and was closed unmerged. Masked varlen still keeps one necessary
.item()for the data-dependent key lengths, as noted after the two changes.Makes the flash-attn / sage varlen attention backends usable under
torch.compilewith dynamic shapes. Two changes in the_prepare_for_flash_attn_or_sage_varlen_*helpers, with no numerical change:cu_seqlensis now built withtorch.arangeinstead ofcumsum(full(...)). Inductor rewrites the cumsum-of-a-constant pattern internally, and that rewrite fails when the fill value is a symbolic sequence length. The lengths are uniform here (every sequence in the batch has the same length), sotorch.arange(0, (batch_size + 1) * seq_len, seq_len)produces identical offsets directly — and is cheaper.Minimal repro (fails on torch 2.6.0 and 2.12.1):
max_seqlen_q/max_seqlen_kare returned as the already-known Python int instead ofseqlens.max().item(). The.item()forces a GPU→CPU sync and a graph break; the max of uniform lengths is justseq_lenitself, which the caller already has as an int. Same type as before (.item()also returned an int), so all call sites are unaffected.In the masked helper only the query side changes: key lengths are data-dependent (
attn_mask.sum(dim=1)), so their cumsum does not hit the inductor rewrite and their.item()is inherent to varlen with padding.Found while enabling flash-attn varlen +
torch.compilefor the Bria Fibo pipelines, but the helpers are shared by every model using these backends.Verification
flash_attn_varlen_func(flash-attn 2.8.3, H200) compiles withtorch.compile(dynamic=True, fullgraph=True)across changing batch/sequence shapes and matches eager output exactly (max diff 0.0).Before submitting
self-reviewskill on the diff?Self-review notes
Two-function diff reviewed against
.ai/review-rules.md. Verdict: READY. No API change: same return tuple structure, same values, and the max-seqlen entries keep the type.item()produced (Python int). Checked all 10 call sites inattention_dispatch.py, including the hub-kernel forward/backward ops added recently — the masked-path callers already discard the helper'smax_seqlen_qand useseq_len_qlocally, consistent with this change; unmasked-path callers consume the returned ints unchanged (ctxscalar attributes, neversave_for_backward). Equivalence verified numerically against the previous cumsum construction, masked and unmasked. Left for the reviewer: no CPU unit test covers these helpers directly; the behavior is exercised through the GPU flash-attn backend tests.Who can review?
@sayakpaul @DN6