Repository navigation
Conversation
…nd doubling its cache Three defects on the `add_cross_attention=True` path, none of them present in GPT-2: - `ImageGPTModel` built a plain `DynamicCache`, so the cross-attention branch read an unbound `is_updated` and raised `UnboundLocalError` on the default `use_cache=True` path (huggingface#40811). - `ImageGPTAttention` never applied `encoder_attention_mask` and masked cross-attention with the decoder's own mask instead, so encoder padding was attended and a padded decoder batch raised a shape error (huggingface#38635). - `ImageGPTAttention` wrote the cross-attention cache on every step, so from the second step the cache was appended to itself and `generate` failed on the key length (huggingface#38635; GPT-2 got the guard in huggingface#39754, decision_transformer through its `# Copied from`). Each fix is GPT-2's own code: `modeling_gpt2.py:560-566`, `:173` and `:193-195`.
molbap
reviewed
Sep 11, 2026
molbap
left a comment
Collaborator
There was a problem hiding this comment.
thanks, lgtm as long as the tests are simplified, we don't want to add that much code surface.
…che-write condition
Contributor
|
[For maintainers] Suggested jobs to run (before merge) run-slow: imagegpt |
Contributor
CI recapDashboard: View test results in Grafana |
Contributor
Author
|
Addressed in 7e6c795: the three cross-attention tests are one |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes #48693, where @vasqu agreed it is worth fixing for now, while noting it is not high priority
and that this feature may be removed later. No urgency from me — happy for it to sit, or to close
it if the path is going away.
With
add_cross_attention=True,ImageGPTModelis broken four ways where GPT-2 is fine: thedefault
use_cache=Truecall raisesUnboundLocalErroronis_updated; the encoder mask isdiscarded, so padded encoder positions still reach the output; a padded decoder
attention_maskraises a
RuntimeErroron the mask shape; and one cached decoding step leaves 22 keys in thecross-attention cache for 11 encoder positions.
Each of the three fixes is GPT-2's own code, restored:
attention_mask = encoder_attention_mask(modeling_gpt2.py:173), dropped by [cache] make all classes cache compatible finally #38635. Itreplaces the decoder mask rather than sitting beside it, which also removes the shape clash.
modeling_gpt2.py:193-195), which Fix GPT2 with cross attention #39754 added to GPT-2and, through
# Copied from, todecision_transformer. ImageGPT has no such marker.EncoderDecoderCachewrap (modeling_gpt2.py:561-566), removed by Fixes in check_model_inputs, GPTBigCodeModel and ImageGPTModel #40811. It costs nothingwhen
add_cross_attentionis unset.The test ports #47946's encoder-mask check under a padded decoder mask, adds an assertion that
corrupting visible encoder positions moves the output (without it the check passes for an
encoder-blind model), and compares one cached decoding step against the full forward. Reverting
any one fix reds it.
AI-assisted, per
CONTRIBUTING.md. Coordination: #48693, which I opened and @vasqu answered.Differentiation: #45773 fixes the same crash for whisper, moonshine, pix2struct and
audioflamingo3, not ImageGPT; #44076 refactors this file's output collection, not the cache or the
mask. I reviewed every changed line and ran the suite locally.
Code Agent Policy
Before submitting
Pull Request checks?
to it if that's the case. [ImageGPT] Cross-attention crashes, ignores encoder_attention_mask, and doubles its cache #48693
Who can review?
@vasqu