Repository navigation
feat(symphony): keyed arguments written from the table's tags, for Ling's newlines and Hy4's suffixed tags - #2882
Conversation
|
Warning Review limit reachedYour organization has reached its usage spending cap. Adjust your spending cap in the billing tab. Next included review available in 14 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (4)
Comment |
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
hello-alexmcc
left a comment
hello-alexmcc
left a comment
There was a problem hiding this comment.
Rule-17 review of #2882 at a4ce888: comment. I'll approve once the three bot threads are answered; the data below settles the first two. With no old builder to check against, I checked the derived grammars against what each model's own template writes.
Template oracle. I used the parse sets recorded locally from each model's template at its pinned revision: inclusionAI/Ling-3.0-flash, and tencent/Hy4-preview at 705d81ee. The cases with tool calls, up to 1,500 per set, came from BFCL's seven sets, glaive-v2's first six, SWE-bench's call sets and the shapes call sets: 13,628 per model. For each case I derived the grammar from its request's tools with this PR. A throwaway matcher for the structural-tag JSON then checked each rendered output against it, using jsonschema for the json_schema leaves.
A case counts as one the grammar must accept when every call:
- names a tool of the request,
- uses only the schema's keys, in the schema's order, with every required one,
- has each value valid against its property's schema,
- and has no string value containing a close.
Results:
- All 26,758 such cases are accepted (13,379 per model). That includes 259 zero-argument calls per model, BFCL's parallel calls, and SWE-bench's long code strings.
- Each case was paired with the grammar its prompt calls for. Hy4 has 12,784 whose output starts inside the thought, matched with the reasoning prefix, and 595 that open their own thought, matched without it. All 13,379 of Ling's start inside the thought.
- Calls that break their schema are rejected (extra keys, a missing required key, keys out of order, values outside their schema), as a strict grammar should. The exception is 4 per model on BFCL's
is_unisex. Its schema is{"type": "boolean", "enum": ["True", "False", "dontcare"]}, and the stringenumwins as in the old Kimi K3 builder.
On the threads:
- A Ling call without arguments (keyed.rs:107): the template writes
<tool_call>version_api_VersionApi_get_version\n\n</tool_call>, two newlines, in every one of the 259. The grammar's\n\nis right.formats/ling.rs's single-newline sample is not what the template writes. - Hy4 with the reasoning open (keyed.rs:296): the 12,784 outputs above match the prefix. It's
any_textexcluding all ten:opensourcetags and ends at</think:opensource>, followed by the block's single way in from content, which has no close. A test pinning that, like Ling's, is still worth having. argumentsshared withxtml(keyed.rs:113): your call. Both copies follow the schema's order and the required keys the same way today.
Gates and earlier probes, at a4ce888 merged on main 21d5e44, with Rust 1.98.0, its own target dir and -j 2:
- fmt, clippy and doc each exit 0.
cargo test -p smg-symphony: 332 lib, 10 fixture, 7 contract and 5 README tests.- GLM and DSML vs the old builders: 36 of 36 equal. Kimi K3 is unchanged, equal in all 56 payloads but for
"type": "tag". - Every table's grammar vs #2880's: only Ling's and Hy4's 12 lines each change, from
Noneto a tag. No table goes from a tag toNone.
Rule 17 so far:
- Fixture ids: none changed.
- Protected paths: the crate's grammar module.
- Trailers: the author's
Signed-off-byandCo-authored-by: Chang Su, and no AI trailer.
hello-alexmcc
left a comment
hello-alexmcc
left a comment
There was a problem hiding this comment.
Rule-17 re-review of #2882 at ab7f762: approve. All three threads are answered and resolved, and my checks pass at this head.
The round, checked:
- A Ling call without arguments:
a_ling_call_without_arguments_writes_both_newlinespins<tool_call>ping\n\n</tool_call>, as the template writes it in all 259 recorded zero-argument calls. - Hy4 with the reasoning open:
hy4_with_the_reasoning_open_takes_the_prefix_and_its_one_way_inpins the prefix's end, its ten excludes, and the block's single trigger and way in. - The shared walk:
schema::argumentsis the walk the two syntaxes shared, moved as it was.xtmlpasses itsargumentfunction andkeyedits pair closure.
No output moves with the refactor. The template oracle's 27,256 per-case grammars (13,628 recorded tool-call cases per model) are byte-identical to the first round's, with the reasoning both open and closed. Paired with each prompt's reasoning state, all 26,758 schema-conforming cases are accepted:
- Hy4: 12,784 starting inside the thought, and 595 not.
- Ling: 13,379, all starting inside the thought.
Parity:
- Kimi K3 vs
KimiK3Parser::build_structural_tag: all 56 payloads are still equal except for"type": "tag". - GLM and DSML: 36 of 36 equal.
- Every table's grammar vs #2880's: only Ling's and Hy4's 12 lines each differ.
Gates. I ran them at ab7f762 merged on main 21d5e44, with Rust 1.98.0, its own target dir and -j 2; each exited 0:
cargo +nightly fmt -p smg-symphony --checkcargo clippy -p smg-symphony --all-targets --all-features -- -D warningscargo test -p smg-symphony: 334 lib, 10 fixture, 7 contract and 5 README testsRUSTDOCFLAGS="-D warnings" cargo doc -p smg-symphony --no-deps
Rule 17:
- Fixture ids: none changed.
- Property tests: no new format.
- Protected paths: the crate's grammar module. Nothing public changes, so rule 18 isn't touched.
- Trailers: the squash body carries the author's
Signed-off-byand fiveCo-authored-bylines, one each for Chang Su, Keyang Ru, key4ng, Chen Hongtao and the LightSeek Foundation account. As on #2881, that's for rule 27, since this moves the port of the old builder's argument walk. No AI trailer. - Threads: three, all answered and resolved.
The merge is the maintainer's (rule 25), after #2881.
b041332 to
39e22c5
Compare
ab7f762 to
fb3dffe
Compare
39e22c5 to
d173b5e
Compare
9e03a58 to
9acf6bf
Compare
hello-alexmcc
left a comment
hello-alexmcc
left a comment
There was a problem hiding this comment.
Rule-17 re-review of #2882 at 9acf6bf (rebased on #2881 ad49353): approve. The rebase changes nothing this PR derives. Its own diff passes, and all its threads are resolved.
- Template oracle: I matched the recorded Ling and Hy4 tool-call cases (13,628 per model) against this head's grammars, each case paired with its prompt's reasoning state. All 26,758 schema-conforming cases are accepted: Hy4 12,784 with the thought open and 595 without, Ling 13,379. Calls that break their schema are rejected, as before.
- Dump: only Ling's and Hy4's 12 lines each change against #2881.
- Parity: GLM and DSML 36 of 36. Kimi K3 is as at #2881.
- Gates: fmt, clippy and doc exit 0; tests 336 lib, 10 fixture, 7 contract and 5 README.
- Trailers: six in the squash body, and no AI trailer.
#2881's open threads (the unbounded pointer walk) are its own. This PR picks up their fix through its base.
…ng's newlines and Hy4's suffixed tags The engines' `glm_xml` style writes the compact spelling of keyed arguments, GLM 4.7's and IQuest's, and nothing else. Ling writes a newline after the call's name, after each `</arg_key>` and before `</tool_call>`, and Hy4 suffixes every tag `:opensource` and puts the calls in a block, so their tags are now written from the table's own tags: the call's opener and the tool's name, one `<arg_key>` and `<arg_value>` pair per property of the schema in its order, optional unless required, the newline where the spelling has it, and the call's close; the key is the property's name, the value follows the property's type as the templates write it (a string as it is, up to the value's close or a marker that ends the call; the rest as JSON, through the schema module); a property without a type, or a schema without properties, takes any text. Hy4's block takes the block's ways in, as Kimi K3's does, through a block builder the two now share. GLM and IQuest keep the `glm_xml` style, the shape the gateway writes today. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com> Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…two keyed cases are pinned The Kimi K3 and keyed modules walked a tool's schema the same way: the non-empty properties, the required names, the definitions, one argument per property in the schema's order, optional unless required, and a fallback for a schema without properties. The walk moves to the schema module, and each syntax gives it only how one property becomes a grammar and what the fallback is. Two tests pin what the review asked: a Ling call without arguments writes both of the template's newlines, as every one of the 259 such calls in its recorded sets does, and Hy4 with the reasoning open takes the prefix and then its block's one way in. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com> Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
9acf6bf to
9009f6a
Compare
hello-alexmcc
left a comment
hello-alexmcc
left a comment
There was a problem hiding this comment.
Rule-17 re-review of #2882 at 9009f6a (rebased on main 3c430bd): approve. It's the 9acf6bf I approved plus the rebase.
- Template oracle: all 26,758 schema-conforming recorded Ling and Hy4 cases are accepted, each paired with its prompt's reasoning state: Hy4 12,784 with the thought open and 595 without, Ling 13,379.
- Dump: only Ling's and Hy4's 12 lines each change against main.
- Parity: GLM and DSML 36 of 36. Kimi K3 is as on main.
- Gates: fmt, clippy and doc exit 0; tests 336 lib, 10 fixture, 7 contract and 5 README.
- Trailers: six in the squash body, and no AI trailer. Threads: all resolved.
2 participants
What
Stacked on #2881 (base
refactor/symphony-grammar-schema): the keyed spellings the engines'glm_xmlstyle does not write, insidecrates/symphony/only.grammar/keyed.rs: a call written from the table's ownTags: the call's opener and the tool's name, one<arg_key>/<arg_value>pair per property of the schema in the schema's order (optional unless required), and the call's close, with the spelling's newline (Tags::between) after the name, after each key's close and before the call's close, which is where Ling's template writes it; Hy4's tags carry their:opensourcesuffix and its calls sit in a block, framed by the block's ways in as Kimi K3's are. The key is the property's name; the value follows the property's type as the templates write it (a string as it is, any text up to the value's close or a marker that ends the call or the block;enumas its values; the rest as JSON through the schema module); a property without a type, or a schema without properties, takes any text.Format::grammar:Keyed(PLAIN)keeps theglm_xmlstyle (GLM, IQuest; parity with the gateway), every otherKeyedspelling takes the new path. Ling and Hy4 leave theNonelist; with the reasoning open they take the same prefix as GLM.block_of(ingrammar.rs): the block framing Kimi K3 built inline, now shared with Hy4.Spellings, from the templates
'<tool_call>' + name, newline, per argument'<arg_key>' + k + '</arg_key>','\n<arg_value>', the value (vif a string, elsetojson),'</arg_value>'; then'\n</tool_call>'<tool_call>get_weather\n<arg_key>city</arg_key>\n<arg_value>Paris</arg_value>\n</tool_call>toolcalls_begin_token, per calltoolcall_begin_token ~ name, per argumentargkey_begin ~ key ~ argkey_end,argvalue_begin ~ value ~ argvalue_end(tojsonunless a string),toolcall_end_token, thentoolcalls_end_token, every expression whitespace-stripped<tool_calls:opensource><tool_call:opensource>get_weather<arg_key:opensource>city</arg_key:opensource><arg_value:opensource>Paris</arg_value:opensource></tool_call:opensource></tool_calls:opensource>No old builder is an oracle for either: the gateway takes the JSON-schema path for Ling, and its Hy4 builder spells the tags without the suffix. The tests pin the derived payloads literally against the templates above.
Gates
By exit code (
.claude/symphony/gates.sh, baserefactor/symphony-grammar-schema): fmt, clippy-D warnings, test (334 lib, 10 fixture, 7 contract, 5 README), doc, width, codespell, all 0.Not in this PR
None.Merge
After #2881, rebased onto main. Squash under rule 25 (rule 18: nothing of
Format's changes; the maintainer merges). Subject: the PR title plus(#<n>). Body, with the authors of the old Kimi K3 parser whose property walk (build_args_format) moves intogrammar/schema.rshere and which the keyed syntaxes now build on (rule 27, as on #2881):