Repository navigation
Commit d5e4ae9
Give causal attention the mask PyTorch asks for (pytorch#22443)
Fixes pytorch#22426.
### The problem
Attention with `is_causal=True` returns wrong values, with nothing
raised, whenever the query is shorter than the keys:
| shape | error against eager |
| --- | --- |
| query 1, keys 16 | **2.7 to 3.4** |
| query 16, keys 16 | 3.6e-07 |
The two libraries anchor a causal mask at opposite corners. PyTorch puts
it at the top left, so the query at row `i` attends to keys `0..i`. MLX
puts it at the bottom right, aligning the last query with the last key.
They agree only when the lengths are equal, and nothing checked that.
This is the decode step of any model that generates a token at a time,
so it is not a corner case.
### The fix
Slice the keys and values to the query length when the lengths are not
provably equal, then let the kernel apply its own mask to what is now a
square problem.
That is what PyTorch computes: a row past the last query never reads a
later key, so dropping those keys changes nothing. Measured over query
lengths 1 to 63 against key lengths 2 to 512, the sliced form and the
original agree **exactly** in float32. A slice before attention is also
what the cache-aware path in this file already does, so no mask tensor
is built and the fused kernel is kept.
A causal query **longer** than its keys is left to decompose instead.
PyTorch clamps each row to the keys that exist, and neither the kernel's
own mask nor a slice reproduces that.
### Why the speech example changes too
This backend already knew about the conflict. `mlx::custom_sdpa` exists
for cached attention and takes a start position so the kernel gets a
square problem, and seven models here use it.
The speech example does not. It slices the cache itself and then asks
for a causal mask over the result, which in PyTorch means **the new
token sees only the first cached key**. It has been getting attention
over the whole cache because the kernel read its request the other way
round, so it depends on the behaviour this change corrects. Moving it
onto the same path as the other models makes it ask for what it wants.
Worth stating plainly: that example's own eager math was already 3.3
away from what it intended at a decode step, so this is not a regression
being introduced, it is one being removed.
### Test plan
Two cases added to the operator suite, a query of 1 and of 6 against 32
keys, which run the compiled runtime and compare against eager. **Both
fail before this change on numerics and pass after.**
Ran the shapes on an Apple Silicon Mac against eager:
- query 1, 2 and 6 against longer keys, float32 and bfloat16: now exact
or within 4.2e-07, previously off by around 3
- equal lengths and non-causal calls untouched, fused kernel kept in
every case, no mask tensor built
- a causal query longer than its keys decomposes and matches eager to
3.6e-07, previously off by 2.7
- equal-length causal attention serializes to a **byte-identical
program** before and after, so that path is bit-for-bit unchanged
Exported the speech model and compared its logits against HuggingFace's
own decoder at a cached decode step: unchanged from before this change,
and the top token still matches.
Ran the neighbouring backend test files, all passing.
Co-authored-by: PyTorch Bot <pytorchbot@users.noreply.github.com>1 parent 4747ab7 commit d5e4ae9
3 files changed
Lines changed: 55 additions & 10 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
122 | 122 | | |
123 | 123 | | |
124 | 124 | | |
125 | | - | |
126 | | - | |
127 | | - | |
128 | | - | |
129 | | - | |
130 | | - | |
131 | | - | |
132 | | - | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
133 | 136 | | |
134 | 137 | | |
135 | 138 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
583 | 583 | | |
584 | 584 | | |
585 | 585 | | |
| 586 | + | |
| 587 | + | |
| 588 | + | |
| 589 | + | |
| 590 | + | |
| 591 | + | |
| 592 | + | |
| 593 | + | |
586 | 594 | | |
587 | 595 | | |
588 | 596 | | |
| |||
704 | 712 | | |
705 | 713 | | |
706 | 714 | | |
| 715 | + | |
| 716 | + | |
| 717 | + | |
| 718 | + | |
| 719 | + | |
| 720 | + | |
| 721 | + | |
| 722 | + | |
| 723 | + | |
| 724 | + | |
| 725 | + | |
| 726 | + | |
| 727 | + | |
| 728 | + | |
| 729 | + | |
| 730 | + | |
| 731 | + | |
| 732 | + | |
| 733 | + | |
| 734 | + | |
| 735 | + | |
| 736 | + | |
| 737 | + | |
| 738 | + | |
| 739 | + | |
707 | 740 | | |
708 | 741 | | |
709 | 742 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
6212 | 6212 | | |
6213 | 6213 | | |
6214 | 6214 | | |
| 6215 | + | |
6215 | 6216 | | |
6216 | 6217 | | |
6217 | 6218 | | |
| |||
6221 | 6222 | | |
6222 | 6223 | | |
6223 | 6224 | | |
| 6225 | + | |
6224 | 6226 | | |
6225 | 6227 | | |
6226 | 6228 | | |
6227 | 6229 | | |
6228 | 6230 | | |
6229 | 6231 | | |
| 6232 | + | |
| 6233 | + | |
6230 | 6234 | | |
6231 | 6235 | | |
6232 | 6236 | | |
| |||
6241 | 6245 | | |
6242 | 6246 | | |
6243 | 6247 | | |
| 6248 | + | |
| 6249 | + | |
| 6250 | + | |
| 6251 | + | |
| 6252 | + | |
6244 | 6253 | | |
6245 | 6254 | | |
6246 | 6255 | | |
| |||
6256 | 6265 | | |
6257 | 6266 | | |
6258 | 6267 | | |
6259 | | - | |
6260 | | - | |
| 6268 | + | |
| 6269 | + | |
6261 | 6270 | | |
6262 | 6271 | | |
6263 | 6272 | | |
| |||
0 commit comments