[Rust Frontend] Fix GLM-5.2 chat template rendering parity - #51426
Conversation
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Woosuk Kwon <woosuk@inferact.ai>
|
This pull request has merge conflicts that must be resolved before it can be |
Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Woosuk Kwon <woosuk@inferact.ai>
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
/ci run |
|
✅ Triggered Buildkite CI #84301 for commit |
|
@codex review |
This was written by Codex btw |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fba49b03b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .map(|tool| { | ||
| let mut function = serde_json::Map::new(); | ||
| function.insert("name".to_string(), JsonValue::String(tool.name.clone())); | ||
| if let Some(description) = &tool.description { |
There was a problem hiding this comment.
Preserve null descriptions in template tools
When a tool omits its optional description, this branch drops description from the template context entirely. The Python frontend uses tool.model_dump() (vllm/renderers/online_renderer.py:180), while FunctionDefinition retains description=None and only removes absent strict/defer_loading (vllm/entrypoints/openai/engine/protocol.py:299-314), so Python supplies description: null. Templates that serialize tools or distinguish undefined from null—such as tool_chat_template_functiongemma.jinja:18—will therefore render different prompts and token sequences, whereas the previous typed serializer preserved null. Insert JsonValue::Null at this canonical position when description is None, while continuing to omit absent strict.
Useful? React with 👍 / 👎.
Good catch! It turns out the most recent release of minijinja v0.24 has resolved this issue (mitsuhiko/minijinja#920) so we can stick to structs. I'll push some more commits. |
My last |
|
😄 Fair point—the latest No code changes, commit, or follow-up pull request were created. |
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
|
/ci run |
|
✅ Triggered Buildkite CI #84316 for commit |
…ect#51426) Co-authored-by: Bugen Zhao <i@bugenzhao.com> Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Bugen Zhao <i@bugenzhao.com> Signed-off-by: Woosuk Kwon <woosuk@inferact.ai> Signed-off-by: Zhu, Zufang <zufang.zhu@intel.com>
Purpose
Fix GLM-5.2 chat-template rendering and align Rust tool serialization with the Python frontend.
The MiniJinja 2.22 update from #51235 is now in
main. Relative to currentmain, this PR:values.0.name;name,description,parameters, thenstrictwhen present;descriptionserializes asnull, a nullparametersstaysnull, an absentstrictis omitted, andstrict: falseis retained;zai-org/GLM-5.2-FP8render/parse roundtrip covering both reasoning-plus-content and mixed tool-call fixtures.Version boundary
unexpected float, expected identifier or integer.preserve_orderfeature, also preserves Serde struct definition order during template iteration. This closes the strict Python prompt-parity gap while allowing the tool projection to remain typed and express field absence with Serde attributes.Test Plan
The GLM-5.2 roundtrip loads the real tokenizer and bundled chat template, renders fixed conversations, parses the assistant output, appends it to history, and renders the next turn.
Test Result
Red/green evidence:
chat:93.description|name|parameters|.name|description|parameters|.descriptionwhen absent and fails the optional-field regression test.description=null,parameters=null, omitted absentstrict, and retainedstrict=false.Final validation:
roundtrip_glm52: passed on both reasoning-plus-content and mixed tool-call fixtures.cargo fmt, Clippy with warnings denied, andgit diff --check: passed.b3d3bfd8964d9bb3998d4e96a7e7c5d8adc36478e0dbdd4d26ecd3ef31acfd28.AI assistance was used for source analysis, implementation, testing, and drafting this description. The submitter must review every changed line and validate the results before marking this PR ready for review.