Repository navigation
Harmonize past_key_value to past_key_valueS everywhere - #39956
Merged
Merged
Conversation
|
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. |
Cyrilvallez
changed the title
past_key_value to past_key_values everywherepast_key_value to past_key_valueS everywhere
ArthurZucker
reviewed
Aug 8, 2025
ArthurZucker
left a comment
ArthurZucker
left a comment
Collaborator
There was a problem hiding this comment.
This is breaking! Let's add just 1 deprecation cycle, it does not cost much IMO!
ArthurZucker
approved these changes
Aug 8, 2025
ArthurZucker
reviewed
Aug 8, 2025
Comment on lines
-73
to
-77
| if "past_key_value" in kwargs and kwargs["past_key_value"] is not None: | ||
| kwargs["past_key_value"] = None | ||
| message += " `past_key_value=None`," | ||
| do_warn = True | ||
|
|
Collaborator
There was a problem hiding this comment.
to not break we still nedd this no?
Member
Author
There was a problem hiding this comment.
Humm good catch, since the decorator will change the name correctly on the forward args but this is __call__ of the Module, I believe it should indeed still be there!
Contributor
|
[For maintainers] Suggested jobs to run (before merge) run-slow: arcee, aria, autoformer, bamba, bart, bert, bert_generation, big_bird, bigbird_pegasus, biogpt, bitnet, blenderbot, blenderbot_small |
2 of 4 tasks
ydshieh
added a commit
that referenced
this pull request
Aug 16, 2026
…h WhisperForCausalLM Fix 1 — is_updated UnboundLocalError (WhisperAttention.forward) PR #39956 wrapped the `is_updated` assignment in an isinstance guard but left it consumed unconditionally below. The bug was dormant while Whisper always received EncoderDecoderCache. PR #43679 changed generate() to pre-initialize a plain DynamicCache for is_encoder_decoder=False models, activating the latent bug. Fix: initialize is_updated=False before the guard, identical to PR #40517 which fixed 55 other models. Fix 2a — Wrong cache type: DynamicCache instead of EncoderDecoderCache (WhisperDecoder.forward) generate() uses config.is_encoder_decoder to pick the cache type. WhisperForCausalLM sets is_encoder_decoder=False, so PR #43679 made generate() pre-initialize a plain DynamicCache. But WhisperForCausalLM uses cross-attention: self- and cross-attention share the same layer_idx slots in DynamicCache and overwrite each other. Fix: when generate() passes an empty DynamicCache and encoder_hidden_states are present, convert it to EncoderDecoderCache before the first forward pass. Fix 2b — Wrong layer count: 32 slots pre-allocated for a 2-layer decoder (WhisperDecoder.forward) DynamicCache(config=self.config) calls config.get_text_config(decoder=True), which for WhisperConfig returns self unchanged (flat config, no nested decoder sub-config). It then reads config.num_hidden_layers, which WhisperConfig maps via attribute_map to encoder_layers=32. For distil-whisper-large-v2, encoder_layers=32 but decoder_layers=2, so 32 cache slots are pre-allocated for a 2-layer decoder. Slots 2-31 are uninitialized and crash on crop(-n). Fix: deepcopy the config and override num_hidden_layers with decoder_layers before passing to DynamicCache. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ydshieh
added a commit
that referenced
this pull request
Aug 16, 2026
…on, and speed regression (#48000) * [Whisper] Fix cache type and layer count for speculative decoding with WhisperForCausalLM Fix 1 — is_updated UnboundLocalError (WhisperAttention.forward) PR #39956 wrapped the `is_updated` assignment in an isinstance guard but left it consumed unconditionally below. The bug was dormant while Whisper always received EncoderDecoderCache. PR #43679 changed generate() to pre-initialize a plain DynamicCache for is_encoder_decoder=False models, activating the latent bug. Fix: initialize is_updated=False before the guard, identical to PR #40517 which fixed 55 other models. Fix 2a — Wrong cache type: DynamicCache instead of EncoderDecoderCache (WhisperDecoder.forward) generate() uses config.is_encoder_decoder to pick the cache type. WhisperForCausalLM sets is_encoder_decoder=False, so PR #43679 made generate() pre-initialize a plain DynamicCache. But WhisperForCausalLM uses cross-attention: self- and cross-attention share the same layer_idx slots in DynamicCache and overwrite each other. Fix: when generate() passes an empty DynamicCache and encoder_hidden_states are present, convert it to EncoderDecoderCache before the first forward pass. Fix 2b — Wrong layer count: 32 slots pre-allocated for a 2-layer decoder (WhisperDecoder.forward) DynamicCache(config=self.config) calls config.get_text_config(decoder=True), which for WhisperConfig returns self unchanged (flat config, no nested decoder sub-config). It then reads config.num_hidden_layers, which WhisperConfig maps via attribute_map to encoder_layers=32. For distil-whisper-large-v2, encoder_layers=32 but decoder_layers=2, so 32 cache slots are pre-allocated for a 2-layer decoder. Slots 2-31 are uninitialized and crash on crop(-n). Fix: deepcopy the config and override num_hidden_layers with decoder_layers before passing to DynamicCache. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [Whisper] Restore absolute cache crop in _update_past_and_masks for speculative decoding Fix 3 — Cache size accumulation (candidate_generator.py) Commit 5ec7b73 ("crop with negative always") changed _update_past_and_masks to always remove a fixed number of tokens (crop(-1)) instead of cropping to an absolute target. When multiple speculative candidates are rejected in a round, the cache grows by several tokens but only 1 is trimmed. Over successive rounds, cache.get_seq_length() exceeds input_ids.shape[1]. _prefill then computes next_sequence_length = input_ids.shape[1] - cache.get_seq_length() which goes negative, causing input_ids[:, -n:] to produce a [1, 0] empty tensor that crashes in WhisperAttention.forward() at the reshape. Verified by bisect: at the parent of 5ec7b73 (68075f8), fixes 1+2a+2b alone are sufficient (speed assertion only). At 5ec7b73 with only fixes 1+2a+2b the reshape crash reproduces. Adding this fix restores the test to speed-assertion-only. Fix: compute the absolute target size from input_ids and only crop if the cache exceeds it, using a negative delta to comply with the current Cache.crop(negative) API. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [Generate] Sync adaptive confidence threshold to generation_config for ConfidenceCriteria PR #42702 introduced `self.assistant_generation_config` (a deepcopy with defaults applied) to hold the assistant model's config, but the adaptive threshold update (`update_candidate_strategy`) was only syncing the new best threshold to `self.assistant_generation_config.assistant_confidence_threshold`, not to `self.generation_config.assistant_confidence_threshold`. `ConfidenceCriteria` is constructed each round from `self.generation_config`, so it was always using the initial 0.4 threshold regardless of what the ROC-curve adaptation computed. This caused the assistant to always generate the maximum number of speculative tokens per round, making assisted generation slower than non-assisted on the distil-whisper test. Fix: also update `self.generation_config.assistant_confidence_threshold` whenever the adaptive threshold is updated, so each new round's ConfidenceCriteria reflects the current best threshold. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [Whisper] Format: wrap long EncoderDecoderCache line Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [AudioFlamingo3] Fix is_updated UnboundLocalError in cross-attention (sync with modular) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
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.
As per the title. I'm getting annoyed to see both, it was time to finally make it coherent everywhere.
All the changes are made, so I'll only need to remove the decorators after next release (it's only internal modules, so no need for a long deprecation cycle)
Also reapplied modular to
examples/modular-transformer