Repository navigation
feat(symphony): Qwen3 speaks the tagged call syntax, and starts inside a thought the prompt opened - #2838
Conversation
…dds nothing
BFCL declares a list whose members come from an enum as
{"type": "array", "items": {"type": "string"}, "enum": [...]}. Kind::of
added `string` for the enum and read the parameter as a string, so the
value `[]` reached the client as "[]". vLLM reads `type` when it is there;
now so does Symphony, and the enum of strings speaks only for a schema
with no `type`. Found by replaying bellwether's qwen3-coder-next parse
cases (bfcl-live-multiple-146-58-0).
Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…e a thought the prompt opened The format takes a CallSyntax: Json (Qwen3's object) or Tagged with the request's declared parameter types (Qwen 3.5 and later, Qwen3-Coder), and runs the matching assembler inside the call region; Qwen3::with_tagged_calls builds the second. Everything else about the region is shared: the index, the id, the surplus after the call, the end of the stream and the early close by marker. The prompt now decides where the output starts. Qwen 3.5's template opens <think> in the generation prompt, so the output starts inside the thought and its first </think> closes it; a prompt whose last <think> comes after its last </think> starts the output in reasoning, with ReasoningStart pushed for the prompt. Qwen3's prompt opens nothing, and a prompt that disables thinking closes it itself, so nothing changes for them. The parity test replays the tagged models bellwether has recorded (qwen3.5-27b, qwen3.6-27b, qwen3.8-27b, qwen3-coder-30b-a3b-instruct, qwen3-coder-next; qwen3.5-9b when it is), each after the prompt tail its template leaves, read from the case's request for thinking off. It allows two classes of difference the corpus itself has and counts them: a reference argument whose type the output cannot carry (bellwether #56 refuses those at record time) and reasoning in a reference whose template writes no thought (bellwether #52). On the scale-run fixtures of 2026-10-06 that is 0 differences: per thinking model 2436 cases, 7 bitwise, 2312 separators only, 2 listed, 115 allowed; per coder model 2435 cases, 2313 bitwise, 2 separators only, 1 listed, 119 allowed. For Qwen3.6-27B the 112 allowed cases of three sets are exactly the 112 that bellwether #56 refuses. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com> Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
github-actions
Bot
added
tests
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughQwen3 now supports JSON and tagged tool-call syntax. Prompt markers determine whether output starts in reasoning or content. The crate exports the new call syntax and tool declaration types. Tagged-model fixture parity checks now account for prompt tails and specified corpus differences. ChangesQwen3 Tagged Call Parsing
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant InputClient
participant Qwen3
participant TaggedAssembler
participant EventStream
InputClient->>Qwen3: Supply prompt and output chunks
Qwen3->>TaggedAssembler: Feed tagged call bytes and declared tools
TaggedAssembler-->>Qwen3: Return call events
Qwen3->>EventStream: Emit relabeled events
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change adds tagged tool-call parsing for Qwen3 and prompt-based reasoning detection. No actionable merge-blocking risk was identified, and the author reports that the standard checks passed. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| /// Whether the prompt ends inside the thinking region: its last `<think>` comes after its last | ||
| /// `</think>`. | ||
| fn leaves_thinking_open(prompt: &str) -> bool { |
There was a problem hiding this comment.
🔴 Important: This scans the whole rendered prompt, so a literal <think> anywhere in the conversation flips the output into reasoning. That includes a user message, a system prompt, or a tool result.
Input::Prompt::text is the full prompt "decoded with special tokens kept". Take a conversation where any earlier message has a <think> with no </think> after it. Some examples:
- a user asks "why does my output start with
<think>?" - Qwen3-Coder reads a file or grep result that contains
"<think>" - an earlier assistant turn was cut by length inside its thought, which the Qwen3 template keeps as-is
In each case rfind("<think>") > rfind("</think>"), and the parser starts in Region::Reasoning. For a model that never writes </think> (Qwen3-Coder, Qwen3-Instruct-2507), nothing ever closes that region:
- The whole answer comes back as
reasoning_contentinstead ofcontent. - Every
<tool_call>hits the(Region::Reasoning, _)arm inmarker()and becomes reasoning text, so the tool calls are lost without any error.
This also changes Qwen3::new(), which used to ignore the prompt.
The prompt only decides anything inside the turn being generated, so scope the search to that turn:
fn leaves_thinking_open(prompt: &str) -> bool {
// Only the turn being generated: a `<think>` quoted in an earlier message opens nothing.
let turn = prompt.rfind("<|im_start|>").map_or(prompt, |at| &prompt[at..]);
match (turn.rfind(MARKERS[THINK_OPEN]), turn.rfind(MARKERS[THINK_CLOSE])) {
(Some(open), Some(close)) => open > close,
(Some(_), None) => true,
(None, _) => false,
}
}Please also add a case to a_prompt_that_opens_the_thought_starts_the_output_in_reasoning. Use a prompt like "<|im_start|>user\nwhat is <think>?<|im_end|>\n<|im_start|>assistant\n" with TAGGED_CALL as output, and assert that the output starts in content and the call is parsed.
| continue; | ||
| } | ||
| let spelled_the_same = match (value, reference) { | ||
| (serde_json::Value::String(text), reference) if !reference.is_string() => { |
There was a problem hiding this comment.
🟡 Nit: DeclaredTypeConflict is broader than its doc says, and it can hide the very regression this PR fixes.
The doc on the variant (line 88) says the reference's argument is "a boolean, a number or null for a parameter the tool declares string". The match arm checks neither part:
- It accepts any non-string reference, including arrays and objects, compared by
reference.to_string(). - It never looks up what the tool actually declares.
So before the fix(symphony) commit, the BFCL case bfcl-live-multiple-146-58-0 would land here. The parser said {"metrics": "[]"} and the reference has {"metrics": []}. Since Value::Array([]).to_string() == "[]", that case would be printed as "allowed: declared-type conflict (corpus)" instead of failing. The same goes for any future Kind::of regression that turns a declared integer, number, boolean, or array into a string. That contradicts the module doc's claim (line 19) that "the classes cannot hide anything else".
Cheapest fix: match the doc and allow only scalar references:
(serde_json::Value::String(text), reference @ (serde_json::Value::Bool(_) | serde_json::Value::Number(_) | serde_json::Value::Null)) => {Stricter fix: pass the fixture's Declared in and require declared.kind(name, key) to be Some(Kind::String | Kind::NullableString). That way the allowance only covers parameters the tool really declares string.
| /// Text after a call's object and before its closing marker: each run of whitespace is the | ||
| /// template's wrapping and is dropped, each run of anything else is malformed. Classifying run | ||
| /// by run keeps the result the same wherever the chunks were cut. | ||
| /// Text after a call and before its closing marker: each run of whitespace is the template's |
There was a problem hiding this comment.
🟡 Nit: This doc comment now says "after a call" so it covers both syntaxes. But the Malformed reason it emits is still TEXT_AFTER_THE_OBJECT (line 70): "text between a call's object and its closing marker". With the tagged syntax there is no object; the surplus comes after </function>. Clients and logs will see that reason for tagged calls, so it should be reworded to match. For example: "text between a call and its closing marker".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Brings the three commits main gained since eda1134 (#2835 Cargo.lock's toml entry, #2833 the tagged discovery config with one runtime conversion, #2838 Qwen3's tagged call syntax) under the leap branch. Resolutions, both import lists: - model_gateway/src/config/builder.rs: the `crate::config` import takes main's `KubernetesDiscoveryConfig` and the leap's `KvIndexKind`, in rustfmt order. - model_gateway/src/main.rs: the same pair in the same list. Cargo.lock, bindings/python/src/lib.rs, config/types.rs and config/validation.rs merged on their own; the bindings build the discovery config in main's enum form and the runtime conversion is main's. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
1 participant
Description
Follows #2831 (merged as 396c78e) on main. Part of the Symphony plan (S2, the Qwen family's second call syntax).
Problem
Qwen3reads a call only as Qwen3 writes it, a JSON object between<tool_call>and</tool_call>. Qwen 3.5 and later and Qwen3-Coder write the same region as tags,<function=NAME>and<parameter=KEY>around each value's text, which #2823 and #2831 taught thetaggedassembler to turn into events; no format ran it yet. Replaying bellwether's recorded cases for those models also showed two things the format did not know: Qwen 3.5's template opens<think>in the generation prompt, so the output starts inside the thought and a parser that waits for<think>reports\n</think>\n\nas content; and BFCL declares a list whose members come from an enum as{"type": "array", "enum": [...]}, whichKind::ofread as a string, so[]reached the client as"[]".Solution
CallSyntax::{Json, Tagged(Declared)}. The format holds the syntax and runs the matching assembler inside the call region; everything else about the region is shared (the index and id, the surplus after the call, the end of the stream, the early close by marker).Qwen3::new()is the JSON syntax as before;Qwen3::with_tagged_calls(Declared::of(&request.tools))is the tagged one. Both are exported withDeclared.<think>comes after its last</think>starts the output in reasoning, withReasoningStartpushed for the prompt (leaves_thinking_open). Qwen3's prompt opens nothing, and a prompt that disables thinking ends with<think>\n\n</think>\n\nand leaves nothing open, so nothing changes for them. The module doc's sentence that the prompt was ignored is replaced by this rule.typedecides alone (fix(symphony)commit): the enum of strings speaks only for a schema with notype, as vLLM readstypewhen it is there.tagged_parse_fixtures_match_the_referencereplays each tagged slug bellwether has recorded throughwith_tagged_calls, typed by the case's request tools, after the prompt tail its template leaves (<think>\n;<think>\n\n</think>\n\nwhen the request'schat_template_kwargsturn thinking off; nothing for Qwen3-Coder). The fixtures carry the request and the output but not the rendered prompt, so the tail is stated in the test per model; ageneration_promptfield on bellwether's parse cases would remove that, and it is on Simo's list. The known-difference list matches a case's id after its slug, so one list serves every model.falseforsmoking_allowed, declared a string enum of"True","False","dontcare";2fornumber_of_roomsdeclared a string;nullfortodeclared a string. The template writes the value as it writes the string, the parser gives the string back as vLLM v0.31.0 does, and bellwether Extract multimodal module into standalone llm-multimodal crate #56 refuses such a case at record time; the fixtures replayed here predate it. (2) Reasoning in a reference whose template writes no thought (Qwen3-Coder), which the output cannot carry: bellwether refactor: Extract MCP module into standalone workspace crate #52.Changes
crates/symphony/src/formats/qwen3.rs:CallSyntax,enum Call,with_tagged_calls, the prompt rule, module doc; five new tests (a tagged call after thinking, two tagged calls and an undeclared tool, the early close and the cut stream, every chunking of a tagged output, the prompt that opens the thought).crates/symphony/src/formats/mod.rs,src/lib.rs: exports.crates/symphony/src/tagged/value.rs:admitted_typesreads the enum only without atype; doc; two tests.crates/symphony/src/tagged/assembler.rs: one test, the BFCL array shape through the assembler.crates/symphony/tests/bellwether_parse_fixtures.rs:TAGGEDwithGenerationPrompt,Request.chat_template_kwargs,Allowance,paritytaking the prompt tail and the allowances,KNOWN_TAGGED_DIFFERENCES, suffix-matched known ids;tests/common/mod.rs:replay_after.Test Plan
cargo +nightly fmt -p smg-symphony --check,cargo clippy -p smg-symphony --all-targets --all-features -- -D warnings,cargo test -p smg-symphony(177 lib tests, 3 fixture tests skipping withoutBELLWETHER_FIXTURES, 7 contract tests),RUSTDOCFLAGS="-D warnings" cargo doc -p smg-symphony --no-deps: all exit 0 at 5c61011 (on main eda1134).Preview against bellwether's scale-run fixtures of 2026-10-06 (local, not CI; CI's pinned fixtures have Qwen3-8B only).
BELLWETHER_FIXTURES=<plain tree> cargo test -p smg-symphony --test bellwether_parse_fixtures tagged_parse_fixtures_match_the_referencepasses, 0 differences, 239 s:reasoning-parsercrate #17)protocolscrate #16)qwen3.5-9b has no parse sets yet and is skipped with a notice. Before the prompt rule, the thinking models had 7 bitwise and 2427 differences (
content: Some("\n</think>\n\n")); before thetypefix, the coder models had 2313 bitwise and 120 differences.Cross-check of the allowance:
bellwether recordat Extract multimodal module into standalone llm-multimodal crate #56's head, offline, on Qwen3.6-27B'sbfcl-live-multiple,bfcl-live-simpleandbfcl-parallel-multiple, refuses 112 cases; the allowed cases of this test on the same three sets are the same 112 ids.The Qwen3-8B tests are unchanged in what they accept: the same two known differences, now matched after the slug.
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses