Skip to content

Fix UTF-8 offset in the STL regex pre-tokenizer fallback - #1136

Open
Yueqing Zhang (BoarQing) wants to merge 3 commits into
microsoft:mainfrom
BoarQing:fix/stl-regex-utf8-offset
Open

Yueqing Zhang (BoarQing) wants to merge 3 commits into
microsoft:mainfrom
BoarQing:fix/stl-regex-utf8-offset

Conversation

@BoarQing

Copy link
Copy Markdown

Summary

PreTokenizerWithRegEx::MatchWithSTLRegEx (the std::regex fallback for Split patterns without a hardcoded matcher) desynced its UTF-8 and UTF-32 copies of the text whenever a multi-byte character came before a match. A later match could then be missing from the UTF-32 copy, so m_text.substr(npos) threw invalid string_view position and encoding failed.

Cause

regex_search runs on the UTF-8 copy (m_utf8_text), but both copies were advanced by m_text.find(res), a code-point index into the UTF-32 copy (m_text):

auto pos = m_text.find(res);                                    // code points
m_utf8_text = m_utf8_text.substr(pos + match[0].str().size());  // used as bytes

With café\nnext\n中文\nend and the pattern (?:\r?\n)+(?!\r?\n), the first \n is at byte 5 but code point 4. From then on the two copies start at different places. After the third match the UTF-8 copy has been cut mid-character and still contains a \n that m_text no longer has.

Fix

Advance the UTF-8 copy by the match's byte position and length (match.position(0), match.length(0)). Advance the UTF-32 copy by the number of code points before the match, counted as the non-continuation bytes before match.position(0).

How it was found

The tokenizer of the Laguna-XS 2.1 model begins its pre-tokenizer with Split on (?:\r?\n)+(?!\r?\n). That pattern takes the fallback path, so encoding any prompt with a non-ASCII character (for example ’) followed by a newline and more text threw. Hugging Face tokenizers 0.23.2 encodes the same text without error.

Test

  • New OrtxTokenizerTest.FallbackRegexAfterMultiByteChar in test/pp_api_test/test_tokenizer_impl.cc. Without the fix it fails with invalid string_view position; with the fix it passes.
  • pp_api_test on Windows x64 (MSVC 19.44, RelWithDebInfo): 223 tests passed.
  • With the same change on the 99fec2db pin used by onnxruntime-genai 0.17, OGA encodes two real Laguna prompts (121 and 1973 tokens) that threw before. The token IDs match Hugging Face tokenizers exactly, apart from the BOS token that OGA leaves out by default (add_special_tokens=false).

Not in this PR

Split steps inside a Sequence are always treated as behavior=Isolated (SplitIsolated). Laguna's first Split uses MergedWithNext. This didn't change the token IDs on the prompts above, but it is a separate gap.

AI assistance: Cursor helped find the cause and write the fix and test. I reviewed the change and ran the tests above.

MatchWithSTLRegEx searched the UTF-8 copy of the text but advanced it by
m_text.find(match), a code-point index into the UTF-32 copy. Once a
multi-byte character preceded a match the two copies drifted apart, and
a later match could be absent from m_text, so substr(npos) threw
"invalid string_view position".

Use the match's byte position and length for the UTF-8 copy, and count
the code points before the match for the UTF-32 copy.

Made-with: Cursor
Co-Authored-By: Cursor <cursoragent@cursor.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Zero-length fallback matches can empty the cursor before callers access its next character.

1 open issue
What changed in this PR

Fixes UTF-8 and UTF-32 cursor synchronization in the tokenizer’s std::regex fallback.

Changes:

  • Advances each text representation using its corresponding offset units.
  • Adds a regression test for newline matches after multibyte characters.
File Description
test/​pp_api_test/​test_tokenizer_impl.cc Tests fallback splitting after multibyte characters.
operators/​tokenizer/​bpe_utils.hpp Separates byte offsets from code-point offsets.

Review effort: Balanced

Comment thread operators/tokenizer/bpe_utils.hpp
Yueqing Zhang (BoarQing) added a commit to ROCm/hip-ep that referenced this pull request Oct 20, 2026
## Summary

`wrap_qmoe` now supports QMoE's optional `router_weights` input (input
14). Before this change it rejected the input and returned -1. Every MoE
layer of a model that uses it was left uncomputed, but the run still
exited 0 and produced plausible text. On Laguna's Jenkins
`prompt_128.txt`, `main` matches the ORT CPU EP's greedy output for 5
tokens and then diverges (" AMD has gone over generative AI" instead of
" AMD has gone all-in on generative AI"). With this change, all 24
tokens compared match. Laguna-XS 2.1 (39 QMoE nodes, k=8,
`normalize_routing_weights=0`) now runs end to end on gfx1151.

