Skip to content

Restore dynamic_full full-layer cache guard for assisted/contrastive decoding - #47153

Closed
qflen wants to merge 1 commit into
huggingface:mainfrom
qflen:restore-dynamic-full-cache-guard
Closed

Restore dynamic_full full-layer cache guard for assisted/contrastive decoding#47153
qflen wants to merge 1 commit into
huggingface:mainfrom
qflen:restore-dynamic-full-cache-guard

Conversation

@qflen

@qflen qflen commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

CI

Edit (2026-07-17): Reduced to the regression test only. Since I opened this, #47347 (Inkling) reworked _prepare_cache_for_generation to always pass the config and then swap sliding layers for full ones, which already fixes #46629 on main and makes the guard below redundant. The guard was also breaking Inkling's test_generate_with_mtp in the merge queue (it withholds the config for Inkling's hybrid/hybrid_sliding layers, so has_previous_state raises); see comment below. This PR now adds only test_assisted_generation_sliding_window_model, which passes on current main and fails on the pre-#47347 code. Original description preserved below.

Fixes #46629

_prepare_cache_for_generation used to withhold the model config from DynamicCache when cache_implementation="dynamic_full", the mode forced by assisted / prompt-lookup / contrastive decoding (which roll the cache back with crop(), unsupported by sliding-window layers).

#46600 started passing the config unconditionally, so sliding-window models (Gemma, Mistral, Phi-3, ...) now build DynamicSlidingWindowLayers in that mode and crop() raises once generation runs past the window:

ValueError: Cannot `crop` a `DynamicSlidingWindowLayer` after it has seen more tokens than its sliding window

It also dropped the guard because MiniMax-M3 sparse layers need their dedicated MiniMaxM3VLSparseCacheLayer (built only when the config is passed).

So instead of a plain revert, restore the guard but keep passing the config for the layer types that require it, now including minimax_m3_sparse (non-sliding and croppable, so it does not reintroduce the crash):

text_config = self.config.get_text_config(decoder=True)
needs_config = any(
    x in ("mamba", "conv", "linear_attention", "minimax_m3_sparse")
    for x in (getattr(text_config, "layer_types", []) or [])
)
if generation_config.cache_implementation != "dynamic_full" or needs_config:
    dynamic_cache_kwargs["config"] = text_config

The list stays minimal on purpose:
DeepSeek's heavily_compressed_attention / compressed_sparse_attention are left out because those models also carry sliding_attention layers, so forcing the config back for them would rebuild the sliding layers and reintroduce the crash. Only non-sliding, croppable layer types that need their own cache class belong here.

Regression test added (test_assisted_generation_sliding_window_model): a tiny sliding-window Gemma2 run under assisted + prompt-lookup decoding, which raises on main and passes with the fix.

Before / after on a tiny sliding-window Gemma2 (CPU, no downloads)

Gemma2 with sliding_window=4, generating past the window under assisted and prompt-lookup decoding:

# before (on main)
[assisted]      ValueError: Cannot `crop` a `DynamicSlidingWindowLayer` after it has seen more tokens than its sliding window
[prompt_lookup] ValueError: Cannot `crop` a `DynamicSlidingWindowLayer` after it has seen more tokens than its sliding window

# after (this PR)
[assisted]      OK -> generated 20 tokens
[prompt_lookup] OK -> generated 20 tokens

cc @ArthurZucker (#46600 author), @zucchini-nlp

@zucchini-nlp zucchini-nlp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, great catch, and thanks for the test!

Comment thread src/transformers/generation/utils.py Outdated
Comment on lines +1961 to +1963
needs_config = any(
x in ("mamba", "conv", "linear_attention", "minimax_m3_sparse")
for x in (getattr(text_config, "layer_types", []) or [])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really a fan of enlisting layer types here, but since it was there prev, fine. We'll have to put this somewhere on config/gen_config at some point i think, as a single property

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, agreed

@HuggingFaceDocBuilderDev

Copy link
Copy Markdown

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.

@zucchini-nlp
zucchini-nlp added this pull request to the merge queue Jul 16, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 16, 2026
@zucchini-nlp
zucchini-nlp added this pull request to the merge queue Jul 17, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 17, 2026
@zucchini-nlp
zucchini-nlp added this pull request to the merge queue Jul 17, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 17, 2026
@zucchini-nlp
zucchini-nlp added this pull request to the merge queue Jul 17, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 17, 2026
@zucchini-nlp
zucchini-nlp added this pull request to the merge queue Jul 17, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 17, 2026
@zucchini-nlp
zucchini-nlp added this pull request to the merge queue Jul 17, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 17, 2026
@zucchini-nlp

Copy link
Copy Markdown
Member

What is wrong with CI 馃槶

@qflen

qflen commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

lol

@qflen

qflen commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

@zucchini-nlp since I opened this, #47347 (Inkling) reworked _prepare_cache_for_generation to always pass the config to DynamicCache and swap sliding layers for full ones. That fixes the issue on its own, and it also breaks the queue against this PR.

This PR withholds the config under dynamic_full unless the layer type is in a small allowlist (mamba/conv/linear_attention/minimax_m3_sparse). Inkling's hybrid/hybrid_sliding aren't in it, so DynamicCache builds plain attention layers and has_previous_state raises in Inkling{Audio2Text,Vision2Text}ModelTest::test_generate_with_mtp.

The two edits touch different lines, so it merges cleanly and this PR's own checks stay green, which is why only the merge queue catches it.

I'll reduce this to just the regression test. Otherwise, feel free to close it (and the issue).

@qflen
qflen force-pushed the restore-dynamic-full-cache-guard branch from 503cc99 to a98dc67 Compare July 17, 2026 19:00
@github-actions

Copy link
Copy Markdown
Contributor

CI recap

Dashboard: View test results in Grafana
Latest run: 29605954488:2
Result: failure | Jobs: 15 | Tests: 172,134 | Failures: 0 | Duration: 15h 18m

@zucchini-nlp

Copy link
Copy Markdown
Member

ahh, right, that makes sense, didn't notice the changes from Inkling release!

I will close the PR in that case as I think the regression test won't be necessary, thanks a lot for your efforts 鉂わ笍

@qflen
qflen deleted the restore-dynamic-full-cache-guard branch July 20, 2026 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG? transformers version '5.12.0' gemma-4 generate DynamicSlidingWindowLayer

3 participants