Skip to content

Fix missing f-prefix in _normalize_token error message - #722

Open
HumphreySun98 wants to merge 1 commit into
google-deepmind:mainfrom
HumphreySun98:fix/normalize-token-error-message
Open

Fix missing f-prefix in _normalize_token error message#722
HumphreySun98 wants to merge 1 commit into
google-deepmind:mainfrom
HumphreySun98:fix/normalize-token-error-message

Conversation

@HumphreySun98

Copy link
Copy Markdown

Summary

_normalize_token() in gemma/gm/text/_sampler.py raises a ValueError
when a stop_token / forbidden_token string does not map to a single
token id. The message string was missing its f prefix, so {token!r}
was printed literally instead of interpolating the offending token,
making these misconfigurations hard to debug.

Fixes #658.

Change

-        'Invalid token: {token!r}. `stop_token`s and `forbidden_token`s must'
+        f'Invalid token: {token!r}. `stop_token`s and `forbidden_token`s must'

The second (implicitly concatenated) line has no placeholders, so it is
left unchanged.

Testing

Added test_normalize_token_error_message_includes_token, which drives the
error 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.

$ python -m pytest gemma/gm/text/_sampler_test.py -q
5 passed

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@HumphreySun98

Copy link
Copy Markdown
Author

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!

@HumphreySun98

Copy link
Copy Markdown
Author

@Qwlouse gentle follow-up when you have a moment 🙂 Small, behavior-preserving fix for #658_normalize_token was missing its f-prefix, so the error printed the literal {token!r} instead of the offending token. All checks green, cleanly mergeable, and covered by a regression test. Happy to rebase or adjust anything. Thanks for taking a look!

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.

Bug: Missing f-prefix in error message in _normalize_token() makes stop_tokens debugging impossible

2 participants