## Related issue or design

None. Found while running the Jenkins Laguna job. The tokenizer crash
that job hits first is fixed separately in
[microsoft/onnxruntime-extensions#1136](microsoft/onnxruntime-extensions#1136).

## Why

`router_weights` separates expert selection from expert mixing, as in
the ORT CPU kernel. `router_probs` still picks the top-k experts. Each
selected expert's mixing weight is then read from `router_weights` at
that expert's index, and the selected weights are divided by their sum
only when `normalize_routing_weights=1`. If that sum is zero, the
weights become zero.

## What

- `hip_qmoe_topk_routing` takes an optional `router_weights` pointer.
When it is set, the routing kernel writes the gathered weights instead
of the softmax of the logits.
- The selected weights are staged in fp32 before the optional
normalization, then written as fp16.
- Every decode and prefill path (dp4a, fused decode, grouped and ragged
prefill) already consumes the routing kernel's weights, so none of them
change.
- The launch now clears the HIP last-error state first.

## Test plan

- Numeric (`test/numeric/tests/test_qmoe.py`, `HIPDNN_EP_STRICT=1`,
gfx1151): 10 passed, 2 xfailed, 1 failed.
- New `test_qmoe_gpt_oss_shape_mix_weights`: `normalize` 0 and 1, at
`seq_len` 1, 37 and 128. The mixing weights are random and uncorrelated
with the logits, so using the softmax of the logits instead would not
match.
- The two `seq_len=1` cases are marked `xfail(strict=False)`. The fused
decode path exceeds `atol=0.1` there, as the existing
`test_qmoe_gpt_oss_shape[1]` does without `router_weights`.
- The one failure is `test_qmoe_gpt_oss_shape[1]`. It fails with
identical numbers on `main` (max abs diff 0.615, mean 0.135).
- LIT: 599 passed, 9 unsupported. The 2 failures
(`Dialect/Hipsr/Pipelines/embedding.mlir`,
`Dialect/hip-externalize-constants.mlir`) come from an unquoted Python
path in my local setup and fail the same way without this change.
- End to end, Laguna-XS 2.1 fp16/int4 (block size 128):
`model_benchmark.exe` from the `main` CI package at `05ad6706`, with
this branch's `hipgpu.dll` and `custom_kernels_gfx1151.dll`. OGA was
rebuilt with the ort-extensions fix above, and `enable_graph_capture`
was set to `"0"` (see Notes).
- With `HIPDNN_EP_STRICT=1` and `HIPDNN_EP_DEBUG=1` (verification only),
there are 312 routing calls (39 layers × 8 forward passes), all with
`router_weights=yes`, and no errors.
- The 24-token greedy continuation of the Jenkins `prompt_128.txt` is
identical to the ORT CPU EP on the same model.
- The Jenkins prompts (`prompt_128.txt`, `prompt_2k.txt`) each completed
128 generated tokens × 3 repetitions after 1 warmup.

## Notes for reviewers

- Laguna also needs `"enable_graph_capture": "0"` in its AMDGPU provider
options with OGA 0.17, because graph capture requires
`past_present_share_buffer=true`. That is a model-config question and is
not part of this PR.
- `planQMoE` in HipToTosa still declines `router_weights`, so those
nodes keep the `wrap_qmoe` path this PR fixes. Its comment
("router_weights overrides router_probs and skips the softmax") doesn't
match the semantics above. I left it unchanged to keep this PR focused.
- The mock `wrap_qmoe` already accepted and logged `router_weights`, so
it is unchanged.

## Checklist

- [x] The change is focused, or links a design/series explaining its
scope.
- [x] Relevant tests were added or updated and the results are
documented.
- [x] User-facing or design documentation was updated when needed. No
docs mention a `router_weights` limitation.
- [x] Substantial AI assistance is disclosed, and I reviewed and
understand the result. Cursor helped write the kernel change, test and
description; validated as described above.

Co-authored-by: Cursor <cursoragent@cursor.com>
A zero-length fallback match after the start of the remaining text, such
as $ or a bare lookahead, advanced both copies past the characters before
it while returning an empty view. GetNextToken and SplitIsolated read an
empty view as no match and then take m_text[0] themselves, so the skipped
characters were lost, or substr(1) threw "invalid string_view position"
once the text was exhausted.

Return before moving either copy when the match is empty, and cover both
callers with zero-length patterns.

Made-with: Cursor
Co-Authored-By: Cursor <cursoragent@cursor.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused fix addresses the offset error and prior feedback, with regression coverage and no unresolved findings.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

Yueqing Zhang (BoarQing) added a commit to ROCm/hip-ep that referenced this pull request Oct 20, 2026
Laguna's tokenizer needs microsoft/onnxruntime-extensions#1136. Add
ORTX_PR_PATCHES, the onnxruntime-extensions counterpart of OGA_PR_PATCHES:
each listed PR's pull/<n>.diff is applied with the same rules (--3way,
tests and .github excluded) to the onnxruntime-extensions commit OGA pins
in cmake/deps.txt, read after the OGA PR patches. OGA then builds that tree
through FETCHCONTENT_SOURCE_DIR_ONNXRUNTIME_EXTENSIONS. The list is part
of the OGA cache key.

