Repository navigation
Remap the legacy Gemma 1 hidden_act in the config post-init - #49084
Conversation
The Gemma 1.0 checkpoints ship `hidden_act="gelu"`, which resolves to the exact erf GELU, but they were trained with the tanh approximation. `GemmaMLP` used to correct this by reading `hidden_activation`; huggingface#35235 dropped that field and left the legacy value in force, silently. Remapping in `GemmaConfig.__post_init__` rather than in the model runs after `from_dict`, so it covers configs loaded from the Hub, and it means `save_pretrained` and anything else reading the config see the corrected value too, rather than only `GemmaMLP`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Note: #49081 covers the same ground and was opened before this one. I only saw it after pushing, which is my fault for not checking first. Happy to close this in its favour if you prefer that one, entirely your call. |
| # The Gemma 1.0 checkpoints were released with `hidden_act="gelu"`, which resolves to the exact | ||
| # erf GELU, but they were trained with the tanh approximation. `GemmaMLP` used to correct this | ||
| # by reading `hidden_activation`; #35235 dropped that field and left the legacy value in force. | ||
| # Remapping here rather than in the model means `save_pretrained` and anything reading the | ||
| # config (exporters, other runtimes) see the corrected value too. | ||
| if self.hidden_act == "gelu": | ||
| logger.warning_once( | ||
| '`hidden_act="gelu"` on a Gemma config is the legacy value from the original Gemma 1.0 ' | ||
| "releases and resolves to the exact erf GELU, while these checkpoints were trained with " | ||
| 'the tanh approximation. Setting `hidden_act="gelu_pytorch_tanh"` instead.' | ||
| ) | ||
| self.hidden_act = "gelu_pytorch_tanh" |
There was a problem hiding this comment.
| # The Gemma 1.0 checkpoints were released with `hidden_act="gelu"`, which resolves to the exact | |
| # erf GELU, but they were trained with the tanh approximation. `GemmaMLP` used to correct this | |
| # by reading `hidden_activation`; #35235 dropped that field and left the legacy value in force. | |
| # Remapping here rather than in the model means `save_pretrained` and anything reading the | |
| # config (exporters, other runtimes) see the corrected value too. | |
| if self.hidden_act == "gelu": | |
| logger.warning_once( | |
| '`hidden_act="gelu"` on a Gemma config is the legacy value from the original Gemma 1.0 ' | |
| "releases and resolves to the exact erf GELU, while these checkpoints were trained with " | |
| 'the tanh approximation. Setting `hidden_act="gelu_pytorch_tanh"` instead.' | |
| ) | |
| self.hidden_act = "gelu_pytorch_tanh" | |
| # #35235 dropped this conversion which we now handle here instead | |
| if self.hidden_act == "gelu": | |
| logger.warning_once( | |
| 'We found `hidden_act="gelu"` in this Gemma config. This is a legacy value of the official ' | |
| 'releases but it is meant to target the the tanh approximation. Setting `hidden_act="gelu_pytorch_tanh"` instead.' | |
| ) | |
| self.hidden_act = "gelu_pytorch_tanh" |
|
|
||
|
|
||
| @require_torch | ||
| class GemmaLegacyActivationTest(unittest.TestCase): |
There was a problem hiding this comment.
no new class please, let's keep it simple like a mamba2 regression test were we check that loading with gelu leads to 1 the warning and 2 the config value being the tanh approx
Applies @vasqu's suggestion for the comment and the warning text, and replaces the separate test class with a single regression test in GemmaModelTest, following the diffusion_gemma CaptureLogger pattern: the warning fires, and the config value becomes the tanh approximation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All three done, thanks. I took your suggestion for the comment and the warning, with one doubled "the" removed. The test is now a single method in |
|
|
||
| self.assertIn("gelu_pytorch_tanh", cl.out) | ||
| self.assertEqual(config.hidden_act, "gelu_pytorch_tanh") | ||
|
|
There was a problem hiding this comment.
Let's split the test into tester (like in mamba2)
Also we can capture the emitted warning message completely or a larger portion tbh
…ning Follows the mamba2 pattern: GemmaConfigTester(ConfigTester) with the check run from run_common_tests, wired in via setUp. The assertion is now on the complete emitted message rather than a fragment of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
huh the last commit doesnt make any sense why did we remove the workaround 😢
There was a problem hiding this comment.
my comment on the tests is related to the test design like
transformers/tests/models/mamba2/test_modeling_mamba2.py
Lines 315 to 317 in 763a150
Pls dont just aimlessly throw claude at it
|
Done. The test is now a Apologies for the first pass, I read "no new class" literally and put the method in |
…TY=error CI sets TRANSFORMERS_VERBOSITY=error (.circleci/create_circleci_config.py), so logger.warning_once emitted nothing and CaptureLogger captured an empty string. Wraps the capture in LoggingLevel(logging.WARNING), the same shape tests/generation/test_configuration_utils.py uses for its warning assertions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The __post_init__ remap was lost in 0042edc: a local mutation check had run `git checkout origin/main -- <source files>`, which updates the index as well as the working tree, and the follow-up commit staged only the test file. The source files were therefore committed back at their origin/main state while the working tree still held the fix, so every local run kept passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Sorry for the red. Two things, both mine. The config change had been reverted out of the commit by a local mutation check that left my working tree intact, so I was testing something I had not actually pushed. Restored in 17e4cbe. The test also needed |
Moves the check onto GemmaModelTester as create_and_check_legacy_hidden_act_remap, with a short delegating test method on GemmaModelTest, matching the mamba2 shape at tests/models/mamba2/test_modeling_mamba2.py#L315-L317. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Done, split the way you meant: |
|
run-slow: gemma |
vasqu
left a comment
There was a problem hiding this comment.
Looks good sanity checking with the slow ci 🤗 thanks!
|
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. |
|
Hmm I see that its those gemmas that are affected of the remap, I dont want to merge unless we find the commits that actually changed the output (after the regression) so we make sure that its actually as intended |
|
Our 2 cents on this. The expectations those tests use were re-recorded after the regression, so they encode the exact-GELU output. #35235 landed as
Four of the five were recorded twenty months after the regression, specifically for A10G. Worth one caveat: More directly: those tests pass on The precision pattern points the same way. Only bf16, fp16 and 4bit moved; no F32 test changed. That matches the measurement in #49051, where the top-10 ranks are unchanged at F32 and 7 of 30 move at BF16. I cannot regenerate the expected values myself, since they are per-device and I only have a consumer card. |
|
That's totally what I'm looking for! I was just wanting to make sure that the regression also forced updates on these slow tests and they did I will update the values myself whenever I find time today 🫡 |
|
Thanks for the heads-up @PCfVW, and congrats on getting this across the line — the review with @vasqu looks great. Since #49084 is further along and already has maintainer engagement, I'll close this one in its favor. The fix matters more than whose commit it is. Glad the bug is getting fixed either way. 🤗 |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: gemma |
|
run-slow: gemma |
Nvidia CIThis comment contains models: ["models/gemma"] |
AMD CIThis comment contains models: ["models/gemma"] |
CI recapDashboard: View test results in Grafana |
CI Results (AMD)Commit Info
Model CI Report❌ 2 new failed tests from this PR 😭
|
…inistic_algorithms Static cache triggers torch.compile which uses the Triton autotuner. The autotuner benchmarks multiple kernel configs based on GPU timing, which is non-deterministic and can flip a logit near a decision boundary introduced by PR #49084 (gelu_pytorch_tanh). torch.use_deterministic_algorithms(True) forces a single deterministic kernel path, making the output stable across runs and machines (verified: 6/6 stable on flaky A10G). Also removes the redundant ("cuda", 8) entry and the output truncation workaround. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…p16_static_cache Static cache triggers torch.compile which uses the Triton autotuner. The autotuner benchmarks multiple kernel configs based on GPU timing, which is non-deterministic and can flip a logit near the decision boundary introduced by PR #49084 (gelu_pytorch_tanh). Replacing the XPU-only decorator with require_deterministic_for_accelerator ensures deterministic kernel selection on all accelerators (CUDA, XPU, etc.), making the output stable across runs and machines (verified: 6/6 stable on flaky A10G runner). Also removes the redundant ("cuda", 8) Expectations entry and the output-truncation workaround that was masking the flakiness. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… after huggingface#49084 (huggingface#49133) * [Gemma] Fix test_model_7b_fp16_static_cache expected value for cuda 8 after activation fix PR huggingface#49084 corrected the Gemma 1 hidden_act from "gelu" to "gelu_pytorch_tanh" (the activation the model was actually trained with). This changes the generated output on A10G (cuda, 8): "1999 4.0L 4x4" → "1995 4.0L 4x4" for test_model_7b_fp16_static_cache. Verified 30/30 runs on an A10G runner. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * revert: restore 1999 expected value, add dummy comment to retrigger CI Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [Gemma] Fix test_model_7b_fp16_static_cache flakiness with use_deterministic_algorithms Static cache triggers torch.compile which uses the Triton autotuner. The autotuner benchmarks multiple kernel configs based on GPU timing, which is non-deterministic and can flip a logit near a decision boundary introduced by PR huggingface#49084 (gelu_pytorch_tanh). torch.use_deterministic_algorithms(True) forces a single deterministic kernel path, making the output stable across runs and machines (verified: 6/6 stable on flaky A10G). Also removes the redundant ("cuda", 8) entry and the output truncation workaround. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [Gemma] Use require_deterministic_for_accelerator for test_model_7b_fp16_static_cache Static cache triggers torch.compile which uses the Triton autotuner. The autotuner benchmarks multiple kernel configs based on GPU timing, which is non-deterministic and can flip a logit near the decision boundary introduced by PR huggingface#49084 (gelu_pytorch_tanh). Replacing the XPU-only decorator with require_deterministic_for_accelerator ensures deterministic kernel selection on all accelerators (CUDA, XPU, etc.), making the output stable across runs and machines (verified: 6/6 stable on flaky A10G runner). Also removes the redundant ("cuda", 8) Expectations entry and the output-truncation workaround that was masking the flakiness. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: ydshieh <ydshieh@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixes #49051.
The Gemma 1.0 checkpoints ship
hidden_act="gelu", which resolves to the exact erf GELU, but they were trained with the tanh approximation.GemmaMLPused to correct this by readinghidden_activation; #35235 dropped that field and left the legacy value in force.As @vasqu suggested on the issue, the remap goes in
GemmaConfig.__post_init__rather than in model init. It therefore runs afterfrom_dict, which is the path every Hub config takes, and it meanssave_pretrainedand anything else reading the config see the corrected value too, not justGemmaMLP.Verified on
google/gemma-2b: with this patch the logits aretorch.equalto a run forced togelu_pytorch_tanh, on CPU/F32 and CUDA/BF16. Without it, the top-10 max abs-diff is1.978e-2in F32, and BF16 reorders 7 of 30 top-10 positions.Tests cover the legacy value, the
from_dictpath, thesave_pretrainedround-trip, the default, and deliberate values (gelu_new,silu,relu) left alone. The four that assert the fix fail without it.