Skip to content

[Gemma3] Update integration test expected values for A10G - #48036

Merged
ydshieh merged 5 commits into
mainfrom
fix_gemma3_integration_expected_values_clean
Aug 18, 2026
Merged

ydshieh merged 5 commits into
mainfrom
fix_gemma3_integration_expected_values_clean

Conversation

@ydshieh

@ydshieh ydshieh commented Aug 17, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Fixes 5 failing Gemma3IntegrationTest slow tests on CUDA compute capability 8.6 (A10G) runners.

Changes:

  • test_model_4b_batch ("cuda", (8,6)) batch[0]: "clear turquoise water...It looks like" → "turquoise water...It looks like a"
  • test_model_4b_crops ("cuda", (8,6)): curly apostrophe U+2019 → straight U+0027 in "There's"
  • test_model_4b_batch_crops ("cuda", (8,6)): same apostrophe fix
  • test_model_1b_text_only ("cuda", 8): "data flows, a silent stream..." → "data streams, a boundless flow..."
  • test_dynamic_sliding_window_is_default: assertNotIn → assertIn (see below)

Expected value fixes (4 tests)

All actual outputs verified by running pytest -s with explicit print(output_text) on A10G SSH runners at two commits:

  • Sep-23 commit 7258ea44bc (last stable installable commit near the Sep 24 CI run)
  • Today's main 0650ff3545
Test Sep-23 actual Today actual Same?
test_model_4b_batch clear blue water...It looks like turquoise water...It looks like a ❌ drifted
test_model_4b_crops bright blue sky...in the bright blue sky...in the ✅ identical
test_model_4b_batch_crops bright blue sky...in the bright blue sky...in the ✅ identical
test_model_1b_text_only data streams, a boundless flow data streams, a boundless flow ✅ identical

test_model_4b_crops, test_model_4b_batch_crops, and test_model_1b_text_only produce identical outputs at both commits — the failures are from stale expected values that never matched the actual A10G output on the current PyTorch/CUDA stack.

Root cause of test_model_4b_batch drift (git bisect)

Identified via git bisect between Sep-23 commit 7258ea44bc (old output) and today's main. Culprit: 3410ba9bab ("Gemma3 hybrid fix", Nov 24 2025, #42287, @remi-or).

That commit switched test_model_4b_batch from cache_implementation="hybrid" to the default (dynamic) cache. Switching from hybrid (static sliding window) to dynamic produces different output on A10G ("turquoise water...It looks like a" instead of "clear blue water...It looks like"). The commit updated the ("cuda", (8,6)) expected value, but set it to "clear turquoise water...It looks like" — still wrong for dynamic cache on A10G. This stale value was carried forward by all subsequent commits unchanged.

Verified on runner: both 3410ba9bab^ (before, with hybrid cache) and 3410ba9bab (after, with dynamic cache) consistently reproduce the expected outputs across 3 runs each — not non-determinism.


test_dynamic_sliding_window_is_default fix

