Skip to content

npu: a Replace-only normalizer asks for no leading metaspace, so neither path may add or strip one (engine#2215) - #2226

Closed
bong-water-water-bong wants to merge 1 commit into
mainfrom
fix-2215-gemma-metaspace
Closed

bong-water-water-bong wants to merge 1 commit into
mainfrom
fix-2215-gemma-metaspace

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Fixes #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, because SentencePiece's add_dummy_prefix synthesises one.

The reader keyed on that prepend:

spm_ = has_meta && (merges.empty() || spm_prepends_meta(root["normalizer"]));

spm_prepends_meta() is true only for a Prepend ▁ 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_ = has_meta && (merges.empty() || prepends || replaces);
spm_prepend_meta_ = prepends || merges.empty();

spm() and spm_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 turned decode [▁] 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, via tokenizer_test:

golden before after
Gemma4-E2B-IT-NPU2 2/17 17/17
Gemma3-1B-NPU2 2/17 17/17
LFM2-1.2B-NPU2 17/17 17/17
Qwen3-0.6B-NPU2 17/17 17/17
Qwen3-1.7B-NPU2 17/17 17/17
Qwen3.5-4B-NPU2 17/17 17/17

The 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 the here▁is▁a▁Python output the issue reports.

Why this was invisible

tests/data/tokenizers/ holds goldens for 18 models, but they are registered against hf_tokenizer_test — the Rust tokenizer behind a C ABI — under two switches that are both off by default (ONEBIT_HF_TOKENIZERS OFF, ONEBIT_TOKENIZER_ROOT empty). 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 while main was green.

A follow-up worth taking (not in this PR, to keep it a reader fix): register tokenizer_test against 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.

…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.
@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

2215 - Partially compliant

Compliant requirements:

  • The NPU tokenizer now correctly identifies Replace-only normalizers
  • The encoding and decoding logic properly handles the metaspace symbol without incorrectly prepending or stripping it
  • The fix addresses the specific issue of incorrect tokenization for Gemma-family models
  • The fix is measured against the reference library and produces matching results

Non-compliant requirements:

  • None

Requires further human verification:

  • None
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incorrect metaspace handling in Replace-only normalizer

The tokenizer incorrectly handles the metaspace symbol when a Replace-only normalizer is used. In this case, the normalizer replaces spaces with the metaspace symbol without prepending one, but the tokenizer was still prepending a metaspace during encoding and stripping it during decoding. This caused incorrect tokenization and decoding behavior for Gemma-family models.

if (spm_prepend_meta_) s += kMeta;
Incorrect decoding of leading spaces

When decoding tokens, the tokenizer was stripping the first metaspace regardless of whether it was a dummy prefix or a real space. With a Replace-only normalizer, the first metaspace is a real space, so stripping it incorrectly converted decode ["\u2581"] into "" and caused two-space leads to become one.

if (strip_dummy_prefix && spm_prepend_meta_ && !out.empty() && out[0] == ' ') out.erase(0, 1);

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 only

A {"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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.)

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: CHANGES at 04cb6bc — the tokenizer hunk may be better than the one I already READY'd, and the other eight files are copies of four open PRs.

Every containment check is NO

git merge-base --is-ancestor  #2226 contains #2217: NO   #2195: NO   #2209: NO   #2227: NO

Ten files, +145/−17, and the code overlap is:

with shared
#2217 npu/tokenizer.cpp, npu/tokenizer.h
#2195 q4nx_forward.cpp, scripts/npu-window11.sh, tests/hyperloom_benchmark_test.py, tools/ci/unreferenced_py_tests.txt
#2209 q4nx_forward.cpp, scripts/npu-window11.sh
#2227 q4nx_forward.cpp, scripts/npu-window11.sh, tests/hyperloom_benchmark_test.py, tools/ci/unreferenced_py_tests.txt

No ancestry to any of them, so this is not a fold — a fold carries its parents (#2195 did exactly that, with both closed predecessors as ancestors, and it worked). This is four PRs' content re-typed, which means merging it lands changes that no reviewer of those four PRs approved in this form, and conflicts in five files with whichever of them merges first.

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:

"spm() and spm_merged() honour the flag when building the text, and the decoder needs the same distinction"

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 ▁-in-the-reply defect lives on the decode side. So the honest resolution is not "keep #2217": the two owners should settle which detection ships, and the winner should cover the decoder. Fixes #2215 cannot be claimed twice, and the issue will close on whichever lands first — taking the other's coverage with it.

What I would keep

Only 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 tests_referenced_check run and #2209's provenance table.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

/home/bcloud/.cache/scratch-pi-prs/c2226.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

npu tokenizer: a Gemma-family tokenizer.json encodes to the wrong ids (31 for the reference's 21) and replies keep the raw word-boundary symbols

1 participant