Repository navigation
fix(server): make the chat-template tojson filter match json.dumps - #1382
Merged
Merged
Conversation
mlxcel rendered chat templates with minijinja's builtin `tojson`, which accepts only `indent` and emits serde_json's compact form. Published templates are written against `transformers`, where `tojson` is CPython `json.dumps` with `ensure_ascii=False` prefilled, so they call it with `json.dumps` keyword arguments and expect `json.dumps` output. `poolside/Laguna-XS-2.1` (`tojson(ensure_ascii=False)`) and `thinkingmachines/Inkling-Small` (`tojson(sort_keys=true, separators=(",", ":"))`) failed to render at all, dropping the request to the generic `User:/Assistant:` prompt, and every template using a bare `x | tojson` produced different token ids from the provider's own tokenization.
`src/server/chat_template_json.rs` implements `python_tojson`, registered in `configure_environment` where it shadows the builtin. It supports `ensure_ascii` (default false), `indent` (positional or keyword, `true` meaning 2), `separators`, and `sort_keys` (recursive, code-point order), still calls `assert_all_used` so an unknown kwarg is an error, and walks the minijinja value directly so integers stay integers, non-finite floats render bare as `NaN` / `Infinity` / `-Infinity`, and object keys keep insertion order unless sorted. Jinja's HTML-safe escaping of `<`, `>`, `&` and `'` is deliberately not applied: `transformers` does not apply it, and a JSON Schema `pattern` full of comparison operators has to reach the prompt intact.
Float formatting has the most room to be subtly wrong, so it is a separate function with its own tests. Rust's `{}` for f64 differs from CPython's `repr` on three points: the `.0` suffix for an integral float, the decimal-exponent thresholds where CPython switches to exponent form (`<= -4` or `> 16`, so `0.0001` stays fixed while `0.00001` becomes `1e-05`, and `1e15` stays fixed while `1e16` becomes `1e+16`), and the signed two digit exponent. The shortest round-trip digits come from Rust's `LowerExp`, which matches CPython's dtoa mode 0.
Container recursion is capped at 128 levels, matching serde_json's own default. The value being serialized comes from the client request body, and minijinja's fuel budget bounds VM instructions rather than native stack frames inside one filter call.
Two pinned Muse Glimmer render digests move. That template serializes each tool's JSON Schema with `fn.parameters | tojson`, so both renders grow by the 13 bytes of `json.dumps` separator spacing; substituting the compact schema back into either new render reproduces the previous digest exactly, so the separators are the whole of the difference. The `tojson` assertion in `chat_request_tests` moves for the same reason. The `supports_tools` heuristic is untouched.
Validated with `cargo test --lib --features metal,accelerate server::` (1883 passed, 0 failed), `cargo clippy --lib --tests --features metal,accelerate -- -D warnings`, and `cargo fmt --all -- --check`. Every expected string in the new tests was generated from CPython `json.dumps` / `repr` rather than written by hand. The real-checkpoint check against `poolside/Laguna-XS-2.1-NVFP4` is blocked on the NVFP4 loader and is not part of this change.
Refs #1309
Review of the CPython-compatible tojson filter found three ways a template or a request body can drive it into unbounded work, plus one ceiling set low enough to break the case the filter exists to fix. `indent` is template-supplied and is multiplied by the nesting level to produce literal padding, so `width * level` could overflow `usize` and a merely large width emitted gigabytes of spaces. It is now clamped to 64 columns, and the padding is reserved once per break rather than pushed a space at a time. Clamping beats rejecting here: a template asking for a huge indent wants pretty output, and a failed render costs it the tools block. Nothing bounded the rendered size. minijinja's fuel counts VM instructions, not bytes emitted inside a single filter call, and `tojson(indent=4)` (which the Llama 3.1, Llama 4 Scout and Granite templates all call) amplifies its input substantially, as does `ensure_ascii` on non-ASCII text. A 64 MiB ceiling is now checked on entry to every node, so the walk stops shortly after crossing rather than after rendering everything. `separators` was collected in full before its arity was checked, so a large lazy sequence allocated before the rejection could fire. It now takes three elements and rejects anything that is not exactly two. MAX_DEPTH was 128, exactly the limit serde_json applies to the request body. `chat_request` re-parses `function.arguments` out of its wire string and splices the result back into the message tree at roughly depth 4, and that inner parse gets a fresh 128-level budget of its own, so an assembled `messages` value can legitimately reach about 132. At 128 a template that serializes a whole message rather than just the arguments object would error, abort the render, and degrade to the generic prompt that drops tool declarations, which is the failure this filter exists to prevent. It is now 192, and the new test asserts the headroom invariant rather than the literal value so that lowering it back to match serde_json fails. Also records the one known divergence from CPython: `sort_keys` sorts the stringified keys while CPython sorts the original key objects, so numeric keys order differently. Only a template-authored dict literal can produce a non-string key, since keys arriving as JSON from a client are always strings.
be-free-snowboy
pushed a commit
to freestylejs/snowbrain-mlxcel
that referenced
this pull request
Oct 27, 2026
…ablup#1382) ## Summary mlxcel rendered chat templates with minijinja's builtin `tojson`, which accepts only `indent` and always emits serde_json's compact form. Published templates are written against `transformers`, where Jinja's `tojson` is CPython `json.dumps` with `ensure_ascii=False` prefilled, so template authors call it with `json.dumps` keyword arguments and expect `json.dumps` output. This registers an mlxcel-owned `tojson` filter that shadows the builtin and reproduces `json.dumps` byte for byte for the argument combinations templates actually use. Two failure modes are fixed. The loud one: `poolside/Laguna-XS-2.1` calls `tojson(ensure_ascii=False)` and `thinkingmachines/Inkling-Small` calls `tojson(sort_keys=true, separators=(",", ":"))`, and the builtin's `Kwargs::assert_all_used` rejected both outright, so the whole render aborted and the request degraded to the generic `User:/Assistant:` prompt with no tool declarations and no tool-call history. The silent one: a bare `x | tojson` emitted `{"a":1}` where the reference renderer emits `{"a": 1}`, so tool schemas reaching the model tokenized differently from the prompt its provider trained and evaluated against. ## What changed - **`src/server/chat_template_json.rs` (new)** implements `python_tojson`: `ensure_ascii` (default `false`, which is transformers' default rather than CPython's own `True`), `indent` (positional or keyword, `true` accepted as 2 for minijinja compatibility), `separators` (a two element tuple, defaulting to `(", ", ": ")` with no indent and `(",", ": ")` with one), and `sort_keys` (recursive, Unicode code point order). `assert_all_used` is still called, so an unknown kwarg remains an error rather than being silently ignored. - The emitter walks the `minijinja::Value` directly rather than converting to `serde_json::Value` first. That keeps integers and floats apart (`2` vs `2.0`), keeps object keys in insertion order unless `sort_keys` is set, and preserves non-finite floats, which `serde_json::to_value` turns into `null` (`Number::from_f64(f).map_or(Value::Null, Value::Number)`) and CPython writes bare as `NaN` / `Infinity` / `-Infinity`. This is a deliberate departure from the implementation plan in the issue, which prescribed `serde_json::to_value`; that route cannot satisfy the issue's own non-finite requirement. - String escaping follows CPython's table: `"` and `\`, the short forms `\b \f \n \r \t`, other C0 controls as lowercase `\uXXXX`, U+007F left alone unless `ensure_ascii`, and code points above the BMP written as a UTF-16 surrogate pair. Jinja's HTML-safe escaping of `<`, `>`, `&` and `'` is deliberately not applied: `transformers` does not apply it, and a JSON Schema `pattern` full of comparison operators has to reach the prompt intact. Note that this is a real behavior change beyond spacing, since minijinja's builtin does perform that escaping. - Float formatting is a separate function with its own tests, because Rust's `{}` for `f64` differs from CPython's `repr` on three independent points: the `.0` suffix for an integral float, the decimal-exponent thresholds where CPython switches to exponent form (`<= -4` or `> 16`, so `0.0001` stays fixed while `0.00001` becomes `1e-05`, and `1e15` stays fixed while `1e16` becomes `1e+16`), and the signed, at-least-two-digit exponent. The shortest round-trip digits come from Rust's `LowerExp`, which matches CPython's `_Py_dg_dtoa` mode 0. - **`src/server/chat_template.rs`** registers the filter next to `raise_exception` and `strftime_now` inside `configure_environment`, the single funnel the project's chat-template security convention requires, and the module docs now describe the template environment the renderer provides. The `supports_tools` heuristic is untouched. - **`docs/supported-models.md`** gains a subsection describing the filter's accepted arguments, its limits, and its two user-visible consequences. ## Bounds The value being serialized comes from the client request body while the arguments come from the template, and minijinja's fuel budget counts VM instructions rather than bytes emitted or frames entered inside a single filter call. Three limits bound the filter, all set far above any legitimate render because crossing one fails the render and costs the prompt its tool declarations, which is the failure this PR exists to prevent: - `indent` is clamped to 64 columns. It is multiplied by the nesting level to produce literal padding, so without a clamp `width * level` can overflow `usize` and a merely large width emits gigabytes of spaces. Clamping rather than rejecting keeps a template that asks for generous indentation working. - A single call emits at most 64 MiB, checked on entry to every node so the walk stops shortly after crossing rather than after rendering everything. `tojson(indent=4)` is not hypothetical: the Llama 3.1, Llama 4 Scout and Granite templates all call it, and pretty-printing plus `ensure_ascii` amplify their input substantially. - Container nesting is capped at 192 levels. That deliberately leaves headroom over the 128-level limit serde_json applies to the request body, because `chat_request` re-parses `function.arguments` out of its wire string and splices the result back into the message tree at roughly depth 4, and that inner parse gets a fresh 128-level budget of its own, so an assembled `messages` value can legitimately reach about 132. A cap of exactly 128 would therefore reject a valid request and fall back to the generic prompt. A `const` assertion at the definition site fails the build if the headroom is removed. `separators` is also checked for arity after taking three elements rather than after collecting the whole sequence, so a large lazy sequence cannot allocate before the rejection fires. ## Known limitations - `sort_keys` sorts the stringified keys, while CPython sorts the original key objects, so numeric keys order differently (`{10: .., 9: ..}` gives `10` first here and `9` first in CPython). Only a template-authored dict literal can produce a non-string key; keys arriving as JSON from a client are always strings, where the two orders agree. Recorded in a code comment. - Sequences are materialized into a `Vec<Value>` before writing, so a template serializing a very large lazy iterable (`range(...) | tojson`) allocates before emitting. The builtin streamed through serde instead. This is not reachable from client JSON, only from a template that authors such a range. - `indent=true` means 2 here and 1 in CPython (where `True == 1`). This preserves the pre-existing minijinja behavior so a template written against minijinja does not change meaning, and it is the one known exception to the byte-identical claim. - `json.dumps`'s `default=`, an `indent` given as a string, and `separators` given as a 2-character string are all rejected rather than supported. No published template uses them. ## Test-pin movements Two pinned Muse Glimmer render digests and one `chat_request` assertion move, and both movements are the intended behavior change rather than collateral. The Muse Glimmer template serializes each tool's JSON Schema with `fn.parameters | tojson`, so each affected render grows by exactly the 13 bytes of `json.dumps` separator spacing (5 commas and 8 colons in `weather_tool`'s schema). This was confirmed two ways: substituting the compact schema back into either new render reproduces the previous digest exactly, and rendering the same template in real Jinja2 3.1.6 reproduces both the old and the new pinned digests bit for bit. `prepare_chat_request_tojson_templates_render_normalized_arguments` pinned `{"path":"/foo","recursive":true}` and now pins `{"path": "/foo", "recursive": true}` for the same reason. The two pre-existing `tojson` regression tests (`test_tojson_filter_serializes_tools_into_prompt` and the `preserve_order` guard `test_tool_parameter_key_order_preserved_through_render`) are separator-agnostic and still pass unchanged. Reviewing those digests surfaced a separate, pre-existing defect that this PR neither causes nor fixes: the pinned Muse Glimmer renders encode `<|eom|>` where the reference renderer emits `<|eot|>`, on both sides of this change. That is tracked as lablup#1383 and will move these digests again when it is fixed. ## Test plan - [x] `cargo test --workspace --profile test-fast --features metal,accelerate`, the documented gate: 8294 passed, 0 failed. That includes 5804 root-lib tests and 1521 `mlxcel-core` tests, so the workspace scope is real rather than the root package alone. - [x] `cargo clippy --workspace --all-targets --features metal,accelerate -- -D warnings`: clean. - [x] `cargo fmt --all -- --check`: clean. - [x] `python3 scripts/ci/check_cross_repo_refs.py`: clean. - [x] The filter's output was differentially fuzzed against real CPython `json.dumps` during review: roughly 400,000 random f64 bit patterns plus targeted sweeps across every exponent threshold against `repr`, and 3,840,000 comparisons over random nested structures crossed with `ensure_ascii`, `indent`, `separators` and `sort_keys`, with payloads covering C0 controls, U+007F, non-ASCII, astral-plane characters, quotes and backslashes. Zero mismatches. - [x] Every expected string in the committed tests came from running CPython `json.dumps` / `repr` and pasting the result, not from writing it by hand. - [x] New tests for the bounds: indent clamping, separator arity rejection, and depth-limit behavior at the cap. Not verified here: the real-checkpoint comparison against `poolside/Laguna-XS-2.1-NVFP4` from the issue's validation section (b). That checkpoint needs the NVFP4 loader, which is a separate issue, so template rendering is covered by the unit tests above instead. Separately, `server::reasoning_effort_tests` overflows the default 2 MiB test-thread stack under the `dev` profile while rendering the large Qwen3.8 template. That reproduces with this filter registration reverted, so it predates this branch and does not occur under the `test-fast` gate profile; it is tracked as lablup#1384. Closes lablup#1309
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.
Summary
mlxcel rendered chat templates with minijinja's builtin
tojson, which accepts onlyindentand always emits serde_json's compact form. Published templates are written againsttransformers, where Jinja'stojsonis CPythonjson.dumpswithensure_ascii=Falseprefilled, so template authors call it withjson.dumpskeyword arguments and expectjson.dumpsoutput. This registers an mlxcel-ownedtojsonfilter that shadows the builtin and reproducesjson.dumpsbyte for byte for the argument combinations templates actually use.Two failure modes are fixed. The loud one:
poolside/Laguna-XS-2.1callstojson(ensure_ascii=False)andthinkingmachines/Inkling-Smallcallstojson(sort_keys=true, separators=(",", ":")), and the builtin'sKwargs::assert_all_usedrejected both outright, so the whole render aborted and the request degraded to the genericUser:/Assistant:prompt with no tool declarations and no tool-call history. The silent one: a barex | tojsonemitted{"a":1}where the reference renderer emits{"a": 1}, so tool schemas reaching the model tokenized differently from the prompt its provider trained and evaluated against.What changed
src/server/chat_template_json.rs(new) implementspython_tojson:ensure_ascii(defaultfalse, which is transformers' default rather than CPython's ownTrue),indent(positional or keyword,trueaccepted as 2 for minijinja compatibility),separators(a two element tuple, defaulting to(", ", ": ")with no indent and(",", ": ")with one), andsort_keys(recursive, Unicode code point order).assert_all_usedis still called, so an unknown kwarg remains an error rather than being silently ignored.minijinja::Valuedirectly rather than converting toserde_json::Valuefirst. That keeps integers and floats apart (2vs2.0), keeps object keys in insertion order unlesssort_keysis set, and preserves non-finite floats, whichserde_json::to_valueturns intonull(Number::from_f64(f).map_or(Value::Null, Value::Number)) and CPython writes bare asNaN/Infinity/-Infinity. This is a deliberate departure from the implementation plan in the issue, which prescribedserde_json::to_value; that route cannot satisfy the issue's own non-finite requirement."and\, the short forms\b \f \n \r \t, other C0 controls as lowercase\uXXXX, U+007F left alone unlessensure_ascii, and code points above the BMP written as a UTF-16 surrogate pair. Jinja's HTML-safe escaping of<,>,&and'is deliberately not applied:transformersdoes not apply it, and a JSON Schemapatternfull of comparison operators has to reach the prompt intact. Note that this is a real behavior change beyond spacing, since minijinja's builtin does perform that escaping.{}forf64differs from CPython'srepron three independent points: the.0suffix for an integral float, the decimal-exponent thresholds where CPython switches to exponent form (<= -4or> 16, so0.0001stays fixed while0.00001becomes1e-05, and1e15stays fixed while1e16becomes1e+16), and the signed, at-least-two-digit exponent. The shortest round-trip digits come from Rust'sLowerExp, which matches CPython's_Py_dg_dtoamode 0.src/server/chat_template.rsregisters the filter next toraise_exceptionandstrftime_nowinsideconfigure_environment, the single funnel the project's chat-template security convention requires, and the module docs now describe the template environment the renderer provides. Thesupports_toolsheuristic is untouched.docs/supported-models.mdgains a subsection describing the filter's accepted arguments, its limits, and its two user-visible consequences.Bounds
The value being serialized comes from the client request body while the arguments come from the template, and minijinja's fuel budget counts VM instructions rather than bytes emitted or frames entered inside a single filter call. Three limits bound the filter, all set far above any legitimate render because crossing one fails the render and costs the prompt its tool declarations, which is the failure this PR exists to prevent:
indentis clamped to 64 columns. It is multiplied by the nesting level to produce literal padding, so without a clampwidth * levelcan overflowusizeand a merely large width emits gigabytes of spaces. Clamping rather than rejecting keeps a template that asks for generous indentation working.tojson(indent=4)is not hypothetical: the Llama 3.1, Llama 4 Scout and Granite templates all call it, and pretty-printing plusensure_asciiamplify their input substantially.chat_requestre-parsesfunction.argumentsout of its wire string and splices the result back into the message tree at roughly depth 4, and that inner parse gets a fresh 128-level budget of its own, so an assembledmessagesvalue can legitimately reach about 132. A cap of exactly 128 would therefore reject a valid request and fall back to the generic prompt. Aconstassertion at the definition site fails the build if the headroom is removed.separatorsis also checked for arity after taking three elements rather than after collecting the whole sequence, so a large lazy sequence cannot allocate before the rejection fires.Known limitations
sort_keyssorts the stringified keys, while CPython sorts the original key objects, so numeric keys order differently ({10: .., 9: ..}gives10first here and9first in CPython). Only a template-authored dict literal can produce a non-string key; keys arriving as JSON from a client are always strings, where the two orders agree. Recorded in a code comment.Vec<Value>before writing, so a template serializing a very large lazy iterable (range(...) | tojson) allocates before emitting. The builtin streamed through serde instead. This is not reachable from client JSON, only from a template that authors such a range.indent=truemeans 2 here and 1 in CPython (whereTrue == 1). This preserves the pre-existing minijinja behavior so a template written against minijinja does not change meaning, and it is the one known exception to the byte-identical claim.json.dumps'sdefault=, anindentgiven as a string, andseparatorsgiven as a 2-character string are all rejected rather than supported. No published template uses them.Test-pin movements
Two pinned Muse Glimmer render digests and one
chat_requestassertion move, and both movements are the intended behavior change rather than collateral. The Muse Glimmer template serializes each tool's JSON Schema withfn.parameters | tojson, so each affected render grows by exactly the 13 bytes ofjson.dumpsseparator spacing (5 commas and 8 colons inweather_tool's schema). This was confirmed two ways: substituting the compact schema back into either new render reproduces the previous digest exactly, and rendering the same template in real Jinja2 3.1.6 reproduces both the old and the new pinned digests bit for bit.prepare_chat_request_tojson_templates_render_normalized_argumentspinned{"path":"/foo","recursive":true}and now pins{"path": "/foo", "recursive": true}for the same reason. The two pre-existingtojsonregression tests (test_tojson_filter_serializes_tools_into_promptand thepreserve_orderguardtest_tool_parameter_key_order_preserved_through_render) are separator-agnostic and still pass unchanged.Reviewing those digests surfaced a separate, pre-existing defect that this PR neither causes nor fixes: the pinned Muse Glimmer renders encode
<|eom|>where the reference renderer emits<|eot|>, on both sides of this change. That is tracked as #1383 and will move these digests again when it is fixed.Test plan
cargo test --workspace --profile test-fast --features metal,accelerate, the documented gate: 8294 passed, 0 failed. That includes 5804 root-lib tests and 1521mlxcel-coretests, so the workspace scope is real rather than the root package alone.cargo clippy --workspace --all-targets --features metal,accelerate -- -D warnings: clean.cargo fmt --all -- --check: clean.python3 scripts/ci/check_cross_repo_refs.py: clean.json.dumpsduring review: roughly 400,000 random f64 bit patterns plus targeted sweeps across every exponent threshold againstrepr, and 3,840,000 comparisons over random nested structures crossed withensure_ascii,indent,separatorsandsort_keys, with payloads covering C0 controls, U+007F, non-ASCII, astral-plane characters, quotes and backslashes. Zero mismatches.json.dumps/reprand pasting the result, not from writing it by hand.Not verified here: the real-checkpoint comparison against
poolside/Laguna-XS-2.1-NVFP4from the issue's validation section (b). That checkpoint needs the NVFP4 loader, which is a separate issue, so template rendering is covered by the unit tests above instead. Separately,server::reasoning_effort_testsoverflows the default 2 MiB test-thread stack under thedevprofile while rendering the large Qwen3.8 template. That reproduces with this filter registration reverted, so it predates this branch and does not occur under thetest-fastgate profile; it is tracked as #1384.Closes #1309