feat(parsers): add Qwen3 unified request modes - #174
Conversation
|
📊 Conformance matrix rendered — view in CI summary |
5f11b5f to
8c5d212
Compare
… Dynamo request modes Puts gemma4 on the UnifiedParser: ONE state machine per stream that owns reasoning, visible content and tool calls and emits a single ordered event list. gemma4 only; every other family keeps the split path unchanged. `unified/gemma4.rs` is a factory over the shared `ScannerUnified` — the family supplies its markers, its emitter and a `ReasoningSpec`. Registration is one line in `unified_registry!` plus one flag in the family manifest. `ReasoningSpec.start_label` is new: gemma4's channel opener carries a `thought\n` role label that is structural and must be stripped from the thought. It stays a separate field rather than being folded into `start`, because folding it makes a plain `<|channel>` open nothing and leak its markup as visible text (I3) in exactly the case the label is absent (P4). `opener_len_at` is a thin caller of `reasoning_opener_len` in scan.rs, and the guided drain calls the same function — ONE owner for both request modes, which is the rule the native and guided paths keep breaking. The matching holdback keeps a label split across a chunk boundary from being released early, so a chunk boundary cannot change the output. Rebased onto #174 (which replaced #163). Two things the rebase surfaced: `ReasoningSpec` gained `start_label`, and #174 added a `guided_reset_...` test fixture that constructs the struct directly. The rebase merged both cleanly and the result did not compile. Fixture now pins `start_label: None`, matching the qwen3 spec it mirrors. Five qwen3 cases still documented a Dynamo divergence (`reason_markup_in_arg`, `reason_markup_in_arg_with_text`, `tool_in_reason`, `tool_in_reason_with_text`, `tool_no_close`) while the live unified parser matches the oracle. That is the exact failure the `SPLIT()` helper's docstring already describes — "when qwen3 joined, its per-case annotations were left claiming MERGE and stayed wrong for a whole release, because the Dynamo column is computed LIVE and nothing reads this field back". These five were hand-written per-family verdicts that bypassed the helper, so they went stale again. All five now route through `SPLIT()`, which yields `match` for a unified family and the divergence for a split one, and the new `a_unified_family_is_never_annotated_as_diverging` test in unified_parity.rs makes the class fail loudly instead of quietly misleading a reader. Corpus. Giving gemma4 a unified parser changes 2 of the 237 inputs — gemma4 and kimi_k2 `UNIFIED.40.b`, where `prefilled_reasoning_with_guided_json` now derives guided JSON per family instead of pinning native markup under a `tool_output_mode=GuidedJson` label, which graded those two against output guided decoding never produces. `golden/` is byte-identical to the base across all 237. Every rendered column is a live capture at 237 = the `inputs/` count: `dynamo_v2-0.1.27` (live), `dynamo_v2-0.1.22` and `0.1.23` (back-captured through the split-only cross-version harness — both predate the unified module), and `vllm_rust-0.25.1`, `vllm_python-0.25.1`, `sglang_python-0.5.16` re-captured against this branch's inputs. The 0.1.22/0.1.23 back-captures reproduce the base's columns byte-for-byte on all 235 unchanged cases, so only the 2 changed cells moved. `dynamo_v2-0.1.25` is left byte-for-byte as shipped. It is a same-label composite built across several commits rather than a single reproducible build, so re-capturing part of it would mix producers under one label. Its two `UNIFIED.40.b` cells therefore still answer the pre-change input; that provenance drift is tracked on #174, not papered over here. Gates (CI's commands): `cargo test --workspace --all-targets --locked` 0 failed · `cargo fmt --check` · `cargo clippy --workspace --all-targets --locked -D warnings` · conformance python 111 non-browser + 25 browser (browser run alone) · `render_table_v2.sh` rc=0 with `postdates that build` and `not captured at` both grepping 0 · `git diff --check` rc=0. Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com>
There was a problem hiding this comment.
Overall, this PR delivers what it claims at the parser-library level for Qwen3.
It introduces one parser that owns reasoning, visible text, and tool calls, and emits them as an ordered event stream.
This fixes the fundamental split-parser ownership problem: a tool call can interrupt reasoning without being swallowed or leaked, while reasoning-like markers inside tool arguments remain argument data.
I also compared this architecture against the parser/tool-leak fixes from the last two months.
It structurally addresses the most important class: reasoning/tool ownership, event ordering, and many chunk-boundary marker leaks for correctly ported marker-based families.
In my understanding What remains(Follow Up PRs) :
- Production serving does not consume UnifiedParser yet, so this is a parser implementation and registration—not a runtime rollout.
- Malformed assembled arguments currently become {}. That can make a corrupted side-effecting call indistinguishable from a legitimate no-argument call; I would prefer failing closed.
- Guided parse failures become visible text plus tracing warnings. A typed diagnostic/error result would let serving distinguish assistant text from invalid constrained output.
- Tool name, schema, tool_choice, and parallel-call validation remain outside the parser. That boundary should be explicit and enforced before dispatch.
- Native and guided modes share the ordered API but use separate internal state machines, so their parity is test-enforced rather than guaranteed by construction.
- Correct prompt-prefilled state depends on caller plumbing. That needs boundary-level serving tests when integration happens.
- The current reasoning grammar is sufficient for Qwen3 and other simple paired-marker families, but not yet a universal replacement for Granite/Inkling/Harmony-style parsers.
Thanks a lot @keivenchang for adding this critical peice!
Second half of the split of PR #174. The first half (peer-trait alignment, vendor registry, GUI and conformance tooling) is DIS-2644; this carries everything that depends on qwen3 request modes and must move atomically with its evidence. WHAT THIS ADDS Qwen3 gains the two request modes the reasoning+tool split cannot honour: UnifiedParserStartingState the prompt already opened a channel, so generated output begins inside reasoning or inside the visible response and closes a marker it never opened. UnifiedToolOutputMode guided decoding emits bare JSON instead of the model's native markup, so the parser must be told which grammar it is reading. Plus the scanner work these require: bounded prefix-header ownership, checkpoints and rollback, guided payload draining, and recovery that never loses bytes. CORPUS The authored feed goes from main's 33 scenarios / 99 cases to 79 / 237. 44 of the 46 added scenarios are request-scoped (groups 30, 31, 40, 41, 50, 51); the other two are generic group-4 cases that travel here because the generator and the LFS corpus have a single owner. Generator, taxonomy, docs, schema consumers, archives and manifest pins move together — an intermediate state with blank historical columns or mismatched pointers is not a reviewable boundary. DOC DRIFT FIXED UNIFIED_CASES.md claimed 63 scenarios / 189 cases while the generator emitted 79/237, said gemma4 runs the native UnifiedParser when `unified_registry!` has exactly one entry (qwen3), and described the 0.1.22 archive as holding 162 entries when it holds 237. All three now match what the generator and archives actually contain. LFS HYGIENE Six archive changes are dropped rather than carried: four toolcalling tarballs whose extracted payloads are IDENTICAL to the merge base (only gzip bytes differed), one whose only change is `captured_with.dynamo_v2` 0.1.23 -> 0.1.24, and two new toolcalling shards. None is request-mode evidence. Their manifest entries revert with them, which the parity test verifies by sha256. Evidence: cargo test --workspace --all-targets --locked 1429 passed, 33 suites, 0 failed golden unified feed, 237 cases sha256 1486bfc6da2a812a8ca1... BYTE-IDENTICAL to the pre-split tree cargo clippy --workspace --all-targets 0 warnings Still open, tracked not fixed: the single-initialization-owner consolidation (UnifiedParserInit) and a caller-actionable signal for malformed guided payloads. Both are reviewer findings with agreed designs; see the open threads on #174. Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com>
eb78c72 to
9ddbc0d
Compare
|
@indrajit96 — heads up that this PR was split after your review, so the code you commented on has moved and the history was rewritten.
Your five comments were against
The two fixed threads are resolved; the three open ones are genuinely open, not stale. I re-verified each against the rewritten branch rather than letting the force-push make them look outdated. Two further things your review led to, which you did not see: an independent audit found that the vendor registry I added afterwards had an alias gap (registering |
… and let vendors supply their own Split out of DIS-2544 / PR #174, which bundled several unrelated concerns into ~7k lines across 56 files and stalled in review. This carries everything that does NOT depend on qwen3 request modes. No corpus change: the unified golden feed is byte-identical to main, enforced by conformance/utils/piece1_coupling_gate.sh. PEER-TRAIT ALIGNMENT UnifiedDelta::Text { text } -> UnifiedParserEvent::Text(String) peer name, peer variant ORDER, peer payload shapes. push (required) -> parse_into (required), push derived from it finish -> Result<Vec<..>> -> finish -> Result<UnifiedParserOutput> + initialize(&[u32]), reset, preserve_special_tokens, tool_call_id + UnifiedParserOutput with the peer accumulation helpers `UnifiedParserOutput` is documented as a CUMULATIVE buffer whose appends coalesce, so a caller cannot index a per-advance window and the built-in agrees with the helpers vendors are told to use. The stay-committed-on-error guarantee is stated on `parse_into`, whose buffer the caller owns; `push` returns `Result<Vec<_>>` and has nowhere to carry partial output. `ScannerUnified` overrides `reset` to drain the scanner and restart the tool index, and a parser that buffers is required to do the same. VENDOR REGISTRY register_unified_parser("acme_v1", factory) add a family register_unified_parser("qwen3", factory) SHADOW a family we ship unregister_unified_parser("qwen3") ours is reachable again Keyed on the CANONICAL family name, so registering `qwen3` also shadows its `qwen3_coder` alias — keying on the caller's spelling meant one family silently ran two different parsers depending on routing. Shadowing is non-destructive. `builtin_unified_families()` excludes vendor entries, because the conformance suite iterates it and a vendor family has no cases here. `CUSTOM_PARSERS.md` documents the trait and the contract a parser must honour. Its worked example is compiled as tests/vendor_parser_example.rs, and a test asserts the Markdown block and the compiled file are the same implementation — the previous "the build fails here first" claim was false, and the two had already drifted. GUI AND CONFORMANCE TOOLING Hover popups no longer depend on a media query. Listener REGISTRATION sat behind `matchMedia('(hover: hover)')`, and Chrome reports that false on machines that deliver real hover — the browser was sending hover events with nothing listening, and the same gate silently removed keyboard access. Listeners are now unconditional; only tap-to-pin consults `PointerEvent.pointerType`, consumed per click so a touch cannot arm a later keyboard activation. Evidence: cargo test --workspace --all-targets --locked 1373 passed, 33 suites, 0 failed python3 -m pytest conformance/utils/tests/ 128 passed cargo clippy --workspace --all-targets 0 warnings piece-1 coupling gate PASS golden.tar.gz / inputs.tar.gz 43db2838... / 70fda09d... unchanged Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com> SEMVER: bumped dynamo-parsers-v2 0.1.27 -> 0.2.0. This removes the public `UnifiedDelta` export and changes the `UnifiedParser` trait surface, so a downstream crate importing `UnifiedDelta` or implementing the old trait stops compiling. Under Cargo's 0.x rules the MINOR position is the breaking one, so 0.2.0 is the honest version. Deprecated compatibility wrappers were considered and rejected: the enum's variants changed shape (`Text { text }` -> `Text(String)`), so a wrapper could not preserve pattern matching, and the crate has no external consumers today — every in-repo dependant is updated in this change.
… and let vendors supply their own Split out of DIS-2544 / PR #174, which bundled several unrelated concerns into ~7k lines across 56 files and stalled in review. This carries everything that does NOT depend on qwen3 request modes. No corpus change: the unified golden feed is byte-identical to main, enforced by conformance/utils/piece1_coupling_gate.sh. PEER-TRAIT ALIGNMENT UnifiedDelta::Text { text } -> UnifiedParserEvent::Text(String) peer name, peer variant ORDER, peer payload shapes. push (required) -> parse_into (required), push derived from it finish -> Result<Vec<..>> -> finish -> Result<UnifiedParserOutput> + initialize(&[u32]), reset, preserve_special_tokens, tool_call_id + UnifiedParserOutput with the peer accumulation helpers `UnifiedParserOutput` is documented as a CUMULATIVE buffer whose appends coalesce, so a caller cannot index a per-advance window and the built-in agrees with the helpers vendors are told to use. The stay-committed-on-error guarantee is stated on `parse_into`, whose buffer the caller owns; `push` returns `Result<Vec<_>>` and has nowhere to carry partial output. `ScannerUnified` overrides `reset` to drain the scanner and restart the tool index, and a parser that buffers is required to do the same. VENDOR REGISTRY register_unified_parser("acme_v1", factory) add a family register_unified_parser("qwen3", factory) SHADOW a family we ship unregister_unified_parser("qwen3") ours is reachable again Keyed on the CANONICAL family name, so registering `qwen3` also shadows its `qwen3_coder` alias — keying on the caller's spelling meant one family silently ran two different parsers depending on routing. Shadowing is non-destructive. `builtin_unified_families()` excludes vendor entries, because the conformance suite iterates it and a vendor family has no cases here. `CUSTOM_PARSERS.md` documents the trait and the contract a parser must honour. Its worked example is compiled as tests/vendor_parser_example.rs, and a test asserts the Markdown block and the compiled file are the same implementation — the previous "the build fails here first" claim was false, and the two had already drifted. GUI AND CONFORMANCE TOOLING Hover popups no longer depend on a media query. Listener REGISTRATION sat behind `matchMedia('(hover: hover)')`, and Chrome reports that false on machines that deliver real hover — the browser was sending hover events with nothing listening, and the same gate silently removed keyboard access. Listeners are now unconditional; only tap-to-pin consults `PointerEvent.pointerType`, consumed per click so a touch cannot arm a later keyboard activation. Evidence: cargo test --workspace --all-targets --locked 1373 passed, 33 suites, 0 failed python3 -m pytest conformance/utils/tests/ 128 passed cargo clippy --workspace --all-targets 0 warnings piece-1 coupling gate PASS golden.tar.gz / inputs.tar.gz 43db2838... / 70fda09d... unchanged Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com> SEMVER: bumped dynamo-parsers-v2 0.1.27 -> 0.2.0. This removes the public `UnifiedDelta` export and changes the `UnifiedParser` trait surface, so a downstream crate importing `UnifiedDelta` or implementing the old trait stops compiling. Under Cargo's 0.x rules the MINOR position is the breaking one, so 0.2.0 is the honest version. Deprecated compatibility wrappers were considered and rejected: the enum's variants changed shape (`Text { text }` -> `Text(String)`), so a wrapper could not preserve pattern matching, and the crate has no external consumers today — every in-repo dependant is updated in this change.
a80d80f to
23981e7
Compare
23981e7 to
059851a
Compare
|
/ok to test 059851a |
|
/ok to test c45b81e |
|
/ok to test 0348f20 |
indrajit96
left a comment
There was a problem hiding this comment.
The overall design looks good, but I found three chunk-boundary correctness issues in guided mode.
In each case, identical input can produce different visible output depending on how the stream is chunked.
Happy to re-review once updated.
indrajit96
left a comment
There was a problem hiding this comment.
LGTM!
Thanks for adding this @keivenchang
|
/ok to test 3a88dac |
qwen3 gains the two request modes the reasoning-then-tool split cannot honour: a prompt-prefilled channel (`UnifiedParserStartingState`) and guided-JSON tool output (`UnifiedToolOutputMode`). Other model families are unchanged. Built on the peer-trait surface from #178: the parser writes committed events straight into the caller's `UnifiedParserOutput`, consumes an invoke only after its emitter succeeds, and returns unconsumed bytes from `reset`. Corpus grows 99 -> 240 cases. Released peer captures are append-only: every pre-existing member of `inputs`, `golden`, `dynamo_v2-0.1.23`, `vllm_python-0.25.1` and `vllm_rust-0.25.1` is byte-identical, with the 138 new cases added alongside and the live parser recorded under a new `dynamo_v2-0.2.0` shard rather than rewriting an older label. Split invariance is enforced by generation, not by example: `generated_control_marker_pairs_are_split_invariant` builds inputs from the marker alphabet, takes all ordered pairs, and asserts one-push output equals every char-boundary split — which is what caught the three cases where a competing marker, a payload boundary before a header, and post-payload whitespace each produced different output depending on chunking. Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com>
3a88dac to
5a907ba
Compare
Overview:
Second half of the split from #178, now based directly on
main. #178 landed the peer-trait alignment, vendor registry, and shared GUI/conformance tooling. This PR carries everything that depends on Qwen3 request modes, plus the corpus evidence that has to move atomically with it.Qwen3 gains the two request modes the reasoning/tool split cannot honor: a prompt-prefilled channel (
UnifiedParserStartingState) and guided-JSON tool output (UnifiedToolOutputMode). Other model families are unchanged.Where this sits
flowchart TB A["#178 · MERGED FOUNDATION · ordered parser API, error-safe recovery, vendor registry"] B["#174 · THIS PR · Qwen3 native + guided-JSON request modes · corpus 99 → 240 cases"] C["#166 · NEXT FAMILY · Gemma 4 reuses the same contract and the same 240 cases"] A ==> B ==> C classDef landed fill:#eef1f4,stroke:#57606a,stroke-width:2px,color:#000 classDef here fill:#fff4ce,stroke:#b06000,stroke-width:4px,color:#000 classDef next fill:#f6f8fa,stroke:#8c959f,stroke-width:2px,color:#000 class A landed class B here class C nextThe request-mode contract, failure behavior, evidence, and exclusions are tracked in DIS-2544. #178 owns the reusable surface; this PR consumes it for Qwen3.
Example (before → after):
Input:
<think>Look it up.</think><tool_call>…</tool_call><think>Now answer.</think>It's 18C.Ordering is not a field the split can add; it is lost at the seam between the two parsers.
Details:
UnifiedParserInit, with bounded prefix-header ownership, checkpoints and rollback, guided payload draining, and native-markup fallback that stays control-only across every stream split. Invalid guided payloads default to a typed caller-actionable error; the conformance harness explicitly selects text recovery.reasoning → tool_call → reasoning → textstream remains intact.UNIFIED_CASES.mdsaid 63 scenarios / 189 cases, claimed Gemma 4 ran the nativeUnifiedParserwhen the registry had only Qwen3, and described the 0.1.22 archive as holding 162 entries. The current generator emits 80 scenarios / 240 cases; the corrected 0.1.22 and 0.1.23 archives each contain all 240 cases.captured_with.dynamo_v20.1.23→0.1.24, and two new tool-calling shards. None is request-mode evidence.Verification:
Where should the reviewer start?
parsers/v2/src/unified/mod.rs(request modes and guided state), thenparsers/v2/src/unified/qwen3.rs, then the authored cases inconformance/utils/src/gen_unified_golden.py. Read the generated archives last, by count and hash rather than line by line./coderabbit profile chill
Summary by CodeRabbit
New Features
Bug Fixes
Documentation