Repository navigation
npu: a Replace-only normalizer asks for no leading metaspace, so neither path may add or strip one (engine#2215) - #2226
bong-water-water-bong wants to merge 1 commit into
Conversation
…her path may add or strip one (engine#2215)
Gemma's tokenizer.json is a BPE with 514,906 merges whose normalizer is
{Replace " " -> U+2581} and whose pre_tokenizer is a Split on " " with
MergedWithPrevious -- after the replace there is no space left to split, so the
whole text is one pretoken and the pieces merge by rank over the metaspace
symbols. That is exactly spm_merged()'s work, except that spm_merged()
PREPENDS a metaspace for the whole text, because SentencePiece's
add_dummy_prefix synthesises one.
The reader keyed on that: spm_ only took a merged vocabulary when the
normalizer declared a Prepend (engine#1189's Phi-3.5 shape), so a Replace-only
normalizer fell through to plain BPE, which never introduces the metaspace at
all -- spaces stayed literal and encoded to byte-fallback ids.
So the prepend, not the merging, is what has to differ:
spm_ = has_meta && (merges.empty() || prepends || replaces)
spm_prepend_meta_ = prepends || merges.empty()
and both spm() and spm_merged() honour the flag when building the text. The
decoder needs the same distinction from the other side: it strips the first
metaspace as a dummy prefix, which is only correct when one was synthesised --
otherwise the first metaspace is a real space, and stripping it turned
decode [U+2581] into "" and a two-space lead into one.
Measured against the committed goldens, paired by sha256 with the reference
(tokenizers 0.23.3) on the same tokenizer.json:
Gemma4-E2B-IT-NPU2 before 2/17 after 17/17
Gemma3-1B-NPU2 before 2/17 after 17/17
LFM2-1.2B-NPU2 before 17/17 after 17/17 (regression check)
Qwen3-0.6B-NPU2 before 17/17 after 17/17 (regression check)
Qwen3-1.7B-NPU2 before 17/17 after 17/17
Qwen3.5-4B-NPU2 before 17/17 after 17/17
The first failing encode case before the fix was the smallest possible input:
encode " " wanted [236743] (the metaspace piece) and got [245237], which is the
here/is/a/Python output the issue reports.
Verified incrementally in an existing build tree (npu/tokenizer.{h,cpp} were
identical to main's before this patch): the goldens above are the verification.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 04cb6bc
This is the same fix as #2217 (Fixes the same issue #2215), on the same two files, with a different predicate name and no test — but it carries one thing #2217 does not, and that is what should survive.
Duplicate, measured:
#2226 npu/tokenizer.cpp npu/tokenizer.h (+34/−4, no test)
#2217 npu/tokenizer.cpp npu/tokenizer.h CMakeLists.txt docs/npu.md
tests/tokenizer_replace_meta_test.cpp tests/data/tokenizer_replace_meta.json (9 arms, fixture)
Both add a Replace-with-metaspace normalizer check called from the same place — spm_replaces_meta here, replaces_space_with_meta there. Landing both leaves two predicates answering one question, which is the shape that gets fixed in one place and not the other.
The one substantive difference, which I would carry into #2217 before closing this:
// #2226
const bool space = n.contains("pattern") &&
(n["pattern"].value("String", "") == " " ||
n["pattern"].value("Regex", "") == " "); ← Regex
// #2217
n["pattern"].value("String", "") == " " ← String onlyA {"Regex": " "} pattern is the same rewrite spelled differently, and #2217 would fall through to the byte-level path for it — the exact failure #2215 is about (31 ids where the reference gives 21). That is a real gap in #2217, and it is worth one line there rather than a second PR that also lacks the fixture.
Also worth noting, in favour of this PR's wording: "the text must not gain a LEADING metaspace — the normalizer never asks for one — so the prepend is what has to differ." That states the mechanism more precisely than #2217's docstring, which (as I noted on that PR) puts the "no Prepend" half of the contract inside a predicate that never looks for a Prepend. This sentence describes the caller's decision, which is where it belongs.
Recommendation: close this in favour of #2217, after porting the Regex alternative (and this comment's mechanism sentence) into it. If instead #2226 is preferred, it needs the fixture and the arms, which is the larger end of the work.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 04cb6bc: CHANGES
This duplicates #2217, which is open and READY at 248158e. #2217 fixes the same issue (Fixes #2215), in the same file (npu/tokenizer.cpp), with the same mechanism: a Replace-only metaspace normalizer goes down the merged-SentencePiece path with no dummy prefix added on encode and none stripped on decode. The two PRs conflict with each other (git merge-tree 248158e 04cb6bc), so only one can land.
They behave the same. I linked this head's libonebit_npu.a into the driver I used on #2217 and compared it with Hugging Face tokenizers 0.23.3. On every distinct tokenizer.json under ~/models/npu and ~/scratch-packs (21 files: the Gemma ones plus Qwen, Llama, MiniCPM, ChatGLM, OLMo, LFM, Hunyuan and others), 11 texts each (leading, double and trailing spaces, tabs and newlines, code, accents, CJK, emoji, a long run), this head matches the reference 11 of 11 on all 21 files, the same result as #2217.
What #2217 has and this does not: a ctest (tokenizer_replace_meta, built with onebit, with a nine-token fixture whose expected ids are the reference's). Its decode-prefix mutant fails 3 arms. This PR changes the reader with no test of its own.
Ask: close this in favour of #2217. Your follow-up idea, registering tokenizer_test against the committed tests/data/tokenizers/ goldens, gated on each golden's own sha256, is the valuable part here and is not in #2217. It would make a good small PR on top of it.
(CI has not run this head: GitHub Actions has started no job since about 20:20Z, see the board.)
|
VERDICT: CHANGES at Every containment check is NOTen files,
No ancestry to any of them, so this is not a fold — a fold carries its parents ( The tokenizer hunk is the part worth arguing about#2217: "True when the normalizer rewrites every space to the metaspace symbol and does NOT prepend one:
# a {Replace " " -> "\u2581"} step, bare or inside a Sequence, with no Prepend beside it."
#2226: "true when the normalizer REPLACES a space with the metaspace symbol without Prepend. Gemma's
# tokenizer.json is exactly this … the normalizer never asks for one — so the prepend is what has to differ."Same detection, two implementations. But #2226's body says something #2217's does not:
If the decoder carries the distinction, that is the more complete half — the ids and the visible string are decided at two ends, and the What I would keepOnly the tokenizer diff, reconciled with #2217, and the docs line if it is the one in #2197/#2209. Drop the other eight files: they belong to #2195, #2209 and #2227, where they have their own verdicts and their own proofs — including #2195's |
|
/home/bcloud/.cache/scratch-pi-prs/c2226.md |
Fixes #2215.
Gemma's
tokenizer.jsonis a BPE with 514,906 merges whose normalizer is{Replace " " -> U+2581}and whose pre-tokenizer is aSpliton" "withMergedWithPrevious. After the replace there is no space left to split, so the whole text is one pretoken and the pieces merge by rank over the metaspace symbols. That is exactlyspm_merged()'s work — except thatspm_merged()prepends a metaspace, because SentencePiece'sadd_dummy_prefixsynthesises one.The reader keyed on that prepend:
spm_ = has_meta && (merges.empty() || spm_prepends_meta(root["normalizer"]));spm_prepends_meta()is true only for aPrepend ▁step (engine#1189's Phi-3.5 shape). A Replace-only normalizer does not ask for one, so gemma fell through to plain BPE — which never introduces the metaspace at all — and spaces encoded to byte-fallback ids.The fix: the prepend differs, not the merging
spm()andspm_merged()honour the flag when building the text, and the decoder needs the same distinction from the other side: it strips the first metaspace as a dummy prefix, which is correct only when one was synthesised. With a Replace-only normalizer the first metaspace is a real space, and stripping it turneddecode [▁]into""and a two-space lead into one.Measured against the committed goldens
Paired by sha256 with the reference (tokenizers 0.23.3) on the same
tokenizer.json, viatokenizer_test:Gemma4-E2B-IT-NPU2Gemma3-1B-NPU2LFM2-1.2B-NPU2Qwen3-0.6B-NPU2Qwen3-1.7B-NPU2Qwen3.5-4B-NPU2The last four are the regression check: the flag is shared, so a family that already matched had to keep matching.
The first failing encode case before the fix was the smallest possible input —
encode " "wanted[236743](the metaspace piece) and got[245237], which is thehere▁is▁a▁Pythonoutput the issue reports.Why this was invisible
tests/data/tokenizers/holds goldens for 18 models, but they are registered againsthf_tokenizer_test— the Rust tokenizer behind a C ABI — under two switches that are both off by default (ONEBIT_HF_TOKENIZERS OFF,ONEBIT_TOKENIZER_ROOTempty). The engine's own reader (tokenizer_test,onebit_npu) — the one that serves the NPU route — is registered against one fixture,tests/data/spm_bpe_tokenizer.json. So the goldens validated a different implementation, and two committed gemma goldens failed the shipping reader by 15 of 17 cases whilemainwas green.A follow-up worth taking (not in this PR, to keep it a reader fix): register
tokenizer_testagainst the same goldens, gated on each golden's own named sha256 so a mismatched pack is a skip with a printed reason rather than a fail on the wrong file. That is what would have caught this without the issue.