Improve robustness for handling Python-authored regexes#24
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves JavaScript compatibility for tokenizer regex patterns coming from Python/Rust by sanitizing problematic escapes, normalizing shorthand classes for Unicode behavior, and adding targeted edge-case tests.
Changes:
- Expand escape sanitization for certain identity escapes before compiling
RegExpin JS. - Normalize
\w/\W/\d/\Dinto Unicode-aware equivalents for better multilingual tokenization behavior. - Add an edge-case test to validate quote-class regex compilation and Unicode
\w+matching (Hebrew).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/utils/core.ts |
Adjusts regex sanitization and introduces Unicode shorthand normalization + boundary warnings during pattern compilation. |
tests/edgeCases.test.ts |
Adds regression tests for quote-class compilation and multilingual \w+ matching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
wow thanks @nico-martin
|
|
friendly ping @nico-martin :) |
|
Hi @eyaler,
|
|
thanks for tackling this! For example, this PR (huggingface/huggingface.js#1418) which fixed jinja templates for top 100k templates on the HF hub was done using existing templates as test cases. I'll write a script which does the same for regexes. |
|
okay, it took a couple of hours to run, but after downloading ~32k tokenizer.json files, there only appear to be ~105 unique regexes? (this seems quite strange tbh... I'll choose to believe it for now) maybe check if these are all parsed correctly (and match python implementation) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
and tbh, this could work well to make |
|
Snuck a full Fable 5 review in before it was shut down 😆 comment attached below Correctness fixes (all 8, in src/utils/core.ts)
Bonus fixes that fell out of the probes: \s/\S → \p{White_Space} (JS \s wrongly adds U+FEFF and misses U+0085), positive classes with \W/\H are lifted into alternations ([\W_] → (?:[_]|[^…word…])), POSIX classes ([[:alpha:]]), \x{...} → \u{...}, \p{Word}, \a/\e, and literal {/}/stray ] (onig-legal, previously fatal JS syntax errors). |
| const tokenizerConfig = JSON.parse(fs.readFileSync(tokenizerConfigPath, "utf-8")); | ||
| return { tokenizerJson, tokenizerConfig }; | ||
| } catch { | ||
| fs.rmSync(cacheDir, { force: true, recursive: true }); |
There was a problem hiding this comment.
did you notice any download corruptions? 👀
There was a problem hiding this comment.
Yes, the npm run test in github actions often fails because of multiple invalid responses.
FAIL tests/memoryLeaks.test.ts (212 MB heap size)
Memory leak tests
✕ should not leak memory (6 ms)
● Memory leak tests › should not leak memory
Failed to fetch https://huggingface.co/TinyLlama/TinyLlama-1.1B-Chat-v1.0/resolve/main/tokenizer_config.json: 429 Too Many Requests
35 | const response = await fetch(url);
36 | if (!response.ok) {
> 37 | throw new Error(`Failed to fetch ${url}: ${response.status} ${response.statusText}`);
| ^
38 | }
39 | return await response.json();
40 | };
One example:
https://github.com/huggingface/tokenizers.js/actions/runs/27579303033/job/81583399599
So I tried multiple things to make the tests more robust. But I wasn't able actually find a robust solution..
|
back to green :) I'd say final reviews are in order now. |
There was a problem hiding this comment.
I think it's ready! @nico-martin feel free to do a final review and merge when you're happy.
Summary
#,&,~,', and".\w,\W,\d,\D).\bor\B, since JavaScript boundary semantics can be ASCII-oriented.['\\\"]) and multilingual\w+matching (including Hebrew text).Why
Validation
npm test -- tests/edgeCases.test.ts.tests/edgeCases.test.tspass, including the new compatibility checks.