| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
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. |
Sorry, something went wrong.
|
Hi @Linxiushen, |
Sorry, something went wrong.
|
Done, @Venkaiahbabuneelam — the branch is now up to date with main (merged upstream/main in via a merge commit, no force-push, so the review history is intact). GitHub had it as BEHIND rather than a content conflict, and the merge was clean: main's recent changes (voices / gaos / types) don't touch local_tokenizer.py, so this PR's diff is still just the two files — google/genai/local_tokenizer.py and the new tests/local_tokenizer/test_roles_alignment.py. pytest google/genai/tests/local_tokenizer/test_roles_alignment.py — 2 passed on the merged branch; the branch's mypy, check-changes and conventionalcommits checks are green. The only remaining red check is cla/google. That one needs the account owner to sign the Google CLA (at cla.developers.google.com) — it's an individual authorization that has to come from them, not something I can complete on their behalf. I've flagged it to them. Happy to make any further changes to the code in the meantime. |
Sorry, something went wrong.
|
@Venkaiahbabuneelam Rebased onto current main at c4c6ba2 — the branch is mergeable again. The two commits are unchanged in content; the tokenizer tests pass before and after (35/35). The cla/google check is a separate step and will be handled from the account side. |
Sorry, something went wrong.
…tokenized
compute_tokens() built `roles` with one entry per Part, but _TextsAccumulator
does not emit one text per Part: a function_call or function_response part
contributes the function name plus every key and string value of its
args/response as separate texts, and a thought_signature-only part contributes
none. The two lists were then combined with zip(), which silently truncates to
the shorter one.
Result, for a model turn carrying function_call(get_weather, {location: NYC})
followed by a user text: four texts are tokenized, two TokensInfo entries come
back, and the second one labels the model's own argument 'location' as
role='user'. With a thought_signature-only part first, the user's own text is
labelled role='model'. TokensInfo.role is documented as 'the role from the
corresponding Content', which this violates.
Extend `roles` by the number of texts the accumulator actually added for each
Content (via a new _TextsAccumulator.__len__), so both zip() sites stay aligned
with get_texts(). Plain-text contents add exactly one text per part, so their
behaviour is unchanged; the existing 33 tests pass.
Adds tests for both tokenizer branches; they fail on the old code.
| Back | FazBrowse Home | New Git URL |
What
LocalTokenizer.compute_tokens() builds roles with one entry per Part:
but _TextsAccumulator does not emit one text per part. A function_call / function_response part contributes the function name plus every key and every string value of its args/response as separate texts; a thought_signature-only part contributes none. The two lists are then combined with zip() at both tokenizer branches (lines 386 and 405), and zip silently truncates to the shorter one.
Effect
Model turn with function_call(get_weather, {location: "NYC"}), then a user text:
Two of the four texts are dropped from the result, and the surviving entries carry the wrong role. With a thought_signature-only part first (routine on Gemini 2.5/3 function-calling and thinking turns), the user's own text is labelled role='model'.
TokensInfo.role is documented as "the role from the corresponding Content" (types.py), which this violates. The output also matches neither "one entry per part" nor "one entry per text", so it isn't a defensible alternate semantics.
Reproduced on the real gemma3 SentencePiece model (_local_tokenizer_loader's pinned download) and, for the HuggingFace/gemma4 branch, with the same mock pattern test_local_tokenizer.py already uses for that branch.
Fix
Extend roles by the number of texts the accumulator actually added for each Content:
with a two-line _TextsAccumulator.__len__. Both zip() sites are now aligned with get_texts() by construction. Plain-text contents add exactly one text per part, so their behaviour is unchanged.
After the fix the example above returns 4 entries with roles model, model, model, user.
Verification
+12 −4 in local_tokenizer.py, plus one test file covering both branches.
CLA
I'll complete the Google CLA when the bot prompts.
Investigated and fixed with AI assistance (Claude); I reviewed the change and ran the verification above myself.