Fix geglu - #986
Merged
Merged
Conversation
lancerts
approved these changes
Dec 23, 2025
3 tasks
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
2 participants
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.
Fixes #959 for fp16
geglu comparison tests with the original torch implementation, were passing with very loose tolerance. This PR fixes the issue for fp16.
geglu tests had loose tolerance both for fp32 and fp16. These seem to be different bugs. Here I fix the bug affecting fp16, which affected only the gradients for the up_proj matrix. Specifically the issue was the recomputation of the forward inside the backward. In the original torch implementaiton, and for fp16, the forward values are implicitly cast to fp16 then stored and reused. The implicit casting step was missing from the current implementation. Note that following downcasting to fp16 I reupcast to fp32 for computations inside the backward.
The FP16 tests now pass with a tolerance of 1e-2, which is a commonly accepted standard. I did not benchmark performance after the bug fix, but I expect any impact to be minimal.
Testing Done
test_geglu.py tests are passing with the tighter tolerance for fp16 on a 1x RTX 5070
@Tcc0403