Repository navigation
feat(parsers): add GLM-4.7 structural tag builder for v1 parser - #290
Conversation
4c747ff to
8bc0191
Compare
8bc0191 to
fe1f606
Compare
rmccorm4
left a comment
There was a problem hiding this comment.
Two findings from local XGrammar and parser validation.
fe1f606 to
a9fbc49
Compare
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: The forced-call path still emits
glm_xmlatglm47.rs:43; no corresponding parser change preserves XML entity literals. The reported<to<argument mutation therefore remains.
Addressed in a9fbc49: glm47_parser.rs no longer decodes entities |
9372d42 to
e56eb62
Compare
e56eb62 to
7b3d796
Compare
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: The
glm_xmltag content still setsany_order: trueinparsers/v1/src/tool_calling/structural_tag/glm47.rs, so the previously reported weakening of schema enforcement for required and duplicate properties remains present. - Original discussion:
any_order: trueremains set unconditionally atglm47.rs:92, so GLM tool schemas can omit required properties and accept duplicate properties, defeating strict schema enforcement.
7b3d796 to
5984f03
Compare
rmccorm4
left a comment
There was a problem hiding this comment.
Both earlier findings are fixed. Two new schema-normalization issues reproduce on this revision.
5984f03 to
ee1bd5d
Compare
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: Still present:
glm_schemaoverwrites a referenced definition's same-key constraint with the sibling value (lines 62-66), instead of preserving both constraints. For example,$refmaximum 1000 plus sibling maximum 2000 becomes maximum 2000, weakening the requested schema. - Original discussion: The GLM builder still calls the generic
resolve_tool_schemaatparsers/v1/src/tool_calling/structural_tag/glm47.rs:80; with the generic policy, tools whosestrictfield is omitted are replaced withtrue, so ordinary GLM tool argument constraints are still dropped instead of being preserved unlessstrict: falseis explicit.
There was a problem hiding this comment.
Thank you for working on this, @qimcis . We have a few moving pieces that impact each other.
Let's iterate and merge this PR once ai-dynamo/dynamo#13654 lands 🙏
[P2: Low-risk, non-blocking issue such as a narrow edge case, defensive-hardening opportunity, or minor maintainability/test gap]
Rebase onto current main and add builder regression coverage
Assuming Dynamo #13654 lands first, please rebase onto current main, preserving the merged GLM reference, nullable-value, and scalar-union fixes. Frontend-crates #196 already updated the shared resolve_tool_schema: omitted/true tool strictness preserves the declared schema, explicit strict:false uses the permissive JSON Schema true, and global Strict overrides false. This PR's existing helper call can remain; a GLM-specific strictness helper is unnecessary.
The current tests cover schema normalization but do not exercise the builder's policy and control flow. Please add coverage for omitted/false strictness, global Strict, and auto/required/named calls with a prompt-opened reasoning block. These paths become more widely used once #13654 enables structural guidance by default.
Signed-off-by: Chi McIsaac <chi@primeintellect.ai>
Infer string coercion from const constraints, including local references and composed schemas, without overriding explicit type constraints. Preserve literal quotes and JSON-looking argument text on the native GLM forced-call path. Cover omitted, false, and true tool strictness with Auto and Strict schema modes; auto, required, and named tool choices; prompt-opened reasoning; and parallel-call settings. Add an opt-in XGrammar-to-public-parser round-trip regression validated with XGrammar 0.2.4 and 0.2.7. Signed-off-by: Chi McIsaac <chi@primeintellect.ai>
ee1bd5d to
8c86cb8
Compare
|
/ok to test 8c86cb8 |
|
📊 Conformance matrix rendered — view in CI summary |
Match string constants against raw GLM arguments before selecting a string hint in v1 and v2. Preserve literal quote and JSON-looking string values, and share percent-encoded local reference resolution with the v1 builder. Add independently authored schema controls across applicable parser families and delivery paths. Pin unpublished parser release versions before recording captures, preserving existing goldens and history. All 118 GLM parser controls pass, and 2,726 executed existing cases retain their exact PR-head outputs. Keep the 43 reproduced baseline batch-on-stream disagreements and real-decoder limits visible. Signed-off-by: furionw <qiwa@nvidia.com>
|
/ok to test f4f357c |
|
@qimcis Following the constant roundtrip discussion, I pushed f4f357c6 with three incremental behavior changes:
The remaining changes add independently authored schema coverage, fixtures/capture tooling, and release-version metadata. Existing golden values and historical captures are preserved. Verification against this pushed commit: all 118 authored controls pass for v1 and v2, including streaming/Unified paths. Seven focused real XGrammar 0.2.7 grammar-to-parser probes pass, with 20 invalid wires rejected. The full-corpus comparison found no output changes in 2,726 existing cases. The PR description records the remaining decoder limits and 43 reproduced baseline batch-on-stream disagreements; these are not hidden by new exceptions. |
Merge current main without rewriting the contributor branch and retain unpublished parser release pins: v1 10.0.1 and v2 0.7.20. Reject nonredundant GLM reference sibling constraints ignored by XGrammar, including nested and aliased schemas. Preserve annotations, redundant constraints, and literal default/example data. Check every live batch-on-stream result against its exact v2 capture before applying documented baseline parity differences. Record the pre-existing DS4 empty-name defect separately from intended differences and preserve every independently authored golden. Refresh v1 capture packaging and v2 source receipts without changing existing parser results or historical captures. Signed-off-by: furionw <qiwa@nvidia.com>
|
/ok to test a9c7589 |
|
@qimcis I pushed a9c7589 to address the remaining reference-sibling review, the required Rust gate failure, and the merge conflict with current This adds one functional correction beyond the three changes explained above: the v1 GLM builder returns an error when a The gate failure exposed 43 unchanged baseline differences. The gate now checks every live v2 result against its exact capture before applying an exception: 42 documented parity differences and one separately pinned existing DS4 empty-name defect. Independently authored goldens and every historical capture remain intact. Fresh v1 10.0.1 captures reproduce all 3,414 prior v1 results; v2 remains 0.7.20. The base update is a normal merge, preserving your branch history. Verification: 2,578 workspace Rust tests and 1,019 conformance Python tests passed, along with formatting, Clippy, dependency checks, and publish dry-run. Against this exact pushed commit, both XGrammar versions passed 42 native grammar-to-parser roundtrips and rejected six invalid wires; unsupported reference-sibling error paths and nested/alias guards passed. The uploaded capture archive was independently downloaded and its SHA/size verified. Exact-head CI is authorized. @furionw, the condition in your review is fulfilled: Dynamo #13654 merged first. The branch now includes current |
|
@dynamo-review-agent, the findings in these review bodies are addressed on a9c7589:
The description records remaining XGrammar limitations and unchanged baseline parser defects; they are not represented as newly supported schema combinations. |
Merge main at 3f7aae6 without rewriting the contributor history. Keep the GLM parser release pins and take protocols 8.1.0 to resolve the workspace dependency conflict. Signed-off-by: furionw <qiwa@nvidia.com>
|
/ok to test c5dc885 |
|
@qimcis I pushed c5dc885 as a normal merge of live GLM parser/grammar source and conformance captures are unchanged from |
Allow 40 minutes for the Rust job after its 30-minute limit cancelled the package verification step. Workspace tests, protocol schema tests, doctests, and Clippy had already passed. Keep every required check. Signed-off-by: furionw <qiwa@nvidia.com>
|
/ok to test e642bb5 |
|
@qimcis I pushed e642bb5 to address the required Rust job's timeout. Its annotation confirms the 30-minute execution limit: all workspace tests, doctests, protocol-schema tests, and Clippy passed, then package verification was cancelled while compiling. The previous green run finished in 26 minutes; this run spent 25 minutes on passing tests alone. This changes one workflow line: the Rust job gets 40 minutes. Every check, action, and permission remains identical; parser source and captures are unchanged. YAML parsing and an exact structural comparison verified that scope. Against the exact pushed commit, real XGrammar 0.2.4 and 0.2.7 each passed 42 grammar-to-parser roundtrips and six invalid-wire rejections. Fresh exact-head CI will verify the complete workflow finishes with the new limit. |
Why
Dynamo’s v1 GLM parser lacks native structural tags for schema-constrained tool calls and named tool choice. Its unquoted argument text needs value-aware typing: raw
42underanyOf: [{const:"auto"}, {type:"integer"}]must remain an integer while matching string constants preserve their literal spelling. XGrammar ignores nonredundant$refsibling constraints, so the builder must detect unsupported schemas before emitting a misleading grammar. Independent schema oracles and exact-output gates expose existing parser differences without allowing new output corruption.What Change
Test Plan
Known limitations
Forty-three baseline parity differences are triaged: 42 documented differences and one separately pinned DS4 empty-name defect. Authored goldens stay intact, including 99 legacy schema disagreements (81 visible report comparisons, 18 duplicate Nano records). XGrammar diagnostics still expose unsupported
allOf, itemless-array, and object/reference combinations; v2’s pre-existing reference-sibling limitation remains. The v1 builder returns an error for unsupported reference siblings; callers retain their existing error/fallback policy.Conformance case inventory
Each variant below defines
TOOLCALLING.batch.7.<letter>.<variant>,TOOLCALLING.streamv1.7.<letter>.<variant>, andUNIFIED.7-<number>.<variant>. Expectations are authored independently of captures, with family-native wire formats.7.o7-17declared_string,declared_integer,declared_number,declared_boolean,declared_object,declared_array,declared_null,id_direct_integer7.p7-18const_plain_bare,const_plain_typed,const_plain_allof,const_plain_ref,const_quoted_bare,const_quoted_typed,const_quoted_allof,const_quoted_ref,const_object_bare,const_object_typed,const_object_allof,const_object_ref,const_array_bare,const_array_typed,const_array_allof,const_array_ref,const_number_bare,const_number_typed,const_number_allof,const_number_ref,const_boolean_bare,const_boolean_typed,const_boolean_allof,const_boolean_ref,const_null_typed,const_null_allof,const_null_ref,const_space_bare,const_space_typed,const_space_allof,const_space_ref,const_unicode_bare,const_unicode_typed,const_unicode_allof,const_unicode_ref,const_entity_bare,const_entity_typed,const_entity_allof,const_entity_ref,const_null_bare7.q7-19union_anyOf_integer_string,union_anyOf_integer_typed,union_anyOf_number_string,union_anyOf_number_typed,union_anyOf_boolean_string,union_anyOf_boolean_typed,union_anyOf_object_string,union_anyOf_object_typed,union_anyOf_array_string,union_anyOf_array_typed,union_anyOf_null_string,union_anyOf_null_typed,union_anyOf_integer_reverse_string,union_anyOf_integer_reverse_typed,union_anyOf_number_reverse_string,union_anyOf_number_reverse_typed,union_anyOf_boolean_reverse_string,union_anyOf_boolean_reverse_typed,union_anyOf_object_reverse_string,union_anyOf_object_reverse_typed,union_anyOf_array_reverse_string,union_anyOf_array_reverse_typed,union_anyOf_null_reverse_string,union_anyOf_null_reverse_typed7.r7-20union_oneOf_integer_string,union_oneOf_integer_typed,union_oneOf_number_string,union_oneOf_number_typed,union_oneOf_boolean_string,union_oneOf_boolean_typed,union_oneOf_object_string,union_oneOf_object_typed,union_oneOf_array_string,union_oneOf_array_typed,union_oneOf_null_string,union_oneOf_null_typed,union_oneOf_integer_reverse_string,union_oneOf_integer_reverse_typed,union_oneOf_number_reverse_string,union_oneOf_number_reverse_typed,union_oneOf_boolean_reverse_string,union_oneOf_boolean_reverse_typed,union_oneOf_object_reverse_string,union_oneOf_object_reverse_typed,union_oneOf_array_reverse_string,union_oneOf_array_reverse_typed,union_oneOf_null_reverse_string,union_oneOf_null_reverse_typed7.s7-21ref_union_anyOf_string,ref_union_anyOf_integer,ref_union_oneOf_string,ref_union_oneOf_integer,ref_sibling_integer,escaped_ref_const,percent_ref_const7.t7-22intersection_anyOf,intersection_oneOf7.u7-23enum_string,enum_integer7.v7-24type_array_string_integer,type_array_integer_null,type_array_integer_null_null,nullable_string7.w7-25null_union_ambiguous,integer_const_ambiguous,object_const_unionEvery group applies to all eight Unified families: deepseek_v4, deepseek_v41, gemma4, glm47, kimi_k2, kimi_k3, muse_glimmer, qwen3.
Legacy v1 batch covers deepseek_v3, deepseek_v3_1, deepseek_v3_2, deepseek_v4, gemma4, glm47, harmony, hermes, inkling, jamba, kimi_k2, kimi_k3, llama3_json, minimax_m2, minimax_m3, mistral, nemotron_deci, nemotron_nano, phi4, pythonic, qwen25, qwen3_coder. Harmony text shares its batch grammar; Muse has no v1 parser.
Legacy v2 streaming covers deepseek_v4, gemma4, glm47, harmony_text, kimi_k2, kimi_k3, minimax_m2, minimax_m3, muse_glimmer, qwen3_coder. Batch-on-stream covers the same list except harmony_text, whose token alias requires real token IDs. Other registered families carry explicit v2 unavailability.
Unresolved/cyclic references and the deliberately invalid typed-enum control remain Rust-only controls. Duplicate schema/null-string spelling is represented once. Ambiguous native text pins existing family interpretations; repository nullable behavior is tested separately from standard schema validation. Historical captures and existing golden values remain intact.
GLM v1/v2 argument results for every new conformance case
The case suffix expands to the batch and streaming IDs above; its Unified counterpart uses the mapped numeric group. Values below are JSON, preserving their types. “Same” equals the final value. Before is refreshed author head
8c86cb8; after includes the value-aware constant fix and the reference-builder guard. Other parser families retain their baseline interpretations; their independent goldens remain unchanged.7.o.declared_string"plain"7.o.declared_integer427.o.declared_number3.57.o.declared_booleantrue7.o.declared_object{"x":1}7.o.declared_array[1,2]7.o.declared_nullnull7.p.const_plain_bare"auto"7.p.const_plain_typed"auto"7.p.const_plain_allof"auto"7.p.const_plain_ref"auto"7.p.const_quoted_bare"hello""\"hello\""7.p.const_quoted_typed"\"hello\""7.p.const_quoted_allof"hello""\"hello\""7.p.const_quoted_ref"hello""\"hello\""7.p.const_object_bare{"x":1}"{\"x\":1}"7.p.const_object_typed"{\"x\":1}"7.p.const_object_allof{"x":1}"{\"x\":1}"7.p.const_object_ref{"x":1}"{\"x\":1}"7.p.const_array_bare[1,2]"[1,2]"7.p.const_array_typed"[1,2]"7.p.const_array_allof[1,2]"[1,2]"7.p.const_array_ref[1,2]"[1,2]"7.p.const_number_bare"42"7.p.const_number_typed"42"7.p.const_number_allof"42"7.p.const_number_ref"42"7.p.const_boolean_bare"true"7.p.const_boolean_typed"true"7.p.const_boolean_allof"true"7.p.const_boolean_ref"true"7.p.const_null_typed"null"7.p.const_null_allof"null"7.p.const_null_ref"null"7.p.const_space_bare" spaced "7.p.const_space_typed" spaced "7.p.const_space_allof" spaced "7.p.const_space_ref" spaced "7.p.const_unicode_bare"café"7.p.const_unicode_typed"café"7.p.const_unicode_allof"café"7.p.const_unicode_ref"café"7.p.const_entity_bare"<"7.p.const_entity_typed"<"7.p.const_entity_allof"<"7.p.const_entity_ref"<"7.q.union_anyOf_integer_string"auto"7.q.union_anyOf_integer_typed"42"427.q.union_anyOf_number_string"auto"7.q.union_anyOf_number_typed"3.5"3.57.q.union_anyOf_boolean_string"auto"7.q.union_anyOf_boolean_typed"true"true7.q.union_anyOf_object_string"auto"7.q.union_anyOf_object_typed"{\"x\":1}"{"x":1}7.q.union_anyOf_array_string"auto"7.q.union_anyOf_array_typed"[1,2]"[1,2]7.q.union_anyOf_null_string"auto"7.q.union_anyOf_null_typednull7.q.union_anyOf_integer_reverse_string"auto"7.q.union_anyOf_integer_reverse_typed"42"427.q.union_anyOf_number_reverse_string"auto"7.q.union_anyOf_number_reverse_typed"3.5"3.57.q.union_anyOf_boolean_reverse_string"auto"7.q.union_anyOf_boolean_reverse_typed"true"true7.q.union_anyOf_object_reverse_string"auto"7.q.union_anyOf_object_reverse_typed"{\"x\":1}"{"x":1}7.q.union_anyOf_array_reverse_string"auto"7.q.union_anyOf_array_reverse_typed"[1,2]"[1,2]7.q.union_anyOf_null_reverse_string"auto"7.q.union_anyOf_null_reverse_typednull7.r.union_oneOf_integer_string"auto"7.r.union_oneOf_integer_typed"42"427.r.union_oneOf_number_string"auto"7.r.union_oneOf_number_typed"3.5"3.57.r.union_oneOf_boolean_string"auto"7.r.union_oneOf_boolean_typed"true"true7.r.union_oneOf_object_string"auto"7.r.union_oneOf_object_typed"{\"x\":1}"{"x":1}7.r.union_oneOf_array_string"auto"7.r.union_oneOf_array_typed"[1,2]"[1,2]7.r.union_oneOf_null_string"auto"7.r.union_oneOf_null_typednull7.r.union_oneOf_integer_reverse_string"auto"7.r.union_oneOf_integer_reverse_typed"42"427.r.union_oneOf_number_reverse_string"auto"7.r.union_oneOf_number_reverse_typed"3.5"3.57.r.union_oneOf_boolean_reverse_string"auto"7.r.union_oneOf_boolean_reverse_typed"true"true7.r.union_oneOf_object_reverse_string"auto"7.r.union_oneOf_object_reverse_typed"{\"x\":1}"{"x":1}7.r.union_oneOf_array_reverse_string"auto"7.r.union_oneOf_array_reverse_typed"[1,2]"[1,2]7.r.union_oneOf_null_reverse_string"auto"7.r.union_oneOf_null_reverse_typednull7.s.ref_union_anyOf_string"auto"7.s.ref_union_anyOf_integer"-7"-77.t.intersection_anyOf427.s.ref_union_oneOf_string"auto"7.s.ref_union_oneOf_integer"-7"-77.t.intersection_oneOf427.s.ref_sibling_integer427.s.escaped_ref_const"hello""\"hello\""7.s.percent_ref_const"42"7.u.enum_string"42"7.u.enum_integer427.v.type_array_string_integer"42"7.v.type_array_integer_null427.v.type_array_integer_null_nullnull7.v.nullable_stringnull7.p.const_null_bare"null"7.w.null_union_ambiguousnull7.w.integer_const_ambiguous42"42"7.w.object_const_union{"x":1}"{\"x\":1}"7.o.id_direct_integer42All parser-positive cases retain their expected arguments. For
ref_sibling_integer, v1 now explicitly tests a builder error because the decoder ignores its narrowing sibling type; parsing remains integer42. Unresolved/cyclic typed-reference controls also assert builder errors. Annotation-only and exactly redundant sibling constraints retain native grammar roundtrip coverage.