fix(tokenizer): look up the decoding-fix dummy token instead of hardcoding its id - #2285
Open
eeshsaxena wants to merge 2 commits into
Open
fix(tokenizer): look up the decoding-fix dummy token instead of hardcoding its id#2285eeshsaxena wants to merge 2 commits into
eeshsaxena wants to merge 2 commits into
Conversation
eeshsaxena
requested review from
andyland,
k223kim and
lianakoleva
as code owners
July 15, 2026 09:07
Author
|
Closing to test the fix locally first and add a test case before resubmitting. |
Ids 33 and 165 are control tokens that decode to an empty string in the Mistral vocabularies, so the prefix-and-strip decoding fix did nothing there and single-token decoding dropped every leading space (Lightning-AI#1822). Fall back to looking \x1e up in the vocabulary, and drop the Mistral/Mixtral exemption from the per-token decode assertion.
Author
|
Ignore my earlier comment about closing this - I kept it open and reworked it instead, because the original diff was wrong. There is no The real cause is the hardcoded dummy token id in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1822.
Tokenizer.decodeworks around the SentencePiece leading-space stripping by prepending a token that decodes to\x1e, then slicing that prefix back off. The id was probed as 33, falling back to 165 for salamandra. In the Mistral vocabularies neither is right - id 33 is[control_31]and 165 is[control_163], both of which decode to an empty string, sodummy_tokenis"", the slice is a no-op, and every single-token decode loses its leading space. That is the run-on output in the issue.Repro against the real
mistralai/Mistral-7B-Instruct-v0.3tokenizer:The fix keeps the 33/165 fast path and otherwise looks
\x1eup in the vocabulary (it is id 31918 for Mistral v0.3). Llama is unaffected - it still resolves to 33 and produces identical output, checked against TinyLlama-1.1B-Chat-v1.0.test_tokenizer_against_hfhad aif not config.name.startswith(("Mistral", "Mixtral"))guard around the per-token decode assertion, which is exactly the case that was broken, so this removes it.I could not run the full test module locally (
lightningis not installed on this machine), so I verified by loadinglitgpt/tokenizer.pydirectly against tokenizer assets downloaded from the two HF repos above rather than by running pytest. CI should cover the rest.