Fix missing f-prefix in _normalize_token error message - #722
Fix missing f-prefix in _normalize_token error message#722HumphreySun98 wants to merge 1 commit into
Conversation
The ValueError raised in _normalize_token() when a stop/forbidden token
does not map to a single token id used a plain string, so the {token!r}
placeholder was emitted literally instead of the offending token value,
making misconfigurations hard to debug.
Add the missing f-prefix and a regression test asserting the message
names the token.
Fixes google-deepmind#658
wbizmo
left a comment
There was a problem hiding this comment.
Cloned it and diffed against main.
_sampler.py gets the missing f prefix, _sampler_test.py has a test for it. Ran the string through both versions myself, old code literally prints {token!r} instead of the token, new one shows you 'multi_token'. Test regex catches the difference too, fails on the old string, passes on the new one.
Approving.
Thank you so much for your review! |
|
@Qwlouse gentle follow-up when you have a moment 🙂 Small, behavior-preserving fix for #658 — |
Summary
_normalize_token()ingemma/gm/text/_sampler.pyraises aValueErrorwhen a
stop_token/forbidden_tokenstring does not map to a singletoken id. The message string was missing its
fprefix, so{token!r}was printed literally instead of interpolating the offending token,
making these misconfigurations hard to debug.
Fixes #658.
Change
The second (implicitly concatenated) line has no placeholders, so it is
left unchanged.
Testing
Added
test_normalize_token_error_message_includes_token, which drives theerror path with a fake tokenizer and asserts the message names the token.
Verified it is a real regression guard — it fails on the pre-fix code
and passes with the fix.