From 519bba0bda525f280c5d2c54e150874744c2e9b2 Mon Sep 17 00:00:00 2001 From: Sam Kemp Date: Fri, 12 Jun 2026 10:15:01 +0100 Subject: [PATCH 1/3] Add missing .replace, .upper, .lower string methods to vendored minja MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- shared/api/minja.hpp | 37 ++++++++++++ test/pp_api_test/test_tokenizer_chat.cc | 75 +++++++++++++++++++++++++ 2 files changed, 112 insertions(+) diff --git a/shared/api/minja.hpp b/shared/api/minja.hpp index 542fd616f..e509bdde7 100644 --- a/shared/api/minja.hpp +++ b/shared/api/minja.hpp @@ -2197,6 +2197,43 @@ namespace minja } return res; } + else if (method->get_name() == "upper") + { + vargs.expectArgs("upper method", {0, 0}, {0, 0}); + auto res = str; + std::transform(res.begin(), res.end(), res.begin(), + [](unsigned char c) { return std::toupper(c); }); + return Value(res); + } + else if (method->get_name() == "lower") + { + vargs.expectArgs("lower method", {0, 0}, {0, 0}); + auto res = str; + std::transform(res.begin(), res.end(), res.begin(), + [](unsigned char c) { return std::tolower(c); }); + return Value(res); + } + else if (method->get_name() == "replace") + { + vargs.expectArgs("replace method", {2, 3}, {0, 0}); + auto before = vargs.args[0].get(); + auto after = vargs.args[1].get(); + auto res = str; + if (before.empty()) + { + return Value(res); + } + auto count = vargs.args.size() == 3 ? vargs.args[2].get() + : static_cast(res.length()); + 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); + } } throw std::runtime_error("Unknown method: " + method->get_name()); } diff --git a/test/pp_api_test/test_tokenizer_chat.cc b/test/pp_api_test/test_tokenizer_chat.cc index 85cf64fde..294539875 100644 --- a/test/pp_api_test/test_tokenizer_chat.cc +++ b/test/pp_api_test/test_tokenizer_chat.cc @@ -2196,4 +2196,79 @@ TEST(OrtxTokenizerTest, MinjaParserRecursionDepthLimit) { messages_json.c_str(), nullptr, result.ToBeAssigned(), false, false); // Should fail gracefully with an error, not crash with a stack overflow. EXPECT_NE(err, kOrtxOK); +} + +// Regression tests for missing string methods (.replace, .upper, .lower) in +// the vendored minja parser. Chat templates from popular HF models (e.g. +// smollm3-3b) call these on string values; previously they failed with +// "Unknown method: ". See: +// - microsoft/onnxruntime-extensions#1081 +// - microsoft/Foundry-Local#800 +namespace { + +void ExpectTemplateRenders(OrtxTokenizer* tokenizer, + const std::string& tmpl, + const std::string& expected) { + std::string messages_json = R"([{"role":"user","content":"hi"}])"; + OrtxObjectPtr result; + auto err = OrtxApplyChatTemplate( + tokenizer, tmpl.c_str(), messages_json.c_str(), nullptr, + result.ToBeAssigned(), false, false); + ASSERT_EQ(err, kOrtxOK) << "template: " << tmpl + << " err: " << OrtxGetLastErrorMessage(); + + OrtxObjectPtr tensor; + OrtxTensorResultGetAt(result.get(), 0, tensor.ToBeAssigned()); + ASSERT_EQ(tensor.Code(), kOrtxOK); + const char* text = nullptr; + OrtxGetTensorData(tensor.get(), reinterpret_cast(&text), + nullptr, nullptr); + EXPECT_STREQ(text, expected.c_str()) << "template: " << tmpl; +} + +} // namespace + +TEST(OrtxTokenizerTest, MinjaStringReplace) { + OrtxObjectPtr tokenizer(OrtxCreateTokenizer, "data/phi-4-base"); + ASSERT_EQ(tokenizer.Code(), kOrtxOK); + + // Single replacement. + ExpectTemplateRenders(tokenizer.get(), + "{{ 'foo and foo'.replace('foo', 'bar') }}", + "bar and bar"); + + // Chained replace + rstrip, exactly the pattern smollm3-3b uses. + ExpectTemplateRenders( + tokenizer.get(), + "{{ 'be helpful /no_think and /think '" + ".replace('/no_think', '').replace('/think', '').rstrip() }}", + "be helpful and"); + + // Replace with optional count argument (Python str.replace semantics). + ExpectTemplateRenders(tokenizer.get(), + "{{ 'aaaa'.replace('a', 'X', 2) }}", + "XXaa"); + + // Replace with empty `before` is a no-op (matches upstream minja behavior). + ExpectTemplateRenders(tokenizer.get(), + "{{ 'abc'.replace('', 'X') }}", + "abc"); + + // No match: original string returned unchanged. + ExpectTemplateRenders(tokenizer.get(), + "{{ 'abc'.replace('z', 'X') }}", + "abc"); +} + +TEST(OrtxTokenizerTest, MinjaStringUpperLower) { + OrtxObjectPtr tokenizer(OrtxCreateTokenizer, "data/phi-4-base"); + ASSERT_EQ(tokenizer.Code(), kOrtxOK); + + ExpectTemplateRenders(tokenizer.get(), "{{ 'Hello, World!'.upper() }}", + "HELLO, WORLD!"); + ExpectTemplateRenders(tokenizer.get(), "{{ 'Hello, World!'.lower() }}", + "hello, world!"); + // Idempotent on already-cased strings. + ExpectTemplateRenders(tokenizer.get(), "{{ 'ABC'.upper() }}", "ABC"); + ExpectTemplateRenders(tokenizer.get(), "{{ 'abc'.lower() }}", "abc"); } \ No newline at end of file From cd83e4ee319e9ed2dfa0699e5751e84070ae0b7b Mon Sep 17 00:00:00 2001 From: Sam Kemp Date: Fri, 12 Jun 2026 10:36:57 +0100 Subject: [PATCH 2/3] Address Copilot review feedback on #1082 - 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> --- shared/api/minja.hpp | 29 ++++++++++++++++++++----- test/pp_api_test/test_tokenizer_chat.cc | 19 +++++++++++++--- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/shared/api/minja.hpp b/shared/api/minja.hpp index e509bdde7..7466f7907 100644 --- a/shared/api/minja.hpp +++ b/shared/api/minja.hpp @@ -2200,17 +2200,24 @@ namespace minja else if (method->get_name() == "upper") { vargs.expectArgs("upper method", {0, 0}, {0, 0}); + // ASCII-only case mapping for deterministic, locale-independent + // behavior. Matches upstream minja in practice for ASCII inputs; + // non-ASCII bytes are passed through unchanged. Full Unicode + // case folding is out of scope for the parser. auto res = str; - std::transform(res.begin(), res.end(), res.begin(), - [](unsigned char c) { return std::toupper(c); }); + for (char& c : res) { + if (c >= 'a' && c <= 'z') c = static_cast(c - ('a' - 'A')); + } return Value(res); } else if (method->get_name() == "lower") { vargs.expectArgs("lower method", {0, 0}, {0, 0}); + // ASCII-only case mapping; see .upper() comment above. auto res = str; - std::transform(res.begin(), res.end(), res.begin(), - [](unsigned char c) { return std::tolower(c); }); + for (char& c : res) { + if (c >= 'A' && c <= 'Z') c = static_cast(c + ('a' - 'A')); + } return Value(res); } else if (method->get_name() == "replace") @@ -2223,8 +2230,18 @@ namespace minja { return Value(res); } - auto count = vargs.args.size() == 3 ? vargs.args[2].get() - : static_cast(res.length()); + // 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(res.length()); + if (vargs.args.size() == 3) + { + auto requested = vargs.args[2].get(); + if (requested >= 0) + { + count = requested; + } + } size_t start_pos = 0; while ((start_pos = res.find(before, start_pos)) != std::string::npos && count-- > 0) diff --git a/test/pp_api_test/test_tokenizer_chat.cc b/test/pp_api_test/test_tokenizer_chat.cc index 294539875..4a1382463 100644 --- a/test/pp_api_test/test_tokenizer_chat.cc +++ b/test/pp_api_test/test_tokenizer_chat.cc @@ -2218,11 +2218,14 @@ void ExpectTemplateRenders(OrtxTokenizer* tokenizer, << " err: " << OrtxGetLastErrorMessage(); OrtxObjectPtr tensor; - OrtxTensorResultGetAt(result.get(), 0, tensor.ToBeAssigned()); + err = OrtxTensorResultGetAt(result.get(), 0, tensor.ToBeAssigned()); + ASSERT_EQ(err, kOrtxOK) << "OrtxTensorResultGetAt failed for template: " << tmpl; ASSERT_EQ(tensor.Code(), kOrtxOK); const char* text = nullptr; - OrtxGetTensorData(tensor.get(), reinterpret_cast(&text), - nullptr, nullptr); + err = OrtxGetTensorData(tensor.get(), reinterpret_cast(&text), + nullptr, nullptr); + ASSERT_EQ(err, kOrtxOK) << "OrtxGetTensorData failed for template: " << tmpl; + ASSERT_NE(text, nullptr) << "OrtxGetTensorData returned null data for template: " << tmpl; EXPECT_STREQ(text, expected.c_str()) << "template: " << tmpl; } @@ -2249,6 +2252,16 @@ TEST(OrtxTokenizerTest, MinjaStringReplace) { "{{ 'aaaa'.replace('a', 'X', 2) }}", "XXaa"); + // Python str.replace semantics: count < 0 means "replace all". + ExpectTemplateRenders(tokenizer.get(), + "{{ 'aaaa'.replace('a', 'X', -1) }}", + "XXXX"); + + // Python str.replace semantics: count == 0 means no replacements. + ExpectTemplateRenders(tokenizer.get(), + "{{ 'aaaa'.replace('a', 'X', 0) }}", + "aaaa"); + // Replace with empty `before` is a no-op (matches upstream minja behavior). ExpectTemplateRenders(tokenizer.get(), "{{ 'abc'.replace('', 'X') }}", From 90239e3a1556baa64ec8977dc4ccde01ccabdff9 Mon Sep 17 00:00:00 2001 From: Samuel Kemp Date: Mon, 22 Jun 2026 10:36:22 +0100 Subject: [PATCH 3/3] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- shared/api/minja.hpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/shared/api/minja.hpp b/shared/api/minja.hpp index 56ffea2c4..cecb4eb26 100644 --- a/shared/api/minja.hpp +++ b/shared/api/minja.hpp @@ -2248,7 +2248,7 @@ namespace minja // 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(res.length()); + int64_t count = (std::numeric_limits::max)(); if (vargs.args.size() == 3) { auto requested = vargs.args[2].get(); @@ -2258,11 +2258,12 @@ namespace minja } } size_t start_pos = 0; - while ((start_pos = res.find(before, start_pos)) != std::string::npos && - count-- > 0) + while (count > 0 && + (start_pos = res.find(before, start_pos)) != std::string::npos) { res.replace(start_pos, before.length(), after); start_pos += after.length(); + --count; } return Value(res); }