feat(tokenizer): recognize and split the Mistral Tekken pre-tokenizer (#168) - #283
feat(tokenizer): recognize and split the Mistral Tekken pre-tokenizer (#168)#283filipsajdak wants to merge 1 commit into
Conversation
…mudler#168) mistralai/Mistral-Nemo-Instruct-2407 dies at DetectPattern with "unrecognized pre-tokenizer split regex". Its regex is not a variant of anything already here: it is tiktoken's o200k_base pat_str with exactly two edits -- the optional (?i:'s|'t|'re|'ve|'m|'ll|'d)? group deleted from both letter alternatives, and \p{N} instead of \p{N}{1,3}. Everything from the punct run onward is byte-equal to o200k_base, so kTekken puts the GPT-4o-family scanner in the tree; an o200k arm later is two flags on this matcher, not a second implementation. Worth stating because it is easy to assume otherwise: the existing Mistral gate (tests/parity/goldens/tokenizer_mistral) is Mistral-7B-v0.3, a SentencePiece Metaspace tokenizer with byte-fallback. It is a different family entirely, so Tekken was genuinely unrepresented rather than a near-neighbour of it. Four differences from the shared Qwen/Llama-3 scanner. Three are flags: no contraction alternative, max_digits=1, and '/' in the punct run's trailing class ([\r\n/]* rather than [\r\n]*). The fourth splits an existing flag -- Tekken carries \p{M} INSIDE both letter classes (like kQwen2) while its punct negation is [^\s\p{L}\p{N}] with no \p{M} (like kLlama3), a combination no other pattern needs, so marks_aware becomes marks_in_run + marks_excluded. The fifth difference is the real work, and the place a bug would hide. The two letter classes OVERLAP on {Lm, Lo, M}, so [U]*[L]+ genuinely backtracks and cannot be MatchLetterRun's single-predicate scan. MatchTekkenAlt records the give-back positions and walks them longest-first; MatchTekkenLetterRun mirrors the engine's ORDERED alternation -- alt 1 with prefix, alt 1 without, alt 2 with prefix, alt 2 without -- rather than taking longest-match. Ordered, not longest, is the part worth a reviewer's eye. Expressing those classes needs letter CASE, which UCat cannot give: it collapses every L* into kLetter. Rather than re-cut kCategoryRanges into finer categories, which would touch every existing consumer to serve one new pattern, this adds a SECOND, narrow table -- LetterCase over 1861 ranges, Lu/Lt -> kUpper, Ll -> kLower, Lm/Lo -> kCaseless, non-letters omitted so a miss means "not a cased letter". Marks are deliberately absent; a caller needing \p{M} in both classes asks Category(), because M is not a letter subcategory and folding it in would make the table lie about what it is. Both tables come from the same generator at the same pinned Unicode version, so they cannot drift apart: regenerating under Python 3.12 (unidata 15.0.0, the version the committed tables were built with) reproduces kCategoryRanges' 1563 ranges and kWhitespaceRanges' 10 ranges BYTE-IDENTICALLY, verified by cmp, sha256 and a clean `git status`. Verified against the HF tokenizers oracle rather than by reasoning: - pretokenizer_goldens.inc gains a third column from gen_pretok_goldens.py. All 108 pre-existing rows keep their exact qwen/llama columns; 8 rows are added for the case split, Lt/dotted-Lu, the mark combination and the '/' tail. - The oracle itself was validated first: this tokenizers is 0.23.1 and the committed goldens were generated with 0.22.2, so the EXISTING table was regenerated and diffed byte-identical before a column was added. The same check on unicode_data caught that no Python to hand shipped unidata 15.0.0 (3.14 -> 16.0.0, 3.13 -> 15.1.0). - test_pretokenizer: 23/23 cases, 101714 assertions, standalone and in-suite. - Round-trip on the real checkpoint against HF tokenizers: 5000 fuzz strings / 90659 token ids plus 45 hand-built edge cases, encode ids and decode both byte-exact. - Linux aarch64, gcc 13.3 / libstdc++: the tokenizer set is 7/7, with test_tokenizer_parity_mistral (SentencePiece) and test_tokenizer_parity_deepseek (the seven-stage pipeline) unchanged -- the regression evidence for the shared scanner and the new table. No vocab fixture is committed: the pre-tokenizer gate needs none, so the round-trip evidence above comes from the real 9.26 MB tokenizer.json without carrying it in-tree. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [ClaudeCode]
7fc1e25 to
26e00db
Compare
|
| this PR | main @ e17e8f8a |
|
|---|---|---|
| failing tests | 9, 44, 59, 234, 238 | the same five, same numbers |
test_load_direct_upload, test_llama_embedding_fold, test_laguna_nvfp4_loader, test_openai_api_server, test_capi |
identical | |
| UBSan sites | test_load_direct_upload.cpp:177:48, src/vt/cpu/cpu_ops.cpp:33:70, src/vllm/model_executor/models/laguna.cpp:1028:56 |
identical file:line:column |
All are 2-byte-alignment violations on short unsigned int — load of misaligned address at the two cpu_ops.cpp / laguna.cpp sites, and reference binding to misaligned address at test_load_direct_upload.cpp:177 — in files this PR does not touch. test_pretokenizer itself passes clean under ASan/UBSan.
Not fixing them here: that would bundle unrelated work into a tokenizer change.
The other two reds were mine, and are fixed
pr-size and agent-record failed the first run with base must be an ancestor of head. I branched from 7c7f024, main moved 37 commits, and the PR's base.sha was therefore not an ancestor of the head — so both gates aborted before doing any work, which is why their logs looked like checkout noise. Rebased onto 2430636; the diff is unchanged at +1017/-15 across the same 10 files.
To be exact about what I can and cannot claim: the cause is removed — the head is now a descendant of the base — but I have not seen those two gates go green, because the workflows on the current head are sitting at action_required and have not run. That is presumably the fork-PR approval gate; the first push was approved and ran all 15 checks, and the force-push minted a new SHA that needs approving again. Flagging it only so the absent checks are not mistaken for a stuck queue.
Two things that may be worth knowing beyond this PR:
- With
mainadvancing this fast, those two gates can go red purely on base ancestry while nothing is wrong with the change. Rebasing immediately before a merge attempt avoids the confusing red. - That remedy has a cost here, though: each rebase mints a new SHA and re-gates the workflows behind another approval. Worth weighing before asking a contributor to rebase.
|
Merged — closes #168. This is a genuinely careful port, and the two seam decisions are the right ones. Your red Two things I verified rather than took on trust, because both are the kind of claim that's easy to assert and expensive to be wrong about:
The part I most appreciated: you widened two seams instead of bending them.
|
… pre-tokenizer (#283) Closes #168. External contribution from Filip Sajdak (@filipsajdak). The PR's red agent-record and pr-size checks were a FORK ARTIFACT, not a defect: both checkers refused with "base must be an ancestor of head" because the branch had not been rebased onto current main, so neither could compute a commit range at all. Every substantive job -- build, cuda-fat- build, build-test-cpu, build-test-cpu-arm64, build-test-vulkan, device-leakage, documentation-checkpoint, commit-protocol-tag -- was green. The sanitize-cpu (address,undefined) failure is the pre-existing main baseline (test_load_direct_upload, test_llama_embedding_fold, test_laguna_nvfp4_loader, test_openai_api_server, test_capi), identical on every open PR. Merging locally supplies the ancestry those two checkers wanted. Tekken is the first pattern here whose letter rule is CASE-AWARE. It is tiktoken's o200k_base pat_str with exactly two edits -- no (?i:'s|'t|...) contraction group, and single-codepoint \p{N} instead of \p{N}{1,3} -- so an uppercase run ends a piece when a lowercase run follows ("HelloWorld" -> "Hello" + "World"). Because the two letter classes OVERLAP on {Lm, Lo, M}, this needs genuine ordered-alternation backtracking and could not fold into MatchLetterRun's single-predicate scan; MatchTekkenAlt walks the greedy give-back stops longest-first, which is exactly what the regex engine picks. Two seams widened rather than bent: - A separate narrow LetterCase table (Lu/Lt=upper, Ll=lower, Lm/Lo in BOTH), because UCat collapses all of L* into kLetter and cannot express the split. UCat and every existing consumer keep their exact bytes. - The single marks_aware flag splits into marks_in_run and marks_excluded. Tekken carries \p{M} inside its letter classes (like kQwen2) while its punct negation omits \p{M} (like kLlama3) -- a combination no other pattern has, which is why one flag sufficed until now. Both derived flags evaluate identically to the old one for kQwen2 and for every other pattern, so this is non-regressive by construction. Verified before merging rather than taken on trust: - tools/gen_unicode_data.py reproduces the checked-in include/vllm/tokenizer/unicode_data.h and src/.../unicode_data.cpp BYTE-FOR-BYTE (md5 identical after a local regen, unidata 15.0.0, 1861 letter-case ranges). The generated table is not hand-edited. - The goldens are oracle-derived, not authored: gen_pretok_goldens.py drives the real HF tokenizers Split pre-tokenizer with the regex quoted verbatim from mistralai/Mistral-Nemo-Instruct-2407 tokenizer.json, over ~90 cases including 60 seeded random strings. - DetectPattern keys on an exact regex-string match, so a checkpoint that does not match falls through to today's behavior. No existing model can change class. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [ClaudeCode]
… ROCm (#234) External contribution from Don Mirror (@bakon11). Lab evidence: dual R9700 gfx1201. The bug it closes: ROCm registered no sampling ops at all, so EngineCore fatally hit "no kernel for op" immediately after prefill on AMD. Every V1 sampler op is now registered for DeviceType::kROCM -- temperature, top-k/top-p, probs, logprobs, random sample, penalties, min-p, logit bias, allowed-token-ids and the sparse bad-words token mask. The PR's red agent-record and pr-size checks were the same FORK ARTIFACT as #283: both refused with "base must be an ancestor of head" because the branch was never rebased, so neither could compute a range. sanitize-cpu (address,undefined) is the pre-existing main baseline. No substantive job was red, and no CI job compiles ROCm at all (there are no AMD runners), so the green checks are orthogonal either way. Because CI cannot compile or run this, I verified the port mechanically rather than by reading it. src/vt/rocm/rocm_sample.hip was normalized against src/vt/cuda/cuda_sample.cu (namespace, hip/cuda prefixes, kernel suffixes and whitespace folded away) and every remaining difference is cosmetic: entry-point names, error-string prefixes, line wrapping, and CUDA's trailing explanatory comments. The numerics are IDENTICAL, including the pieces that decide token identity -- SplitMix64, ExpNoise's (r >> 11) + 1 over 9007199254740993.0 mantissa construction, the gumbel/exp-noise argmax, the temperature's !all_random && t < kSamplingEps guard, and flashinfer's two-pivot sort-free bracket search with the same kThreshMaxIter = 64 and the same min_gt_low / max_le_high snapping. This is a genuine 1:1 port, not a reimplementation, which is what the porting rule requires. Reviewed and accepted as-is: RandomSampleK launches <<<n, 1>>>, one thread per row scanning the whole vocab serially. That is not a defect here -- cuda_sample.cu launches it exactly the same way, so the port is faithful and a change would be a divergence. It is a real optimization target for whoever takes ROCm sampling past correctness-grade. Tests skip cleanly via HasRocm() when no ROCm backend is registered, so they are inert on CPU CI and assert real CPU-vs-ROCm parity on AMD hardware. docs/USAGE.md's non-positive max_tokens claim is backed by code already on main (protocol.cpp:525), not by anything this PR asserts without landing. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [ClaudeCode]
Main gained the GPT-4o / o200k pre-tokenizer reconciled with Tekken (#347, #168, #369), the Muse Glimmer Q/K RoPE converter fix (#359), and a doc-tables ratchet test while this branch was being gated. Clean merge — the tokenizer work is disjoint from everything here because #283 (Tekken) was dropped from this branch as already landed, and the rest of this branch is ROCm kernels, Tenstorrent, the CUDA feature table and the serving SSE/tool-parser seams. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [ClaudeCode]
|
Landed on main as Closing manually: it landed as a rebased copy of this branch, so the head SHA here never became reachable from main and GitHub could not auto-close it. Nothing is outstanding; the PR was just left showing OPEN. Thanks for the contribution. |
Adds
SplitPattern::kTekkensomistralai/Mistral-Nemo-Instruct-2407loads instead of dying attokenizer: unrecognized pre-tokenizer split regex. Refs #168.First: why you can trust the generated table
This adds 1,861 generated Unicode ranges. The reasonable first question is whether that table is right, so it was answered before anything was built on it — by regenerating your existing artefacts and requiring byte-identity, twice:
tokenizerspretokenizer_goldens.inc: byte-identicalunicodedataunicode_data.{h,cpp}: byte-identicalBoth checks mattered. Nothing I had shipped Unicode 15.0.0 by default — 3.14 gives 16.0.0, 3.13 gives 15.1.0 — so without pinning, this PR would have quietly placed a 16.0.0 table beside your 15.0.0 one and every check would still have looked green. Byte-identity on the artefacts you already have is the only thing that catches that.
Verified with
cmp,sha256and a cleangit status, not by eye.Second: this cannot regress what works
The change is additive by construction:
UCatis untouched.kCategoryRanges(1,563) andkWhitespaceRanges(10) come out byte-identical after the regeneration.LetterCaseis a second, narrow table, not a re-cut of the first — re-cutting would have put four working patterns at risk to serve one new one.qwen/llamacolumns. 8 rows are added, and only added, for the new behaviours.SplitPatternchanges behaviour.marks_awaresplits intomarks_in_run+marks_excluded, but every existing pattern sets both to the value it had.tokenizer.jsonstays out of a tree whose largest is 1.96 MB.test_tokenizer_parity_mistral(SentencePiece) andtest_tokenizer_parity_deepseek(the seven-stage pipeline) pass unchanged. Those two are the real regression evidence for the shared scanner and the new table.Third: the arm, and the part worth your attention
Tekken is tiktoken's
o200k_basepat_strwith exactly two edits: the optional(?i:'s|'t|'re|'ve|'m|'ll|'d)?group deleted from both letter alternatives, and\p{N}instead of\p{N}{1,3}. Everything from the punct run onward is byte-equal. So this puts the GPT-4o-family scanner in the tree; ano200karm later would be two flags on the same matcher.Three of the four differences from the shared Qwen/Llama-3 scanner are flags — no contraction alternative,
max_digits = 1, and/in the punct run's trailing class. The fourth splitsmarks_aware, because Tekken carries\p{M}inside both letter classes (likekQwen2) while its punct negation has no\p{M}(likekLlama3), which no other pattern needs.The fifth is where a bug would hide, so it gets named rather than buried. The two letter classes overlap on
{Lm, Lo, M}, so[U]*[L]+genuinely backtracks and cannot beMatchLetterRun's single-predicate scan.MatchTekkenAltrecords the give-back positions and walks them longest-first;MatchTekkenLetterRunmirrors the engine's ordered alternation — alt 1 with prefix, alt 1 without, alt 2 with prefix, alt 2 without — rather than taking longest-match. Ordered, not longest, is the assumption to check.Also worth flagging, since it is easy to assume otherwise: your existing Mistral gate is
Mistral-7B-v0.3, a SentencePiece Metaspace tokenizer with byte-fallback. Different family entirely — Tekken was genuinely unrepresented, not a near-neighbour of something covered. (I got this wrong in #168 and have corrected it there.)Evidence
test_pretokenizer: 23/23 cases, 101,714 assertions — standalone and in-suite. Both are stated because test_qwen36_spec_decode: token drift in-suite, passes standalone 11/11 — filed as 'suite-context-dependent' but the decisive arm was never run #247 records a gate that passes standalone and drifts in-suite, so the standalone number alone would not have answered it.tokenizerslibrary onMistral-Nemo-Instruct-2407: 5,000 fuzz strings / 90,659 token ids, plus 45 hand-built edge cases — encode ids and decode both byte-exact, 100%. Both directions, not just "it splits".test_pretokenizer,test_tokenizer_parity,_mistral,_deepseek,test_unicode_data,test_tokenizer_metaspace_split,test_detokenizer). Run on gcc specifically because libc++ tolerates missing transitive includes that libstdc++ rejects, and this change adds a<vector>dependency.Full suite on macOS: the delta from this change is zero
365 tests, 97% passed, 11 failed — and all 11 reproduce identically on unmodified
7c7f024. The baseline was re-run in a pristinegit archivetree with its own build directory, rather than a stash, so nothing of mine could leak into it:7c7f024test_kv_offload_fstest_kv_offload_tieringtest_kv_offload_connectortest_lmcache_clienttest_lmcache_connectortest_serve_low_toolstest_safetensorstest_load_direct_uploadtest_none_hash_determinismtest_ops_quant_dottest_cpu_kernel_bench_cliSame 11 red before and after,
SEGFAULTincluded. The fiveNot Runare macOS build failures in files this change does not touch (<unistd.h>/getpid,::htonlagainst the macOS macro, and one-Wunused-function);test_serve_low_toolsis already noted as red onmainin #233. Recorded here as the regression evidence for a Unicode-table change, not as a bug report — the fixes are not bundled into this PR.Your own gates, run locally before opening:
check-commit-trailers,check-doc-checkpoint,check-now-current,check-pr-size,check-public-doc-tables— all exit 0.Reproducing the goldens
Happy to split this into the table change and the arm if you would rather review them separately — I kept it as one because the table has no consumer without
kTekken.