Repository navigation
unicode: add custom splitter for k2-horizon pre-tokenizer - #3
Closed
mohitsoni48 wants to merge 1 commit into
Closed
mohitsoni48 wants to merge 1 commit into
mohitsoni48 wants to merge 1 commit into
Conversation
The k2-horizon pre-tokenizer regex is a superset of llama3's: its word-run
group also admits combining marks (\p{M}) and the ZWNJ/ZWJ joiners
(\u200C/\u200D), which matter for scripts like Devanagari. Since that
exact regex string wasn't registered in unicode_regex_split_custom, it
fell through to the generic std::regex/std::wregex path, which cannot
compile \u200C/\u200D escapes at all (any platform, not just MSVC) and
aborts model loading with:
Failed to process regex: '...'
Regex error: regex_error(error_escape): ...
Add unicode_regex_split_custom_k2_horizon (adapted from the llama3
splitter, extending the word-run predicate to include \p{M}/ZWNJ/ZWJ) and
register it for k2-horizon's exact regex string so it uses the same
non-regex code path as llama3/gpt2/qwen2/kimi_k2/afmoe.
Author
|
Duplicate of #1 — I independently ran into and root-caused the same bug, and landed on the identical fix (same custom splitter, same letter-run predicate extension). #1 was opened first and its test case (real Persian ZWNJ text) is better than my synthetic cases, so closing this in favor of that one. Leaving a verification note on #1. |
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
Loading a K2 Horizon / MoVA GGUF (
tokenizer.ggml.pre == "k2-horizon") fails at vocabularyload time with:
LLAMA_VOCAB_PRE_TYPE_K2_HORIZONis correctly registered inllama-vocab.cpp, so this isn'tan "unknown pre-tokenizer type" issue. The problem is one level down: the k2-horizon regex
string isn't registered in
unicode_regex_split_custom(unlike llama3/gpt2/qwen2/qwen35/kimi_k2/afmoe), so it falls through to the generic
std::regex/std::wregexpath inunicode_regex_split. That path does pre-collapse known\p{...}categories before handingthe pattern to
std::regex, but it has no handling for the literal/(ZWNJ/ZWJ) escapes in the pattern — and
std::regex's ECMAScript grammar doesn't support\ucode-point escapes on any platform, not just MSVC — so compilation always throwserror_escape.This reproduces on Linux/GCC too, not just Windows/MSVC (confirmed with a standalone harness
against
unicode_regex_splitdirectly, independent of#17830's Windows-only regression).Fix
The k2-horizon regex is otherwise identical in shape to the llama3 pattern — same
contraction handling, same
\p{N}{1,3}grouping, same trailing whitespace/punctuationrules — except its word-run group is
(?:\p{L}|\p{M}||)+instead of\p{L}+(it also treats combining marks and the ZWNJ/ZWJ joiners as part of a word, which matters
for scripts like Devanagari that rely on them).
This adds
unicode_regex_split_custom_k2_horizon, adapted directly fromunicode_regex_split_custom_llama3with the word-run predicate widened tois_letter || is_accent_mark || cpt in {0x200C, 0x200D}, and registers it inunicode_regex_split_customfor the exact k2-horizon regex string — routing it through thesame non-regex custom-splitter path as the other known tokenizer families, instead of the
std::regexfallback.Testing
tests/test-unicode.cppwith a small case table covering the k2-horizon regex:leading-space attachment, case-insensitive contraction splitting, a combining mark staying
attached to its base letter, a ZWJ keeping two letters joined into one token, and digit
grouping into chunks of ≤3.
(
Failed to process regex/regex_error(error_escape)), and passes after the fix.tests/test-unicode.cppisn't built as part of the default Windows (shared-lib) CMakeconfig here (guarded by
NOT WIN32 OR NOT BUILD_SHARED_LIBSintests/CMakeLists.txt,since
unicode_regex_splitisn't exported viaLLAMA_API), so I also verified by compilingunicode.cpp+unicode-data.cpp+ the test directly withcl.exestandalone. On anon-Windows / static-lib CI config this will build as
test-unicodeas usual.llama-server.exefrom this branch with the fix and confirmed a clean compile/link.No behavior changes for any other pre-tokenizer type — this only adds a new branch to
unicode_regex_split_custom's string dispatch and a new standalone splitter function.