🚨 Enable SDPA (and other attention backends) for T5 and propagate to the T5 family - #47014
Conversation
Signed-off-by: jiqing-feng <jiqing.feng@intel.com>
Signed-off-by: jiqing-feng <jiqing.feng@intel.com>
|
Attention dispatch so cc @ArthurZucker @Cyrilvallez |
|
WIll check it out when I have time but it's a big PR so expect delays 🙏 |
|
This competes with #46946 |
vasqu
left a comment
There was a problem hiding this comment.
Only checked t5 becuse everything else is dependent on that (so the same comments apply), after t5 is solid I will make a scan through all models
| causal_mask = mask[:, :, :, : key_states.shape[-2]] | ||
| if causal_mask.dtype == torch.bool: | ||
| # `sdpa` may materialize a boolean mask (True = keep). Turn it into an additive float mask so it | ||
| # can be folded into the relative position bias, just like the `eager` float mask. | ||
| causal_mask = torch.where( | ||
| causal_mask, | ||
| torch.tensor(0.0, device=causal_mask.device, dtype=position_bias.dtype), | ||
| torch.finfo(position_bias.dtype).min, | ||
| ) |
There was a problem hiding this comment.
| causal_mask = mask[:, :, :, : key_states.shape[-2]] | |
| if causal_mask.dtype == torch.bool: | |
| # `sdpa` may materialize a boolean mask (True = keep). Turn it into an additive float mask so it | |
| # can be folded into the relative position bias, just like the `eager` float mask. | |
| causal_mask = torch.where( | |
| causal_mask, | |
| torch.tensor(0.0, device=causal_mask.device, dtype=position_bias.dtype), | |
| torch.finfo(position_bias.dtype).min, | |
| ) | |
| if causal_mask.dtype == torch.bool: | |
| # `sdpa` may materialize a boolean mask (True = keep). Turn it into an additive float mask so it | |
| # can be folded into the relative position bias, just like the `eager` float mask. | |
| causal_mask = torch.where( | |
| causal_mask, | |
| torch.tensor(0.0, device=causal_mask.device, dtype=position_bias.dtype), | |
| torch.finfo(position_bias.dtype).min, | |
| ) |
- I don't think we need to slice anymore (might be wrong)
- I think we should do the bool -> float conversion at mask creation time tbh
There was a problem hiding this comment.
(1) Dropped the slice for t5/umt5 (no-op there since kv_len already matches key_states.shape[-2]); kept it on pix2struct because its text stack still builds the mask the old way and removing it breaks test_eager_matches_sdpa_inference — will remove it with the pix2struct refactor follow-up. (2) Doing the bool→float conversion in masking_utils would hit every model and other backends rely on the boolean mask (sdpa's is_causal fast path). Did you mean a T5-specific mask_function that returns the additive bias directly? Happy to do that if there's an existing pattern.
There was a problem hiding this comment.
No no, I dont mean in the mask function itself but when we create in T5 so in the stack / model modules after creation, do the conversion directly there.
There was a problem hiding this comment.
With the current approach there's no bool→float conversion needed anymore — since position_bias is passed to the attention interface separately instead of being folded into the mask, we don't need an additive float mask. eager just gets the float mask and sdpa gets the bool mask, each handled natively by its backend. So there's nothing to convert after mask creation.
| # T5 uses a relative attention bias that is added to the attention scores. This is passed as the additive | ||
| # attention mask so that it works with the different attention implementations. As it is always non-`None`, | ||
| # `is_causal` is never inferred, so the causal behavior is fully encoded in the bias itself. | ||
| attention_interface: Callable = eager_attention_forward | ||
| if self.config._attn_implementation != "eager": | ||
| if self.config._attn_implementation != "sdpa": | ||
| raise ValueError( | ||
| "T5 adds a relative position bias on top of the attention scores, which is only supported by the " | ||
| f"`eager` and `sdpa` attention implementations, but got `{self.config._attn_implementation}`." | ||
| ) | ||
| attention_interface = ALL_ATTENTION_FUNCTIONS[self.config._attn_implementation] |
There was a problem hiding this comment.
| # T5 uses a relative attention bias that is added to the attention scores. This is passed as the additive | |
| # attention mask so that it works with the different attention implementations. As it is always non-`None`, | |
| # `is_causal` is never inferred, so the causal behavior is fully encoded in the bias itself. | |
| attention_interface: Callable = eager_attention_forward | |
| if self.config._attn_implementation != "eager": | |
| if self.config._attn_implementation != "sdpa": | |
| raise ValueError( | |
| "T5 adds a relative position bias on top of the attention scores, which is only supported by the " | |
| f"`eager` and `sdpa` attention implementations, but got `{self.config._attn_implementation}`." | |
| ) | |
| attention_interface = ALL_ATTENTION_FUNCTIONS[self.config._attn_implementation] | |
| attention_interface: Callable = ALL_ATTENTION_FUNCTIONS.get_interface( | |
| self.config._attn_implementation, eager_attention_forward | |
| ) |
Imo
- This was the old format
- The flags will already indicate what is supported so we can just go ahead
There was a problem hiding this comment.
Done, applied to umt5 and pix2struct too.
| _no_split_modules = ["T5Block"] | ||
| _keep_in_fp32_modules = ["wo"] | ||
|
|
||
| _supports_attention_backend = True |
There was a problem hiding this comment.
| _supports_attention_backend = True |
would avoid this for now, enc-dec are more complicated
There was a problem hiding this comment.
Removed for t5, umt5 and pix2struct. We can revisit once enc-dec is settled.
| _supports_flash_attn = False | ||
| _supports_flex_attn = False |
There was a problem hiding this comment.
We can add a small comment here as to why == relative bias is not possible
Altho in the future we might be able to use FA4 to integrate the score mod along
There was a problem hiding this comment.
Added a comment: T5 folds the relative position bias into the additive mask, which flash can't take and flex would need a dedicated score_mod for. (Noted the FA4 / score_mod possibility, cc @zucchini-nlp below.)
There was a problem hiding this comment.
I think based on @zucchini-nlp we could actually change the strategy a bit:
- Do not add mask and bias directly
- Pass the bias and mask separately
- Do not convert your mask manually anymore
That would also enable flex as side effect
There was a problem hiding this comment.
Done — reworked exactly along those lines: mask and bias are no longer added together, position_bias is passed to the attention interface as a separate kwarg, and all the manual mask conversion is gone. eager and sdpa each handle the bias and mask natively, and flex is now enabled as a side effect. Applied across the whole T5 family.
| **kwargs, | ||
| **kwargs: Unpack[TransformersKwargs], | ||
| ): | ||
| use_cache = use_cache if use_cache is not None else self.config.use_cache |
There was a problem hiding this comment.
| use_cache = use_cache if use_cache is not None else self.config.use_cache |
already handled by decorator
There was a problem hiding this comment.
Removed, the decorator already handles it.
| @@ -668,8 +714,6 @@ | |||
| raise ValueError("You have to initialize the model with valid token embeddings") | |||
| inputs_embeds = self.embed_tokens(input_ids) | |||
There was a problem hiding this comment.
Let's simplify here
- The gradient ckpting is handled by the decorator
- The input ids and embeds checks can be simplified by a lot (see other encoder-decoder models like bart)
- The value error on inputs embeds that embed tokens is overkill we can skip that if completely
There was a problem hiding this comment.
Simplified as suggested, same for umt5. pix2struct's text stack still uses the old decorator style, so I'll migrate it in a follow-up to keep this PR focused on the SDPA enablement.
| and_mask_function=dummy_and_mask_function, | ||
| ) | ||
| else: | ||
| attention_mask = create_bidirectional_mask( |
There was a problem hiding this comment.
relative bias only on decoder side?
There was a problem hiding this comment.
Relative bias is on both encoder and decoder. The dummy_and_mask_function is only needed on the decoder's create_causal_mask to force the mask to be materialized as a float so the bias can be folded in (otherwise sdpa may take the is_causal shortcut). The encoder uses create_bidirectional_mask which is already additive.
There was a problem hiding this comment.
Based on #47014 (comment) and the model addition of inkling, we also dont need this workaround anymore
There was a problem hiding this comment.
Right — dropped that workaround. Now that the bias is passed separately (like inkling does), there's nothing left to work around here.
|
|
||
| encoder_extended_attention_mask = None | ||
| if self.is_decoder and encoder_hidden_states is not None: | ||
| encoder_extended_attention_mask = create_bidirectional_mask( |
There was a problem hiding this comment.
Cross-attention has no relative bias (has_relative_attention_bias=False), so no dummy mask function is needed here.
| @@ -881,19 +892,16 @@ | |||
| >>> last_hidden_states = outputs.last_hidden_state | |||
| ```""" | |||
| use_cache = use_cache if use_cache is not None else self.config.use_cache | |||
There was a problem hiding this comment.
| use_cache = use_cache if use_cache is not None else self.config.use_cache |
| @@ -1049,7 +1050,6 @@ | |||
| >>> # studies have shown that owning a dog is good for you. | |||
| ```""" | |||
| use_cache = use_cache if use_cache is not None else self.config.use_cache | |||
There was a problem hiding this comment.
Removed here too, and in T5ForQuestionAnswering for consistency. Thanks for the nudge.
|
FWIW, we might take advantage of recent updates to accommodate Inkling, and support FlexAttn with position bias. I didn't check in details, might be wrong if these two transformers/src/transformers/models/inkling/modeling_inkling.py Lines 602 to 609 in 290d5c4 |
|
You could only do this with flex attention using score mod afaik, but flex attention with score mod was quite slow; things might have changed by then but their mask creation is the biggest blocker to make flex attention viable. Edit: This might be ok when triggered via FA4 but still fear the mask creation tbh |
|
Ok yea checked, we added it to the score mod. We can do it for sure, just doubting the perf a bit |
Signed-off-by: jiqing-feng <jiqing.feng@intel.com>
|
Great pointer. Given the perf concern around flex mask creation, I'd keep this PR focused on sdpa and add flex support (via score_mod) as a follow-up once we can benchmark it. Works for you both? |
Signed-off-by: jiqing-feng <jiqing.feng@intel.com>
|
@jiqing-feng I haven't taken a look at inkling too deeply before this but I noticed it reduces a lot of friction if we follow that process, see #47014 (comment) and #47014 (comment) The tl;dr:
|
Signed-off-by: jiqing-feng <jiqing.feng@intel.com>
Following the inkling approach did remove a lot of friction: no more mask workarounds (no forced creation, no float conversion), and the position bias is now passed to the interface as a separate kwarg alongside the mask. Flex attention falls out as a side effect too. Reworked t5 along these lines and propagated it across the whole T5 family. |
vasqu
left a comment
There was a problem hiding this comment.
Smaller comments overall, only bigger thing I noticed was that fullgraph was set to false now which would be nice to solve instead 👀
There was a problem hiding this comment.
And ig the same here re output recorder. Just fmi why it wasnt refactored
There was a problem hiding this comment.
It's orthogonal to the attention-backend work and a fairly heavy refactor, so I'd do it in a dedicated follow-up. It's more than the vision tower — the whole model (text decoder included) still uses the legacy output_attentions/return_dict path with no _can_record_outputs, and being a two-tower model it means reworking three forwards and registering recorders for both towers. Happy to fold it in if you'd rather.
Signed-off-by: jiqing-feng <jiqing.feng@intel.com>
Signed-off-by: jiqing-feng <jiqing.feng@intel.com>
CI recapDashboard: View test results in Grafana |
|
Hi @vasqu . I've fixed your last comment, please rerun the tests. Thanks! |
|
run-slow: blip_2, longt5, mt5, pix2struct, pop2piano, switch_transformers, t5, udop, umt5 |
|
This comment contains models: ["models/blip_2", "models/longt5", "models/mt5", "models/pix2struct", "models/pop2piano", "models/switch_transformers", "models/t5", "models/udop", "models/umt5"] |
|
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. |
CI ResultsCommit Info
Model CI Report❌ 2 new failed tests from this PR 😭
|
|
@jiqing-feng , pls check CI, thx |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: blip_2, longt5, mt5, pix2struct, pop2piano, switch_transformers, t5, udop, umt5 |
|
This is a real bug on The stack loop never writes Proof: I checked out Greedy generation with
( The test barely moved because it feeds I regenerated the
|
|
[For maintainers] Suggested jobs to run (before merge) run-slow: blip_2, longt5, mt5, pix2struct, pop2piano, switch_transformers, t5, udop, umt5 |
|
Fair enough, I was mainly concerned about the switch transformer failure, checking slow ci and then merge |
|
run-slow: blip_2, longt5, mt5, pix2struct, pop2piano, switch_transformers, t5, udop, umt5 |
|
This comment contains models: ["models/blip_2", "models/longt5", "models/mt5", "models/pix2struct", "models/pop2piano", "models/switch_transformers", "models/t5", "models/udop", "models/umt5"] |
CI ResultsCommit Info
The test failure analysis could not be completed. Please check the workflow run for details. |
|
I will check again tomorrow, I think we are safe but I dont want to merge with broken slow ci |
|
run-slow: blip_2, longt5, mt5, pix2struct, pop2piano, switch_transformers, t5, udop, umt5 |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: blip_2, longt5, mt5, pix2struct, pop2piano, switch_transformers, t5, udop, umt5 |
|
This comment contains models: ["models/blip_2", "models/longt5", "models/mt5", "models/pix2struct", "models/pop2piano", "models/switch_transformers", "models/t5", "models/udop", "models/umt5"] |
CI ResultsCommit Info
Model CI Report❌ 1 new failed tests from this PR 😭
|
560f36c
…the T5 family (huggingface#47014) * enable t5 sdpa Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * update Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * update Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * update Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix format Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix comments Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * update comments Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix mask and position Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix use_cache and _supports_attention_backend Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix _can_compile_fullgraph Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix class register Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix tests Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * Update src/transformers/models/t5/modeling_t5.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * Update src/transformers/models/t5/modeling_t5.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * Update src/transformers/models/mt5/modeling_mt5.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * Update src/transformers/models/pix2struct/modeling_pix2struct.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * Update src/transformers/models/pop2piano/modeling_pop2piano.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * Update src/transformers/models/pop2piano/modeling_pop2piano.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * Update src/transformers/models/pop2piano/modeling_pop2piano.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * fix comments Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix name Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix tyests Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix test Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix tests Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix test Signed-off-by: jiqing-feng <jiqing.feng@intel.com> --------- Signed-off-by: jiqing-feng <jiqing.feng@intel.com> Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com>
…the T5 family (huggingface#47014) * enable t5 sdpa Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * update Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * update Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * update Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix format Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix comments Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * update comments Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix mask and position Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix use_cache and _supports_attention_backend Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix _can_compile_fullgraph Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix class register Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix tests Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * Update src/transformers/models/t5/modeling_t5.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * Update src/transformers/models/t5/modeling_t5.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * Update src/transformers/models/mt5/modeling_mt5.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * Update src/transformers/models/pix2struct/modeling_pix2struct.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * Update src/transformers/models/pop2piano/modeling_pop2piano.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * Update src/transformers/models/pop2piano/modeling_pop2piano.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * Update src/transformers/models/pop2piano/modeling_pop2piano.py Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * fix comments Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix name Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix tyests Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix test Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix tests Signed-off-by: jiqing-feng <jiqing.feng@intel.com> * fix test Signed-off-by: jiqing-feng <jiqing.feng@intel.com> --------- Signed-off-by: jiqing-feng <jiqing.feng@intel.com> Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com>
What this PR does
Refactors the T5 attention stack to route through
ALL_ATTENTION_FUNCTIONS,so T5 can dispatch to
sdpa/eagerinstead of the old eager-only path, andpropagates the change to the copied-from family.
Now
_supports_sdpa = True/_supports_attention_backend = Trueand runningtest_eager_matches_sdpa_inference:t5(reference),mt5,udop,pop2pianopix2struct— also required migrating the vision-tower attention(
Pix2StructVisionAttention), a standard dense attention that was not on theinterface, otherwise SDPA is unusable for the full model.
umt5— its attention is its own dense implementation (not# Copied fromT5); migrated the same way.
switch_transformers(modular, inherits T5 attention) is regenerated so itsmodeling matches the refactored T5, but it stays eager-only (
_supports_sdpanot set): it is not on the SDPA path, this only propagates the eager refactor.
Alignment applied to migrated modules: attention goes through
ALL_ATTENTION_FUNCTIONS(witheager_attention_forwardfallback),self.scaling,is_causal; the relative position bias is folded into the additive attention mask;forwards use
**kwargs: Unpack[TransformersKwargs]+@can_return_tuple/@auto_docstring/@merge_with_config_defaults/@capture_outputs, dropping themanual
return_dict/output_attentions/output_hidden_statesresolution.🚨 Behavior changes
sdpa, softmax no longer force-upcasts to fp32 as the old path did;numerics stay within the
eager_matches_sdpatolerance.output_attentionsremoved from internal block/layer tuple returns; attentionsare collected via
capture_outputs.test_eager_matches_sdpa_inferenceis not skipped for any enabled model.longt5 stays not-SDPA-enabled
The regular
LongT5Attention(copied from T5) is on the interface, but the encoderalways runs the block-sparse
LongT5LocalAttention/LongT5TransientGlobalAttention.These operate on 5D blocked tensors
(batch, num_blocks, heads, block_len, 3*block_len),while
sdpa_attention_forwardassumes 4D(batch, heads, seq, dim)— they cannot gothrough the interface at all.
LongT5EncoderModelis purely block-sparse, so theframework refuses to switch its attention implementation:
Forcing
_supports_sdpa = Truemakes the encoder-only sdpa tests fail (fp16mean relative difference: nan). Same precedent aspegasus_x(block-sparseencoder + regular decoder), which also sets
_supports_sdpa = False.Testing
utils/check_copies.pypasses; full sweep green: