Fix/minja missing string methods#1083
Open
samuel100 wants to merge 4 commits into
Open
Conversation
The vendored copy of minja in `shared/api/minja.hpp` was missing handlers
for several standard Jinja string methods that current upstream
`google/minja` supports. The most impactful gap is `.replace()`, which
caused `OrtxApplyChatTemplate` to throw `Unknown method: replace` on
chat templates shipped with widely-used Hugging Face models (e.g.
`smollm3-3b`'s `chat_template.jinja` uses
`system_message.replace("/no_think", "").replace("/think", "").rstrip()`).
This adds the three missing string-method handlers — `.replace`,
`.upper`, `.lower` — to the string-method dispatch in
`MethodCallExpr::do_evaluate`, with semantics matching upstream minja
and Python's `str` methods:
- `.replace(before, after[, count])` performs left-to-right substring
replacement, defaulting to all occurrences when no count is given.
An empty `before` returns the original string (matches upstream).
- `.upper()` / `.lower()` apply `std::toupper` / `std::tolower` via
`std::transform` with an `unsigned char` lambda to avoid UB on
signed-char platforms.
Tests
-----
Adds `MinjaStringReplace` and `MinjaStringUpperLower` regression tests
under `test/pp_api_test/test_tokenizer_chat.cc`, following the same
inline-template pattern introduced in #1070
(`MinjaStringSliceOOBClamped`). Coverage includes:
- single and chained `.replace()`
- replace with optional `count` argument
- replace with empty `before` (no-op)
- replace with no match (unchanged)
- `.upper()` and `.lower()` on mixed-case input
Verified locally with a standalone harness that compiles `minja.hpp`
directly and renders the exact failing line from
`smollm3-3b`'s `chat_template.jinja` — previously thrown with
`Unknown method: replace`, now renders correctly.
Fixes #1081
Refs microsoft/Foundry-Local#800
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- replace(): treat count < 0 as "replace all" to match Python str.replace semantics (previously a negative count silently produced zero replacements via `count-- > 0`). - upper() / lower(): switch from std::toupper / std::tolower to an explicit ASCII-only mapping. This is deterministic across locales (std::toupper/tolower are locale-dependent) and avoids the trap of per-process locale changing template rendering. Non-ASCII bytes are passed through unchanged, matching upstream minja behavior on most locales. Full Unicode case folding is out of scope. - Test helper: assert OrtxTensorResultGetAt and OrtxGetTensorData return kOrtxOK and that the data pointer is non-null, so test failures point at the actual cause rather than a downstream EXPECT_STREQ on undefined memory. - New regression tests for replace count=-1 (replace all) and count=0 (no replacements) to lock in the Python semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tring-methods # Conflicts: # test/pp_api_test/test_tokenizer_chat.cc
Contributor
There was a problem hiding this comment.
⚠️ Not ready to approve
The new replace() implementation has a small but real correctness/robustness issue (default count can overflow int64_t on very large strings and the loop does unnecessary work when count == 0) that should be fixed before approval.
Pull request overview
This PR updates the vendored minja template engine to support additional Jinja2-style string methods (.replace(), .upper(), .lower()) that are required by common Hugging Face chat templates, and adds regression tests to ensure OrtxApplyChatTemplate can render templates using these methods.
Changes:
- Add string method dispatch for
.upper(),.lower(), and.replace()inshared/api/minja.hpp. - Add regression tests covering
.replace()semantics (including chaining +rstripand optionalcount) and.upper()/.lower()intest/pp_api_test/test_tokenizer_chat.cc.
File summaries
| File | Description |
|---|---|
shared/api/minja.hpp |
Adds missing string-method handlers in MethodCallExpr::do_evaluate to unblock real-world HF templates. |
test/pp_api_test/test_tokenizer_chat.cc |
Adds regression tests validating the new string methods via OrtxApplyChatTemplate. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2248
to
+2267
| // Python str.replace semantics: count < 0 means "replace all"; | ||
| // count >= 0 limits the number of replacements; omitted argument | ||
| // also means "replace all". | ||
| int64_t count = static_cast<int64_t>(res.length()); | ||
| if (vargs.args.size() == 3) | ||
| { | ||
| auto requested = vargs.args[2].get<int64_t>(); | ||
| if (requested >= 0) | ||
| { | ||
| count = requested; | ||
| } | ||
| } | ||
| size_t start_pos = 0; | ||
| while ((start_pos = res.find(before, start_pos)) != std::string::npos && | ||
| count-- > 0) | ||
| { | ||
| res.replace(start_pos, before.length(), after); | ||
| start_pos += after.length(); | ||
| } | ||
| return Value(res); |
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.
Replacement for #1082
Summary
Adds the three missing string-method handlers —
.replace,.upper,.lower— to the vendored minja parser atshared/api/minja.hpp. These are standard Jinja2 string methods that Hugging Facetransformerschat templates rely on, and the most impactful gap (.replace) currently breaks any model whosechat_template.jinjauses it — most visiblysmollm3-3b, whereOrtxApplyChatTemplatethrows:Fixes #1081
Refs microsoft/Foundry-Local#800
Diff against upstream
Comparing
shared/api/minja.hppagainst current upstreamgoogle/minja:.strip/.rstrip/.lstrip.split.title.endswith/.startswith/.capitalize.replace.upper.lower(Note:
lowerwas already available as a global filter —{{ x \| lower }}— but not as the method formx.lower()that HF templates actually use.)Implementation notes
Handlers were added in
MethodCallExpr::do_evaluatenext to the existing string-method dispatch:.replace(before, after[, count])— left-to-right substring replacement matching Python'sstr.replaceand upstream minja. Defaults to all occurrences whencountis omitted. Emptybeforereturns the string unchanged (matches upstream behavior; avoids an infinite loop)..upper()/.lower()—std::transformwith anunsigned char-typed lambda aroundstd::toupper/std::tolowerto avoid undefined behavior on signed-char platforms.I deliberately limited the scope of this PR to the methods needed to unblock real HF templates today; a full re-sync of
minja.hppagainst current upstream would also be reasonable but is out of scope here.Tests
Adds two regression tests in
test/pp_api_test/test_tokenizer_chat.cc, following the inline-template_strpattern introduced in #1070 (MinjaStringSliceOOBClamped):MinjaStringReplace— single replace, chained replace + rstrip (the exact smollm3-3b pattern), three-argcount, emptybefore, no-match.MinjaStringUpperLower—.upper()and.lower()on mixed-case input, plus idempotent cases.Local verification
I verified the fix end-to-end with a small standalone harness that compiles
shared/api/minja.hppdirectly and renders the exact failing line fromsmollm3-3b'schat_template.jinja:I did not build the full
pp_api_testbinary locally (would require ORT + sentencepiece + onnxruntime-extensions deps setup on macOS); the new gtest cases are written to existing conventions and will run in CI.Follow-up (out of scope here)
It would be worth adding a periodic re-sync of
shared/api/minja.hppwith current upstreamgoogle/minja, and/or a regression suite that renders the chat templates of a handful of popular HF models (SmolLM3, Qwen3, Llama-3.x, Phi-4) againstOrtxApplyChatTemplate, to catch this kind of drift earlier. I'm happy to follow up with that in a separate PR if maintainers want.