fix(local_tokenizer): align TokensInfo roles with the texts actually tokenized - #2987
Linxiushen wants to merge 1 commit into
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. |
|
Hi @Linxiushen, |
|
Done, @Venkaiahbabuneelam — the branch is now up to date with
The only remaining red check is |
…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.
6ffc75d to
c4c6ba2
Compare
|
@Venkaiahbabuneelam Rebased onto current |
What
LocalTokenizer.compute_tokens()buildsroleswith one entry perPart:but
_TextsAccumulatordoes not emit one text per part. Afunction_call/function_responsepart contributes the function name plus every key and every string value of its args/response as separate texts; athought_signature-only part contributes none. The two lists are then combined withzip()at both tokenizer branches (lines 386 and 405), andzipsilently 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 labelledrole='model'.TokensInfo.roleis 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 patterntest_local_tokenizer.pyalready uses for that branch.Fix
Extend
rolesby the number of texts the accumulator actually added for eachContent:with a two-line
_TextsAccumulator.__len__. Bothzip()sites are now aligned withget_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
pytest google/genai/tests/local_tokenizer/: 35 passed (33 existing + 2 new). The 2 new tests fail onmain.mypy google/genai/local_tokenizer.pywith the repo's strict config: 0 errors in this file, before and after. (My environment reports 7 pre-existing errors in_api_client.py/_local_tokenizer_loader.py/errors.pythat are identical with and without this change — dependency-version noise, not from this diff.)+12 −4inlocal_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.