Skip to content

Add initial T5Gemma S/S support#947

Open
maasir554 wants to merge 8 commits into
google:devfrom
maasir554:t5gemma-support
Open

Add initial T5Gemma S/S support#947
maasir554 wants to merge 8 commits into
google:devfrom
maasir554:t5gemma-support

Conversation

@maasir554

@maasir554 maasir554 commented Jul 8, 2026

Copy link
Copy Markdown

Summary

Adds initial T5Gemma S/S support to gemma.cpp.

This includes:

  • T5Gemma S/S model config and tensor metadata
  • HF safetensors -> SBS conversion through python/convert_from_safetensors.py
  • Encoder-decoder runtime path for fresh seq2seq generation
  • BF16 default conversion path for correctness/parity
  • Basic docs and focused config/tensor/cache tests

Scope

This PR targets T5Gemma v1 S/S only.

Out of scope:

  • T5Gemma 2
  • Other T5Gemma sizes
  • Multiturn/cache reuse with changing encoder inputs
  • Fully optimized T5-specific attention/logits kernels

Local Validation

  • bazel test -c opt //:configs_test //:tensor_info_test //:kv_cache_test
  • Local optimized smoke test with t5gemma-s-s-it
    • prompt: Hello
    • output begins: I am a new user, ...
    • local speed: roughly 30-35 generated tok/s on MacBook M1, 8GB RAM

Notes

Opened as draft to get early maintainer feedback on scope, naming, and whether this should be split further.

Additional Note: //:gemma_lib_test currently fails locally for me in an existing GemmaAttentionFlash/EMU128 golden 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.

@google-cla

google-cla Bot commented Jul 8, 2026

Copy link
Copy Markdown

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.

@maasir554
maasir554 marked this pull request as draft July 8, 2026 18:31

@jan-wassenberg jan-wassenberg left a comment

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.

Remarkably nice work, very clean code, congrats :) A few suggestions:

Comment thread gemma/activations.h Outdated
Comment thread gemma/configs.h Outdated
Comment thread gemma/gemma.cc Outdated
Comment thread gemma/gemma.cc Outdated
Comment thread gemma/gemma.cc Outdated
Comment thread gemma/gemma.cc Outdated
@maasir554

maasir554 commented Jul 11, 2026

Copy link
Copy Markdown
Author

Remarkably nice work, very clean code, congrats :) A few suggestions:

Thank you, really appreciate the review and kind words :)
I’ll address your suggestions in a follow-up update.

@maasir554
maasir554 marked this pull request as ready for review July 15, 2026 18:35
@maasir554

Copy link
Copy Markdown
Author

Hi, I addressed the suggestions in the latest push:

  • added a shared Activations::num_layers
  • made encoder/decoder config fields conditional on is_encoder_decoder
  • moved/reused T5 token IDs via tokenizer.h / BOS_ID
  • reused MaybeLogitsSoftCap
  • hoisted T5Gemma encoder scratch buffers into Activations
  • switched internal encoder-cache checks to HWY_DASSERT

Thanks again for the review!

@jan-wassenberg jan-wassenberg left a comment

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.

Thanks for updating :) A few more questions and one fix:

Comment thread compression/python/compression_clif_aux.cc Outdated
Comment thread gemma/activations.h Outdated
Comment thread gemma/configs.cc Outdated
Comment thread gemma/weights.cc Outdated
HWY_ASSERT(attn_vec_einsum_w.Cols() == qkv_dim);

{
static std::mutex m;

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.

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.

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.

Seems like this might have reverted inadvertently. Might be good to check your code for anything else that might have been dropped.

Comment thread python/BUILD.bazel
@maasir554

Copy link
Copy Markdown
Author

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.

@jan-wassenberg

Copy link
Copy Markdown
Member

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?

@maasir554

Copy link
Copy Markdown
Author

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 pculliton left a comment

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.

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?

Comment thread gemma/configs.h Outdated
Comment thread gemma/weights.cc Outdated
HWY_ASSERT(attn_vec_einsum_w.Cols() == qkv_dim);

{
static std::mutex m;

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.

Seems like this might have reverted inadvertently. Might be good to check your code for anything else that might have been dropped.

@maasir554

maasir554 commented Jul 22, 2026

Copy link
Copy Markdown
Author

Hi @pculliton ! Thanks for checking! I re-checked the current branch after the rebase.

The encoder/decoder config fields are still guarded by if (is_encoder_decoder), and the weights.cc mutex change is using the shared g_mat_owners_mutex now.

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.

Screenshot 2026-07-22 at 8 38 19 PM Screenshot 2026-07-22 at 8 40 12 PM

@maasir554
maasir554 requested a review from pculliton July 22, 2026 15:14
pculliton
pculliton previously approved these changes Jul 22, 2026

@pculliton pculliton left a comment

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.

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.

@maasir554

Copy link
Copy Markdown
Author

No worries, thanks for double-checking!

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.

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.

@maasir554

Copy link
Copy Markdown
Author

Also, I rebased again on current latest upstream/dev after the native tokenizer change and pushed the updated branch. Local configs_test, kv_cache_test, and //:gemma build pass.

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.

3 participants