Skip to content

Fix accuracy issues with Gemma models - #1448

Merged
kunal-vaishnavi merged 29 commits into
mainfrom
kvaishnavi/gemma3-mm
May 9, 2025
Merged

Fix accuracy issues with Gemma models#1448
kunal-vaishnavi merged 29 commits into
mainfrom
kvaishnavi/gemma3-mm

Conversation

@kunal-vaishnavi

Copy link
Copy Markdown
Contributor

Description

This PR fixes accuracy issues with Google's Gemma models by using bfloat16 precision, always using float32 precision to compute any LayerNorms, and casting the output logits to float32 always.

Motivation and Context

This PR has been tested with Gemma-2 and Gemma-3. It is using the bfloat16 changes from this PR as well as the missing final norm changes from this PR.

Comment thread src/python/py/models/builder.py Outdated
Comment thread src/python/py/models/builder.py Outdated
Comment thread src/python/py/models/builder.py
Comment thread src/python/py/models/builder.py Outdated
Comment thread src/python/py/models/builder.py Outdated
Comment thread src/python/py/models/builder.py Outdated
Comment thread src/python/py/models/builder.py Fixed
Comment thread src/python/py/models/builder.py Fixed
self.output_names = [name.replace("logits", "hidden_states") for name in self.output_names]
elif self.include_hidden_states:
self.output_names = ["hidden_states"] + self.output_names
self.make_outputs_init()

Check warning

Code scanning / CodeQL

`__init__` method calls overridden method

Call to self.[make_outputs_init](1) in __init__ method, which is overridden by [method Gemma2Model.make_outputs_init](2).

Copilot Autofix

AI about 1 year ago

To fix the issue, we should avoid calling a potentially overridden method (make_outputs_init) in the __init__ method of the Model class. Instead, we can:

  1. Rename the make_outputs_init method in the Model class to a private method (e.g., _make_outputs_init) to ensure it is not overridden by subclasses.
  2. Call this renamed method (_make_outputs_init) in the Model class's __init__ method.
  3. Subclasses like Gemma2Model can still define their own make_outputs_init method if needed, but it will not interfere with the Model class's initialization process.

This approach ensures that the Model class's initialization is self-contained and does not depend on subclass behavior.


Suggested changeset 1
src/python/py/models/builder.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/python/py/models/builder.py b/src/python/py/models/builder.py
--- a/src/python/py/models/builder.py
+++ b/src/python/py/models/builder.py
@@ -115,3 +115,3 @@
         }
-        self.make_outputs_init()
+        self._make_outputs_init()
 
@@ -323,3 +323,3 @@
 
-    def make_outputs_init(self):
+    def _make_outputs_init(self):
         self.exclude_lm_head = self.extra_options.get("exclude_lm_head", False)
EOF
@@ -115,3 +115,3 @@
}
self.make_outputs_init()
self._make_outputs_init()

@@ -323,3 +323,3 @@

def make_outputs_init(self):
def _make_outputs_init(self):
self.exclude_lm_head = self.extra_options.get("exclude_lm_head", False)
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
# Name = name of original LayerNorm op as if the cast nodes did not exist
# Inputs = inputs into the original LayerNorm op as if the cast nodes did not exist
# Outputs = outputs from the original LayerNorm op as if the cast nodes did not exist
def get_shape_of_value_info(target_name):

Check notice

Code scanning / CodeQL

Explicit returns mixed with implicit (fall through) returns

Mixing implicit and explicit returns may indicate an error as implicit returns always return None.

Copilot Autofix

AI about 1 year ago

To fix the issue, we will add an explicit return None statement at the end of the get_shape_of_value_info function. This ensures that the function always returns a value explicitly, making its behavior consistent and easier to understand. The change will not alter the existing functionality but will make the function's intent clearer.


Suggested changeset 1
src/python/py/models/builder.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/python/py/models/builder.py b/src/python/py/models/builder.py
--- a/src/python/py/models/builder.py
+++ b/src/python/py/models/builder.py
@@ -1127,3 +1127,3 @@
                     return shape
-
+            return None
         # Save original inputs and outputs
EOF
@@ -1127,3 +1127,3 @@
return shape

return None
# Save original inputs and outputs
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
Comment thread src/python/py/models/builder.py Fixed
Comment thread src/python/py/models/builder.py
@kunal-vaishnavi
kunal-vaishnavi enabled auto-merge (squash) May 9, 2025 00:24
@kunal-vaishnavi
kunal-vaishnavi disabled auto-merge May 9, 2025 00:24
@kunal-vaishnavi
kunal-vaishnavi enabled auto-merge (squash) May 9, 2025 00:28
@kunal-vaishnavi
kunal-vaishnavi merged commit c77d9e4 into main May 9, 2025
@kunal-vaishnavi
kunal-vaishnavi deleted the kvaishnavi/gemma3-mm branch May 9, 2025 03:08
Ryan Hill (RyanUnderhill) pushed a commit that referenced this pull request May 12, 2025
### Description

This PR fixes accuracy issues with Google's Gemma models by using
bfloat16 precision, [always using float32 precision to compute any
LayerNorms](https://github.com/huggingface/transformers/blob/fee1190601b5d04ec6d3f7f58fd22788d7f3236d/src/transformers/models/gemma3/modeling_gemma3.py#L141-L146),
and casting the output logits to float32 always.

### Motivation and Context

This PR has been tested with Gemma-2 and Gemma-3. It is using the
bfloat16 changes from [this
PR](#1447) as well as
the missing final norm changes from [this
PR](#1420).

---------

Co-authored-by: Nenad Banfic <46795300+nenad1002@users.noreply.github.com>
Co-authored-by: Nenad Banfic <nebanfic@microsoft.com>
Baiju Meswani (baijumeswani) added a commit that referenced this pull request May 14, 2025
Address previous PR review comments from #1470 (#1473)
Address QNN specific regressions (#1470)
Fix array eos_token_id handling (#1463)
Constrained decoding integration (#1381)
Remove BF16 CPU from valid GQA configuration (#1469)
Avoid adding providers if not requested (#1464)
Persist provider options across ClearProviders, AppendProvider where
possible (#1454)
Fix accuracy issues with Gemma models (#1448)
Add bfloat16 support in model builder (#1447)
Add final norm for LoRA models (#1446)

Update version to 0.8.0-rc3

---------

Co-authored-by: kunal-vaishnavi <115581922+kunal-vaishnavi@users.noreply.github.com>
Co-authored-by: Nenad Banfic <46795300+nenad1002@users.noreply.github.com>
Co-authored-by: Nenad Banfic <nebanfic@microsoft.com>
Co-authored-by: Baiju Meswani <bmeswani@microsoft.com>
Co-authored-by: Abhishek Jindal <abjindal@microsoft.com>
Co-authored-by: Ying Xiong <yingxiong@microsoft.com>
Co-authored-by: Michał Moskal <michal@moskal.me>
Co-authored-by: Kunal Vaishnavi <kvaishnavi@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants