Repository navigation
[model-gateway]: add qwen coder tool parser support xml format for qwen3 coder and microthinker - #12909
Conversation
Summary of ChangesHello @whybeyoung, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Qwen tool parser within the Rust Router by introducing comprehensive support for XML-formatted tool calls. This update ensures that the router can correctly interpret and process tool calls from Qwen models that utilize an XML structure, providing parity with existing Python implementations and improving the overall compatibility and robustness of the system. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for parsing XML-formatted tool calls from Qwen models, which is a great feature enhancement. The implementation covers both complete and incremental (streaming) parsing, and includes logic to detect the format (JSON vs. XML) automatically. The changes are well-structured, but I've identified a few critical performance and correctness issues. Specifically, regular expressions are being recompiled on every function call, which will degrade performance, and the logic for building JSON strings during streaming is brittle and can produce invalid output. I've also included some suggestions to improve code maintainability by reducing duplication and adhering to standard Rust conventions.
|
Why don't we have a qwen3_coder_parser.rs separately? |
It seems overly simplistic to assume that all of Qwen's tools should be implemented here. If we implement them separately, wouldn't we have to designate Qwen3-Coder specifically for the parser? There appears to be a significant amount of redundancy in this approach, so implementing them within Qwen itself shouldn't pose major issues, right? |
CatherineSue
left a comment
There was a problem hiding this comment.
I understand there might be a lot duplications between qwen.rs and qwen-coder. However, the current architecture is a big if-else to me at the moment.
The logic that parses xml in complete or incremental style looks good to me. But my biggest concern is the fall back logic or how do we manage the is_xml_format or is_json_format state thru incremental parsing. I am concerned with future debug and maintenance.
From what we know, qwen3-coder or qwen3-xml-format is specific to qwen3-coder models. The other qwen family models use the json format. It might be clear if we separate qwen-xml.rs from qwen.rs. And in the tool parser registry, we simply add a qwen3-coder match for qwen-xml.
Let me know what you think @whybeyoung @slin1237
If we want to keep them in the same file, then let's add through tests, as detailed as possible. We should add tests nevertheless. But if we are gonna keep them in the same file, more test cases should be covered.
make sense. I agree with the approach of separating them into individual files, and I will fix this following the separation pattern. @CatherineSue |
21df286 to
1d6d1c3
Compare
e778a6f to
9e8b528
Compare
|
/gemini review |
9e8b528 to
bc6941c
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a new QwenCoderParser to handle XML-based tool calls, analogous to the Python implementation. The changes include adding the parser, integrating it into the factory, and providing comprehensive tests. The implementation is robust, covering both complete and incremental parsing, including various edge cases. I've identified a couple of areas for improvement, mainly related to code duplication and a potential bug in JSON fragment construction during streaming. Overall, this is a solid contribution that extends the gateway's capabilities.
| println!("Chunk {}: {:?}", i, chunk); | ||
| println!(" Calls: {:?}", result.calls); | ||
| println!(" Normal text: {:?}", result.normal_text); |
bc6941c to
ab911e8
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new QwenCoderParser to handle XML-based tool calls from Qwen Coder models, which is a great addition. The changes include the parser implementation, factory registration, model mappings, and an extensive suite of unit and integration tests. The implementation is thorough and the test coverage is excellent. I've identified a high-severity correctness issue in the html_unescape helper function that could lead to data corruption, and a medium-severity issue with an unused parameter. With these fixes, this will be a very solid contribution.
3aff23d to
53b1b46
Compare
|
/gemini reviewww |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
010480a to
50a3058
Compare
50a3058 to
278a63c
Compare
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
should we move the tests here to tool_parser_qwen_coder.rs?
…en3 coder and microthinker (sgl-project#12909)
…en3 coder and microthinker (sgl-project#12909)
Summary
Add a new Qwen Coder XML format tool parser for the Rust Router (sgl-model-gateway).
Qwen Coder models use a distinct XML-based tool call format that differs from the standard Qwen JSON format:
This PR implements full support for parsing this format in both complete and streaming modes.
Motivation
Reference: https://huggingface.co/Qwen/Qwen3-Coder-480B-A35B-Instruct-FP8?chat_template=default
Changes
New Parser: QwenCoderParser
Model Auto-Detection
Added model mappings in factory.rs:
registry.map_model("Qwen/Qwen3-Coder*", "qwen_coder");
registry.map_model("Qwen3-Coder*", "qwen_coder");
registry.map_model("qwen3-coder*", "qwen_coder");
registry.map_model("Qwen/Qwen2.5-Coder*", "qwen_coder");
registry.map_model("Qwen2.5-Coder*", "qwen_coder");
registry.map_model("qwen2.5-coder*", "qwen_coder");
Users can also explicitly use
--tool-call-parser qwen_coder.Test Coverage
Test categories:
Test Plan