This replaces the vendored .patch file used before, so nothing has to be
regenerated when the upstream PR changes.

Made-with: Cursor
Co-Authored-By: Cursor <cursoragent@cursor.com>
@BoarQing

Copy link
Copy Markdown
Author

Sayan Shaw (@sayanshaw24) Hi, can u approve and merge this?

Yueqing Zhang (BoarQing) added a commit to BoarQing/onnxruntime-genai that referenced this pull request Oct 20, 2026
CI test vehicle for ROCm/hip-ep, which applies OGA PR diffs on top of
v0.17.0. Points the onnxruntime-extensions pin at 99fec2db plus the two
commits of microsoft/onnxruntime-extensions#1136 (UTF-8 offset fix in the
STL regex pre-tokenizer fallback), hosted on a fork until microsoft#1136 merges.

Made-with: Cursor
Co-Authored-By: Cursor <cursoragent@cursor.com>
Yueqing Zhang (BoarQing) added a commit to ROCm/hip-ep that referenced this pull request Oct 20, 2026
microsoft/onnxruntime-genai#2763 (draft, not for merge) moves OGA's
onnxruntime-extensions pin from 99fec2db to 99fec2db plus the two commits
of microsoft/onnxruntime-extensions#1136, which Laguna's tokenizer needs.

Made-with: Cursor
Co-Authored-By: Cursor <cursoragent@cursor.com>
Ming Wang (amd-mingw) pushed a commit to ROCm/hip-ep that referenced this pull request Oct 21, 2026
## Summary

Test PR, not for merge. It adds the onnxruntime-extensions tokenizer fix
([microsoft/onnxruntime-extensions#1136](microsoft/onnxruntime-extensions#1136))
to the Windows OGA build, so the CI package can run Laguna-XS 2.1 in
Jenkins. The whole change is one entry in `OGA_PR_PATCHES`.

## Related issue or design

-
[microsoft/onnxruntime-extensions#1136](microsoft/onnxruntime-extensions#1136):
the tokenizer fix.
-
[microsoft/onnxruntime-genai#2763](microsoft/onnxruntime-genai#2763):
draft, not for merge. It moves OGA's onnxruntime-extensions pin to
`99fec2db` plus the two #1136 commits, hosted on a fork until #1136
merges.
- #1241: QMoE `router_weights` support, already on main.

## Why

OGA 0.17 (with PRs 2685, 2691, 2705, 2712) pins onnxruntime-extensions
`99fec2db`, which throws `invalid string_view position` when encoding a
Laguna prompt that has a non-ASCII character before a newline. Both
Jenkins prompts (`prompt_128.txt`, `prompt_2k.txt`) hit it.

## What

`windows-deps.yml`: `OGA_PR_PATCHES` gains `2763`. Only the Windows OGA
build changes.

## Test plan

- I replayed the "Apply OGA PR patches" step locally on v0.17.0 with
`2685 2691 2705 2712 2763`. All five applied, and the resulting ortx pin
is the fork commit `c114ea9`. Its `bpe_utils.hpp` is byte-identical to
the #1136 head.
- A local OGA build with this onnxruntime-extensions fix encodes both
Jenkins prompts. The token IDs match Hugging Face `tokenizers` exactly,
apart from the BOS token that OGA omits by default.
- With that OGA, #1241's `hipgpu.dll` and `enable_graph_capture: "0"`,
Laguna completes both Jenkins prompts on gfx1151. Its 24-token greedy
output on `prompt_128.txt` matches the ORT CPU EP.

## Notes for reviewers

- Laguna still needs `"enable_graph_capture": "0"` in its AMDGPU
provider options under OGA 0.17. That is a model-config change and is
not part of this PR.
- Once #1136 merges and OGA pins a commit containing it, replace 2763
with that OGA change and close 2763.

## Checklist

- [x] The change is focused, or links a design/series explaining its
scope.
- [x] Relevant tests were added or updated and the results are
documented.
- [x] User-facing or design documentation was updated when needed. Not
applicable for a CI test change.
- [x] Substantial AI assistance is disclosed, and I reviewed and
understand the result. Cursor prepared the OGA PR, this change and this
description; validated as described above.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants