Repository navigation
Match Hugging Face whitespace splitting in the Llama 3 pre-tokenizer - #1138
Open
Yuri Khrustalev (ykhrustalev) wants to merge 2 commits into
Open
Yuri Khrustalev (ykhrustalev) wants to merge 2 commits into
Yuri Khrustalev (ykhrustalev) wants to merge 2 commits into
Conversation
In \s*[\r\n]+, the regex engine backtracks \s* to the last \r or \n of a whitespace run, so " \n x" splits as " \n", " ", " x". The matcher only handled runs that end in a newline, or took just the leading newlines of a run. So the split changes only for a whitespace run that has a newline after a space or tab and more whitespace after its last newline: a whitespace-only line (a blank line that keeps its indentation, as in code), or trailing spaces before an indented line. Empty blank lines already match on main. For example, qwen2.5 on "def f():\n x = 1\n \n return x" gives [..., 16, 198, 1066, 262, ...] on main and [..., 16, 7213, 262, ...] with this change, the same as Hugging Face tokenizers.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
3 of 9 tasks
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused correction matches the intended regex behavior, and the regression test covers both reported failures.
No open issues 🎉
What changed in this PR
Corrects Llama 3 pre-tokenizer whitespace splitting to match Hugging Face behavior.
Changes:
- Matches whitespace through the last carriage return or newline in a run.
- Adds a regression test covering both reported splitting failures.
| File | Description |
|---|---|
| test/pp_api_test/test_tokenizer_impl.cc | Adds whitespace-splitting regression coverage. |
| operators/tokenizer/bpe_utils.hpp | Corrects newline matching within whitespace runs. |
Review effort: Balanced
Contributor
|
/azp run onnxruntime-extensions.CI |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
kunal-vaishnavi
left a comment
kunal-vaishnavi
left a comment
Contributor
There was a problem hiding this comment.
Can we also address the Phi-3 bug that you mentioned in the PR description?
BpeModel::Load removed every '\r' from the string merges of
tokenizer.json, as it does for the lines of a merges.txt file. A JSON
string is not a text line, so a '\r' there is part of a token. The
merges with a '\r' (40 in the Llama 2 / Phi-3 tokenizer.json, 740 in
Gemma, 24 in ChatGLM) were lost, and some of them overwrote the rank of
an existing merge: "▁ {\r" became "▁ {". For example, phi-3-mini gave
[263, 13, 29871, 30004] for "a\n \r" where Hugging Face gives
[263, 13, 6756].
The merges.txt path, which the merges attribute of the ONNX tokenizer
ops also uses, still removes '\r' from each line.
Contributor
Author
|
Done in 2bb7f4b: string merges from tokenizer.json now keep their |
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.
Problem
In
\s*[\r\n]+, part of the Llama 3 split regex (also used by Qwen2.5, Qwen3, Phi-4, SmolLM3, gpt-oss, LFM2.5 and others), the regex engine backtracks\s*, so the match ends at the last\ror\nof a whitespace run:"x\n \n y"x,\n \n,yx,\n,\n,y"y \n z"y,\n,zy,\n,zMatch_LLAMA3_Pattern_5only matched a run that ends in a newline, or took just the newlines at the start of a run. So the split differs only for a whitespace run that has a newline after a space or tab and more whitespace after its last newline: a whitespace-only line (a blank line that keeps its indentation, as in code), or trailing spaces before an indented line. Empty blank lines already match on main. For example, qwen2.5 on"def f():\n x = 1\n \n return x":[..., 16, 198, 1066, 262, ...][..., 16, 7213, 262, ...]Fix
Match the whitespace run up to its last
\ror\n, as the regex does. New testLlama3RegexWhitespaceTest(expected output from Hugging Facepre_tokenize_str); it fails on main.Second commit, for a Phi-3 string that the first commit alone moved from right to wrong:
BpeModel::Loadremoved every\rfrom the string merges of tokenizer.json, as it does for the lines of a merges.txt file. A JSON string is not a text line, so the\ris part of the token. The merges with a\r(40 in the Llama 2 / Phi-3 tokenizer.json, 740 in gemma, 24 in chatglm) were lost, and 17 of them overwrote the rank of an existing merge (▁ {\rbecame▁ {, which moved▁ {from rank 224 to 6776). Now they are kept. The merges.txt path, which the merges attribute of the ONNX tokenizer ops also uses, is unchanged and still removes\rfrom each line. New testCarriageReturnInJsonMerges(phi-3-mini, expected ids from Hugging Face); it fails on main and on the first commit alone. For example,"a\n \r":[263, 13, 29871, 30004][263, 13, 6756]Testing
Built with
./build.sh -DOCOS_ENABLE_C_API=ONon Linux (Ubuntu 22.04, gcc 11.4, x86_64) and macOS (27.0.1, Apple clang 21, arm64). The numbers are the same on both machines unless a line says otherwise.pp_api_testfails only inImageDecoderTest.TestJpegEncoderDecoder, the same as on main.\rin a string merge (chatglm, gemma, llama2, phi-3, phi-3-mini / medium / vision), with 0 right -> wrong: 1683 / 1651 strings for phi-3-mini (differ from HF 6905 / 6915 with the first commit alone, 5297 / 5337 with both), 3678 / 3582 for gemma, 932 / 933 for chatglm. It also fixes the phi-3 string"...0\n \r\x0b..."that the first commit alone moved from right to wrong.Not changed here: U+0085 (NEL) is
\sfor Hugging Face but not for these matchers. For LFM2.5-2.6B and LFM2.5-8B-A1B, all 461 / 482 remaining differences contain U+0085. Treating U+0085 as whitespace too fixed 461 to 525 more strings for each of qwen2.5, phi-4-mini, phi-4-base, phi-3-small-cvt and LFM2.5-2.6B / 8B-A1B, but moved 7 / 2 phi-3-mini strings from right to wrong, so it is left out.