Repository navigation
Strip the metaspace prefix space only when it is unambiguous - #47862
Open
ArthurZucker wants to merge 2 commits into
Open
ArthurZucker wants to merge 2 commits into
ArthurZucker wants to merge 2 commits into
Conversation
`Metaspace` only prepends `▁` when the text does not already start with one, so with two
or more leading spaces there is no synthetic prefix at all and the trailing
`Strip(content=" ", left=1)` decoder removed one of the user's own spaces:
" hello" -> ▁▁hello -> " hello" -> Strip -> " hello"
Encoding was already lossless here; only the decoder was wrong. Replacing the
unconditional strip with `Replace(Regex(r"\A (?! )"), "")` removes the prefix space only
when there is exactly one, which is precisely the case where it is synthetic.
Encoding is untouched, so ids, added tokens and shipped checkpoints are unchanged, and
the decoder still serializes into `tokenizer.json` so the other bindings get the fix too.
`\A` rather than `^` because the regex engine is multiline and `^` would eat the
indentation of every line rather than just the first.
A single leading space stays unrecoverable: `" hello"` and `"hello"` encode to the same
ids, which is the documented SentencePiece behaviour and is not changed here.
Fixes #47487
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
2 of 4 tasks
ArthurZucker
commented
Aug 27, 2026
ArthurZucker
commented
Aug 27, 2026
ArthurZucker
left a comment
ArthurZucker
left a comment
Collaborator
Author
There was a problem hiding this comment.
I just need to clear the AI slope from it before I can merge!
Contributor
|
[For maintainers] Suggested jobs to run (before merge) run-slow: code_llama, llama |
Contributor
CI recapDashboard: View test results in Grafana
|
This branch has not been deployed
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.
What does this PR do?
Fixes #47487 without touching the encoding pipeline. Follow-up to #47861, which reverted #47488 — this is the version of that fix which does not change any ids.
The actual bug
Metaspaceonly prepends▁when the text does not already start with one. So with two or more leading spaces there is no synthetic prefix at all, and the trailingStrip(content=" ", left=1)removed one of the user's own spaces:Encoding was already lossless here — only the decoder was wrong. That is why this does not need a pipeline change.
The fix
Remove the leading space only when there is exactly one, which is exactly the case where it is synthetic. The count of leading
▁separates the two cases perfectly, because the prepend is guarded by that samestarts_withcondition.\Aand not^: the regex engine is Oniguruma and multiline, so^eats the indentation of every line, not just the first.test_integrationcatches it if you get this wrong.Why this is safe
hf-internal-testing/llama-tokenizer,NousResearch/Llama-2-13b-hfandcodellama/CodeLlama-7b-hf: encode ids are identical tomainon every probe, including text containing special tokens. No id changes, no added-token changes, no checkpoint changes.tokenizer.json, so tokenizers-rs and transformers.js get the fix too — unlike a Python-side_decodeoverride.A single leading space stays unrecoverable:
" hello"and"hello"both encode to[22172]. That is documented SentencePiece behaviour and is deliberately unchanged; #47487 asks for it to stay that way too.Tests
test_decode_keeps_real_leading_whitespaceadded for both models. Each fails onmain(' hello' != ' hello') and passes here. It also pins the two things worth not regressing: text with no leading whitespace must not gain a space, and later lines must keep their indentation.tests/models/{llama,code_llama,moshi,helium}— 182 passed, 33 skipped.The same
Stripappears three times inconvert_slow_tokenizer.py(Llama, Moshi, Helium converters) and is swapped there too. The threeggml.pysites are deliberately left alone: two of them have noFusebefore the strip, so they already strip per token ("▁def", "▁f"->deff) — a separate pre-existing bug in the GGUF path, worth its own issue.Who can review?
@itazap