Repository navigation
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
There was a problem hiding this comment.
Review on behalf of the maintainers (rule 17 checklist, rules 0, 19 to 24 and 26), at head ff44c4b: one commit, feat(symphony): the constrained format for a forced or required tool choice, stacked on #2799's 36bc7b1.
Scope, commit, gates. formats/constrained.rs (new), formats/mod.rs (the module and the shared finish_reason), formats/qwen3.rs (its private match replaced by the shared function, its tests importing FinishReason themselves), lib.rs (one doc line), tests/contract.rs (every property and the lifecycle check over FORMATS, each with its own corpus); Cargo.lock unchanged; event.rs, input.rs and parser.rs untouched. Conventional subject, the author's Signed-off-by, Co-authored-by: Chang Su, no AI trailer. PR body per the template. Gates run here on ff44c4b in a clean worktree, exit codes captured: cargo +nightly fmt --all -- --check 0; cargo +1.98.0 clippy -p smg-symphony --all-targets -- -D warnings 0; cargo +1.98.0 test -p smg-symphony 0 (115 unit tests, six contract tests over the three formats); RUSTDOCFLAGS=-D warnings cargo +1.98.0 doc -p smg-symphony --no-deps 0; codespell 2.4.1 over crates/symphony 0.
Reading. Single accounts for the output's bytes in one place: a fragment while the prefix parser takes them whole, whole_end once the parsed prefix is a complete value (is_complete on it), InvalidArguments from the first byte the parser leaves, and after a whole value the whitespace dropped and the first byte of substance making the rest malformed for good (junk), so no chunking can move the boundary; the start comes at the value's first byte with an empty source and the end at the output's end whether or not the value is whole, and a blank output makes no call. List is a four-phase walk (before, between, object, after, plus broken) that drops the brackets and commas as wrapper and the JSON whitespace as whitespace, hands each { to an Assembler indexed by the calls that started, takes the next free index only when the assembler started a call, and turns anything out of shape into Malformed from that byte to the end; the loop always advances, since the assembler takes everything before its close. Both emit in byte order and every event goes through the ledger, so token identity holds here as it does for Qwen3. The reference behaviour (parse_json_schema_response: the whole output as the forced function's arguments; a list of {name, parameters} for required) is what the oracle below encodes.
Harness. A scratch program outside the repository: 40 outputs (23 for a forced function, 17 for required), each replayed whole, at every two-way cut, character by character and at ten seeded plans: 1539 replays. Checked on every replay: every byte in exactly one event in order with Finish last; no content or reasoning event; starts before fragments before ends, indices from zero in order with ids following, one end per start, tool_calls equal to the starts; and that the fragments, the dropped and malformed text by reason, the calls and the ends do not depend on the cuts. Oracles: for a forced function, any output that is a whole JSON value with whitespace around it gives one call named by the request whose fragments are exactly the value and nothing malformed (objects with nested escapes, Japanese and emoji, the surrogate-pair escape, an array, a string, a number, true, {}); a blank output no call and tool_calls: 0; for required, any output that is a whole list gives the calls serde_json reads from it, by name and the parameters or arguments bytes, in order, a nameless object skipped and the ids running call_0, call_1 across it. The malformed outputs behave as the doc says: a value cut short or cut inside a string ends with what arrived; junk after a whole value is Other("bytes after the arguments") with the whitespace before it dropped and the whitespace inside it kept malformed; a second value after the first is junk; 12abc is the whole 12 and junk abc; hello world is InvalidArguments from its first byte; not a list, junk where a comma should be, bytes after the list, a list cut short before or inside an object, [{"na as the assembler's UnterminatedRegion, all from the first offending byte to the end. 0 failures.
One observation, no change asked. The one output whose events depend on the cuts is truex: fed whole it is InvalidArguments from the first byte, fed true then x it is the fragment true and InvalidArguments for x, and every cut in between moves the boundary (12 of the 1539 replays). That is the prefix parser's literal leniency the #2792 review observed and the stricter acceptor listed in STATE.md would end; it is why the contract corpora avoid junk that begins like a literal, and here it is reachable only when the grammar is violated. A related asymmetry worth a sentence somewhere: the same bytes classify differently by position, 12abc as a whole value plus junk here (Other) and as InvalidArguments inside an assembler's object, each as its module documents.
Approved. Rule 25 at merge time: stacked on #2799, so it retargets as that lands and merges with every check green on the head, the squash body carrying the sign-off and the Chang Su trailer.
36bc7b1 to
80ea22b
Compare
ff44c4b to
e74a6eb
Compare
|
Converted to draft. Ground rule 26 says a format PR is opened only after bellwether's fixtures for that format are merged, and none exist for outputs constrained by |
80ea22b to
ee8456a
Compare
e74a6eb to
dfa837b
Compare
ee8456a to
954a5f6
Compare
b4d5e9d to
85198b2
Compare
14b1952 to
3f80740
Compare
85198b2 to
f6c08f7
Compare
3f80740 to
5ba92b3
Compare
f6c08f7 to
94acfd0
Compare
|
The design question behind this draft is answered. The maintainer, 2026-10-06, on bellwether #21: "makes sense, this should be part of symphony". What that settles: generating the This stays a draft until its fixtures are on bellwether main (#19). They can be built from the BFCL import and need no model, since the request decides the shape. |
5ba92b3 to
f8d4a03
Compare
94acfd0 to
1d205f9
Compare
`tokens::Ledger` turns the deltas' token spans into counts on every text
event. Because a format emits the output's bytes in order, each byte in
exactly one event, the ledger needs no map from bytes to events: it keeps
the absolute offsets at which tokens start and counts, for each run of
text emitted, the tokens that start inside it. A token is counted in the
event that carries its first byte; a span continued from an earlier
delta is not a new token; a token without bytes is a control token,
reported as `Dropped { ControlToken }` with a count of one where it
arrived. Counting is all or nothing per stream: a delta without spans,
or with spans that do not partition its text, leaves the rest uncounted.
The Qwen3 format counts every text it emits, relabels the assembler's
events in order, and reports the reasoning tokens in `Finish`. The
contract tests gain the fifth property: with a synthetic tokenization,
every token is counted in exactly one event whatever the cuts, and the
finish counts the reasoning tokens.
Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The span contract names two tokens without bytes, a held half of a
character completed by a later token and a hidden special token, and
the ledger treated both as control tokens, reported at once and outside
any region. A reasoning block with an emoji then counted its held halves
as control tokens rather than as reasoning. A byte-less span is now
queued like any other token and counted into the run that carries the
next byte, the character it began; the tokens still queued when the
output ends had no bytes to follow them and are reported once, at the
end, as `Dropped { ControlToken }` with their count. A stream that turned
uncounted reports zero reasoning tokens, not a partial sum. Tests: the
emoji probe (five reasoning tokens, no control token), a trailing
byte-less token reported once, a hidden special token mid-stream, and the
contract's synthetic tokenization now ends with a byte-less token.
Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
… token is counted The evidence ground rule 1 asks of token attribution, now that bellwether records `output_pieces` (95f079b). The fixture test gains qwen3_token_plans_count_every_token_where_its_first_byte_lands: every token chunk plan a fixture records, whole and per token included, is fed as the engines feed it, each delta carrying its tokens' ids and the span of each token's piece (an empty piece for a token whose bytes the decoder still holds). For every plan it checks conservation, that every text event is counted, that each event's count is the number of tokens whose first byte it carries, the total, the reasoning count against an independent count of the token starts between <think> and </think>, and that the message equals the text replay's. Against bellwether 95f079b: 16 cases, 785 token plans, 28183 tokens counted, 3127 of them reasoning. Answering the review threads the Claude review bot opened on #2799: the contract's token check summed counts, which conservation all but guaranteed, and now checks each event's count against the synthetic token starts in its bytes; the ledger's docs say a text token whose held bytes were never released (a stripped stop string, an output cut inside a character) is reported with the control tokens, which the spans cannot tell apart; and Qwen3's doc says the rest of a stream turns uncounted once a delta lacks spans or its spans do not partition its text. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com> Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…ng token The per-event check skipped a text event without a count the way it skips the region markers and the finish, and never compared the total with the tokens, so a format that never counts, or one that forgets to report the tokens left at the end, passed it. It now skips only `ReasoningStart`, `ReasoningEnd` and `Finish`, fails on any other event without a count, checks that the counts add up to the tokens, and reads the finish's reasoning count the way the fixture test does. Two mutations of `Qwen3` that the old check passed fail this one: the ledger's finish removed, and the ledger removed altogether. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com> Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Three assertion and notice strings are split with a line continuation, which leaves each message as it was. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com> Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…choice When a request forces a function or requires a call, the gateway constrains the engine with a JSON schema and the output has no markers: the whole output is the forced function's arguments, or a list of call objects. `formats::Constrained` is the Parser for both shapes, known from the request rather than the model. One function: whitespace before the value is dropped, the call starts at the value's first byte with the request's name and an empty source, fragments are the model's bytes while they keep a valid JSON prefix and InvalidArguments after, trailing bytes after a whole value are whitespace or Malformed, and the call ends with the output. Required: brackets and commas are Wrapper, whitespace between them Whitespace, each object goes to the assembler, and anything outside the shape is Malformed from its first byte. Every byte in one event, tokens counted, ids call_<index>. The finish-reason mapping moves to formats::finish_reason, shared with Qwen3. The contract tests run every property over every format with its own corpus; the two new corpora found one chunking variance in the trailing-junk handling, fixed here. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com> Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The module doc's summary line says the same in fewer words, four paragraphs are rewrapped without a change in wording, and the two-call list in the contract corpus is split with a line continuation, which leaves the string's value as it was. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com> Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…eway constrains a list
The list parser took a comma as optional anywhere between objects, so
`[{...}{...}]`, `[,{...}]`, `[{...},]` and `[,,]` went through with
every comma dropped, while the module doc promised `Malformed` for bytes
where a call, a comma or the list's end should be. The one phase between
objects is now three, one per position: after the opening bracket a call
or the end, after a call a comma or the end, after a comma a call; each
reason names what was expected there. A new test holds the four outputs,
and failed before the change.
The docs said an allowed-tools list is constrained to a list of calls;
the gateway does that only in `required` mode, and not for a model it
constrains through a structural tag, which keeps its own markers. Both
are stated where the driver will read them.
Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
f8d4a03 to
277b7f4
Compare
1d205f9 to
8c9a3ac
Compare
…matches and closed where its trailer ends
Problem: a response template writes a call's region as patterns. Its open is a marker and what follows it, which may name the tool (`<\|channel\|>commentary to=functions\.(?P<name>\w+).*?<\|message\|>`); its close is what comes before a marker (`</function>\s*</tool_call>`). Every call syntax here is a family's assembler, so a template whose spelling no family writes cannot be read without writing one more. A pattern can also fail where its marker stands: transformers then reads the marker as text and searches on from the character after its first one, which a stream that opened a call at the marker could not take back.
Solution: `CallSyntax::Shaped(&'static shape::CallShape)`, a call given as data: the header after the region's terminal, which may capture the name; the arguments as one JSON value or as an object that names the call (`shape::Arguments::{Json, Envelope}`); the trailer before the closing terminal; the class `\b` reads; and the most a header or trailer holds undecided. `CallShape::check` refuses by name what a stream cannot run, and `shape::intern` keeps a shape built at run time `'static`, so the syntax stays `Copy`.
The engine reads a shaped call's header before it takes the row, and emits nothing meanwhile; when the terminal is the turn's first piece, the row leaves the state the turn starts in, which is taken only once the header matches. A header that matches opens the call, and the bytes after it are read again inside the call. A header that cannot match gives the terminal back: its first character is the text of the state it came in, and the rest, with every byte the header read, is scanned again from there. A closing terminal closes the call only where the trailer ends right before it, and so does the stream's stop; `shape::Suffix` finds the trailer's earliest start, as a search does. Any other closing terminal is the call's text.
`json::Arguments` reads arguments written as one value, streaming the model's bytes while they are a JSON prefix and holding a word until it is decided, so the cuts never change what is sent. It is the reader smg-project#2806's forced function needs, with the call's start left to the caller. `Format::validate` refuses a shaped call a turn could start in, an ignored terminal beside one, and a row into it from a call state when its header may not match. A shaped call has no structural tag yet. The matcher's machine is now shared with the search, and the matcher answers as before.
Validation: unit tests on abstract tags cover a header given back in content, in a thought, in framing and at a turn's start; a call whose row leaves only the state a turn starts in; headers decided at the end; trailers and stops; envelopes; token counts; and no structural tag for a shaped call. The Qwen3 table with its calls as an envelope gives the JSON syntax's events at every cut. The gpt-oss test template of transformers v5.18.0 now spells its calls and reads them as `parse_response` does (five new recorded cases, every cut). The contract's five properties run for three shaped tables. The search agrees with Python `regex` 2026.9.10 on 277 texts and on every prefix of the written ones (`tests/fixtures/shape/search.jsonl`; `generate_shape_fixtures.py --check` passes and the earlier fixtures are unchanged). The 23 registry tables give byte-identical events in 481,712 runs. cargo test -p smg-symphony --all-features; cargo +nightly fmt --all -- --check; cargo clippy -p smg-symphony --all-targets --all-features -- -D warnings; RUSTDOCFLAGS="-D warnings" cargo doc --no-deps -p smg-symphony.
Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Signed-off-by: yechank <161688079+yechank-nvidia@users.noreply.github.com>
Description
Problem
When a request forces a function (
tool_choice: {"type": "function", ...}) or requires a call (tool_choice: "required", or an allowed-tools list inrequiredmode), the gateway constrains the engine with a JSON schema, unless the model's tool parser has a structural tag, in which case the output keeps the model's own markers, and the output has no markers at all: the whole output is the forced function's arguments, or a JSON array of{"name", "parameters"}objects. Today no parser handles that shape. The gateway parses the whole output once it has ended (parse_json_schema_responseinutils/chat_utils.rs) and, when streaming a forced function, sends the whole output as one call's arguments without looking at it (regular/streaming.rs). Symphony needs aParserfor this shape before the driver can switch, and it is the one shape where the format is known from the request rather than from the model.Solution
formats::Constrained, aParserbuilt fromChoice::Function(name)orChoice::Required. It needs no scanner, since it knows the shape, and it streams the arguments as the model's own bytes, as every format does.One function: whitespace before the value is
Dropped { Whitespace }; the call starts at the value's first byte, named by the request, itssourceempty since no output byte names it; the argument fragments are the output's bytes for as long as they keep the arguments a valid JSON prefix (checked with the ported prefix parser, exactly as the assembler does), and from the first byte that does not they come back asMalformed { InvalidArguments }. Once the value is whole, the whitespace after it is dropped and everything from the first other byte isMalformed { Other }. The call ends when the output ends, whole or not, so a value cut short is a call with what arrived.Required: the list's syntax is checked as JSON has it (a call or the end after the opening bracket, a comma or the end after a call, a call after a comma); brackets and commas in their places are
Dropped { Wrapper }, whitespace between themDropped { Whitespace }, and each object goes to thejson::Assembler, which emits the call's events and says where the object ended; the assembler already acceptsparametersas the arguments key. An output that is not a list, bytes the syntax does not allow where they stand (a missing comma, a leading or trailing one, two in a row), and bytes after the list areMalformed { Other }from the first such byte to the end, with a reason that names what was expected there. A list cut short ends with what arrived.Every byte lands in one event, every text event carries its token count from the
Ledger, ids arecall_<index>, the engine's finish reason is kept withtool_callscounting the calls that started, and reasoning cannot occur. The finish-reason mapping both formats need moves toformats::finish_reason, andQwen3uses it.The contract tests now run over every format with its own corpus:
FORMATSlists a constructor and outputs per format, and the five properties plus the lifecycle check iterate over all of them. The two new corpora (twelve and fourteen outputs) hold the grammar's shapes and what a grammar never produces, since the parser must survive it anyway. The corpora avoid junk that begins like a JSON literal (nope,not json): whereInvalidArgumentsbegins for a literal's prefix depends on the chunking, as Alex noted on #2792, because the prefix parser is as lenient as the old crate's, so those inputs cannot be chunking-invariant for any format built on it.Stacked on #2799 (base branch
feat/symphony-token-attribution); it retargets when that merges.Changes
crates/symphony/src/formats/constrained.rs:Constrained,Choice, the module doc, ten tests.crates/symphony/src/formats/mod.rs: the module,finish_reason;crates/symphony/src/formats/qwen3.rs: uses it (the tests importFinishReasonthemselves now).crates/symphony/src/lib.rs: exportsChoiceandConstrained.crates/symphony/tests/contract.rs:Format,FORMATS,corpus(), the two corpora;replayandreplay_countedtake the format's constructor; every property and the lifecycle check run per format.Nothing outside
crates/symphony/changes;Cargo.lockis unchanged.Test Plan
Run on the head, exit codes checked:
cargo +nightly fmt -p smg-symphony: 0, no change.cargo +1.98.0 clippy -p smg-symphony --all-targets -- -D warnings: 0.cargo +1.98.0 test -p smg-symphony: 0, 126 unit tests, two fixture tests on their skip path and 7 contract tests. The new unit tests: a forced function takes the whole output as its arguments; whitespace around the value is dropped and the call starts at the value; a value cut short is a call with what arrived; bytes that break the prefix areInvalidArgumentsand bytes after a whole value areOtherafter the dropped space; an empty or blank output makes no call; a required list streams each call and drops the list syntax (event by event, with the object cut across two deltas); an output that is not a list, bytes after the list, and a list of non-objects are malformed from the first such byte; a comma comes between calls and nowhere else ([{…}{…}],[,{…}],[{…},]and[,,], each malformed from the byte the syntax refuses); a list cut short ends with what arrived, and a nameless object makes no call; the engine's finish reason is kept. The contract: conservation, chunking invariance, well-formedness, order and token identity hold for both constrained shapes under every chunking of their corpora, and the lifecycle check passes for all three formats. One defect found by the contract and fixed before this head: after a whole value, each delta re-trimmed its own leading whitespace, so a space inside trailing junk was dropped in one chunking and malformed in another; junk is sticky now.RUSTDOCFLAGS="-D warnings" cargo +1.98.0 doc -p smg-symphony --no-deps: 0.git diff --stat feat/symphony-token-attribution...HEAD: the five files above.The shapes were taken from
parse_json_schema_responseand the forced-function streaming branch in the gateway; the Symphony pieces fromjson::Assembler,json::PartialJson,json::is_completeandtokens::Ledger.Checklist
cargo +nightly fmtpassescargo clippy --all-targets -- -D warningspasses forsmg-symphony(the workspace-wide run with--all-featuresneeds OpenCV forllm-multimodalon this machine; CI runs it)