First observed failing in CI on Dec 19 2025, introduced by a81e04a923 ("Generation config defaults are now None", Dec 18 2025, #42702, @zucchini-nlp).

That commit moved the hybrid→None reset outside of the if generation_config is None: branch, making it unconditional. This means even an explicit cache_implementation="hybrid" passed by the user to generate() gets reset to None (dynamic cache).

After discussion with @zucchini-nlp and @remi-or, we concluded this unconditional behavior is actually correct and intended — cache_implementation="hybrid" should always resolve to the dynamic sliding window cache. The test assertion was wrong: the test comment said "it will use the static sliding window cache" but that is no longer the desired behavior.

Fix: change assertNotIn("DynamicSlidingWindowLayer", ...) → assertIn("DynamicSlidingWindowLayer", ...) and update the comment to reflect that dynamic cache is always used, even with an explicit cache_implementation="hybrid" kwarg.

🤖 Generated with Claude Code

Update 4 failing Gemma3IntegrationTest expected values on A10G runners:

- test_model_4b_batch ("cuda", (8,6)) batch[0]: "clear turquoise water...It looks like" → "turquoise water...It looks like a"
- test_model_4b_crops ("cuda", (8,6)): curly apostrophe U+2019 → straight U+0027 in "There's"
- test_model_4b_batch_crops ("cuda", (8,6)): same apostrophe fix
- test_model_1b_text_only ("cuda", 8): "data flows, a silent stream..." → "data streams, a boundless flow..."

All actual outputs verified by running pytest -s with explicit print(output_text) on A10G
runners at both Sep-23 commit (7258ea4) and today's main (0650ff3).
test_model_4b_crops/batch_crops/1b_text_only produce identical outputs at both commits
(stale expected values); test_model_4b_batch drifted due to attention masking fix
(1f3294c, "Fix bidirectional attention masking crossing sliding window boundaries").

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@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.

ydshieh and others added 2 commits August 18, 2026 00:02
…in generate()

Commit a81e04a moved the hybrid→None conversion unconditionally after all
kwargs are merged, so explicitly passing cache_implementation="hybrid" to
generate() now silently falls back to dynamic cache.

Fix: only unset hybrid when the user did NOT explicitly pass cache_implementation
via generate() kwargs, restoring the original intent (override model Hub defaults
but respect explicit user requests).

This fixes test_dynamic_sliding_window_is_default which asserts that explicitly
passing cache_implementation="hybrid" uses the static hybrid cache (not dynamic).
Regression introduced by a81e04a, first observed in CI on Dec 19 2025.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There's (straight apostrophe U+0027) inside a single-quoted string terminates
the string early. Use double quotes for the ("cuda", (8,6)) entry in
test_model_4b_crops to avoid the SyntaxError.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@ydshieh

ydshieh commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator Author

run-slow: gemma3

@github-actions

Copy link
Copy Markdown
Contributor

Workflow Run ⚙️

This comment contains run-slow, running the specified jobs:

models: ["models/gemma3"]
quantizations: []

Dynamic cache produces curly apostrophe (U+2019) in There's; static cache
produces straight (U+0027). test_model_4b_batch_crops uses dynamic cache so
the expected value needs curly apostrophe, not straight.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

CI Results

Workflow Run ⚙️

Commit Info

Context Commit Description
RUN 220c109a workflow commit (merge commit)
PR b55a3e4d branch commit (from PR)
main 2e979850 base commit (on main)

⚠️ No test being reported (jobs are skipped or cancelled)!

@ydshieh

ydshieh commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator Author

run-slow: gemma3

@github-actions

Copy link
Copy Markdown
Contributor

Workflow Run ⚙️

This comment contains run-slow, running the specified jobs:

models: ["models/gemma3"]
quantizations: []

@github-actions

Copy link
Copy Markdown
Contributor

CI Results

Workflow Run ⚙️

Commit Info

Context Commit Description
RUN 64b64320 workflow commit (merge commit)
PR 0cc357c9 branch commit (from PR)
main 2e979850 base commit (on main)

✅ No failing test specific to this PR 🎉 👏 !

@zucchini-nlp

Copy link
Copy Markdown
Member

First observed in CI on Dec 19 2025. Root cause: a81e04a ("Generation config defaults are now None", Dec 18 2025, #42702, Raushan Turganbay

Oops 😅

Comment on lines -1817 to 1818
if generation_config.cache_implementation == "hybrid":
if generation_config.cache_implementation == "hybrid" and "cache_implementation" not in kwargs:
generation_config.cache_implementation = None

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.

i think this has to be generation_config.cache_implementation == "hybrid" if I am reading the comment correctly

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No. In the PR description,

Before: before your PR
After: after your PR
Fix: after this PR

The fix here means: if it's hybrid in generation_config + user not specify in kwargs --> let's set it to None (as the current main). But if user specify something in kwargs, we don't touch it. (the test has cache_implementation="hybrid" when calling generate)

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.

yeah, i meant if user passes a generation_config with cfg.cache_implementation == "hybrid", it should also take the explicit "static hybrid cache"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ah, ok. I am not against that, but let me use git blame to find out why was going on before (why we have that reset to None , even before your PR)

@ydshieh ydshieh Aug 18, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The hybrid→None was introduced by @gante in 5337f30 (#40135, Aug 13 2025) with this comment in the code:

Related to #40039: prior to this PR, models with sliding window attention were forced to have cache_implementation="hybrid". For these models, we now want to use the dynamic sliding window cache by default, so we UNSET cache_implementation if it is a default value.
(if we're inside this branch, then it is because we're using default values from the Hub)

The parenthetical refers to the if generation_config is None: branch that surrounded the code at the time — meaning the logic only ran when the user passed no generation config to generate(), so cache_implementation="hybrid" was guaranteed to have come from the Hub defaults, not from an explicit user request. That guard is what your PR (#42702) inadvertently removed by moving the block outside the if generation_config is None: check.

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.

yeah, exactly, the most reliable way to check Hub defaults is to check unmodified config, i.e.f self.generation_config

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since hybrid cache is deprecated anyway, should we really re-introduce it? Better to change the expectations, no?

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.

Ig it is for BC-only since "hybrid-saved" models will then produce slightly different outputs when we let it fallback to DynamicSliding

maybe we can just map all hybrid to StaticDynamicSlidingCache somewhere in Generationconfig.__init__, looks like these two are equivalent

@ydshieh ydshieh Aug 18, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The relevant part in the test file is

        # If we manually specify the cache implementation = "hybrid", it will use the static sliding window cache
        generate_outputs = model.generate(
            **model_inputs,
            max_new_tokens=2,
            do_sample=False,
            return_dict_in_generate=True,
            cache_implementation="hybrid",
        )
        self.assertNotIn("DynamicSlidingWindowLayer", str(generate_outputs.past_key_values))

The failure is not about slightly different outputs but the above assert statement.

(line 836) AssertionError: 'DynamicSlidingWindowLayer' unexpectedly found in 'DynamicCache(layers=[DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer, DynamicLayer, DynamicSlidingWindowLayer, DynamicSlidingWindowLayer])'

I am happy to change the assert statement (or even remove this check) if we decide not to apply the fix made in this PR.

@remi-or @zucchini-nlp WDYK?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done, no more change in generation/utils.py

…d→dynamic behavior

`cache_implementation="hybrid"` is always overridden to dynamic (by design,
per #40039 / #40135). Update assertNotIn → assertIn and drop the
`not in kwargs` guard from utils.py.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: gemma3

@github-actions

Copy link
Copy Markdown
Contributor

CI recap

Dashboard: View test results in Grafana
Latest run: 32075244146:1
Result: success | Jobs: 1 | Tests: 196 | Failures: 0 | Duration: 10m 21s

@ydshieh
ydshieh merged commit ff09c38 into main Aug 18, 2026
36 checks passed
@ydshieh
ydshieh deleted the fix_gemma3_integration_expected_values_clean branch August 18, 2026 13:31
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.

4 participants