Add initial T5Gemma S/S support#947
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
jan-wassenberg
left a comment
There was a problem hiding this comment.
Remarkably nice work, very clean code, congrats :) A few suggestions:
Thank you, really appreciate the review and kind words :) |
65263cf to
f218a5f
Compare
|
Hi, I addressed the suggestions in the latest push:
Thanks again for the review! |
jan-wassenberg
left a comment
There was a problem hiding this comment.
Thanks for updating :) A few more questions and one fix:
| HWY_ASSERT(attn_vec_einsum_w.Cols() == qkv_dim); | ||
|
|
||
| { | ||
| static std::mutex m; |
There was a problem hiding this comment.
The code on which this was based has been fixed, please also use the same pattern here:
6241d52#diff-513210d58714f91b767de71eccf6fcfac9df6f935ca098eb1316c2c09d080217
(no extra static mutex here, but reuse g_mat_owners_mutex.
There was a problem hiding this comment.
Seems like this might have reverted inadvertently. Might be good to check your code for anything else that might have been dropped.
|
Thanks a lot for the detailed review and suggestions :) I’ve pushed a follow-up commit addressing the comments. A couple of these were cleanup mistakes on my side, especially the temporary single-threading change and the unrelated Bazel flag removal. I’m still new to this codebase, so the review was really helpful for understanding the project conventions better. Thanks again for taking the time to explain the preferred patterns. |
8459a00 to
3f7f53f
Compare
|
Thanks for making the changes. LGTM. We've just landed a big change to add Deepseek v4 which touched several files. Would you mind rebasing? |
b13603f to
52d3d59
Compare
|
Thanks! I noticed the DeepSeek V4 changes yesterday :) I’ve rebased my branch on the latest upstream/dev and pushed the updated PR. Please let me know if anything else needs adjustment. |
pculliton
left a comment
There was a problem hiding this comment.
Hi! Thanks for the PR. Jumping in to help Jan with final reviews here. Looks like some earlier fixes might have been lost. Would you mind doing a pass to make sure you're not losing older changes you'd like to keep?
| HWY_ASSERT(attn_vec_einsum_w.Cols() == qkv_dim); | ||
|
|
||
| { | ||
| static std::mutex m; |
There was a problem hiding this comment.
Seems like this might have reverted inadvertently. Might be good to check your code for anything else that might have been dropped.
|
Hi @pculliton ! Thanks for checking! I re-checked the current branch after the rebase. The encoder/decoder config fields are still guarded by I think ypur comments may be attached to an outdated diff after the rebase/force-push, but please let me know if you’re seeing a remaining case I missed.
|
pculliton
left a comment
There was a problem hiding this comment.
I think ypur comments may be attached to an outdated diff after the rebase/force-push, but please let me know if you’re seeing a remaining case I missed.
Yes! Thanks - sorry, was looking at an outdated diff. Appreciate the double-check. Can't see any other cases.
I think eventually it will make sense to move T5- (and other model-) specific code out of gemma.cc, but for now as long as your testing is working, this all LGTM.
52d3d59 to
e73ed8d
Compare
|
No worries, thanks for double-checking!
Agreed. I also think that would be the better long-term direction as more model families are added. While working on this, I was wondering whether some of the model-specific behavior/configuration could eventually move behind a cleaner model-family abstraction, instead of accumulating in gemma.cc. I’ll keep this PR focused on the current T5Gemma S/S support, but I’d be happy to help with that cleanup separately if maintainers think it makes sense. |
|
Also, I rebased again on current latest upstream/dev after the native tokenizer change and pushed the updated branch. Local |


Summary
Adds initial T5Gemma S/S support to gemma.cpp.
This includes:
python/convert_from_safetensors.pyScope
This PR targets T5Gemma v1 S/S only.
Out of scope:
Local Validation
bazel test -c opt //:configs_test //:tensor_info_test //:kv_cache_testt5gemma-s-s-itHelloI am a new user, ...Notes
Opened as draft to get early maintainer feedback on scope, naming, and whether this should be split further.
Additional Note:
//:gemma_lib_testcurrently fails locally for me in an existingGemmaAttentionFlash/EMU128golden mismatch on Apple Silicon; this reproduced outside the T5Gemma changes.Fixes #661
Note: this initial PR adds T5Gemma S/S support. Additional T5Gemma variants can be added in follow-up PRs after this lands.