Repository navigation
fix(zmq): enforce wire constraints on TokenSpeed direct-backend workers - #2079
Conversation
TokenSpeed defaults its grammar backend to none and drops structured-output
constraints without complaint. The gateway translates all four constraint
kinds onto the wire correctly, so the request arrives intact and is then
ignored: `tool_choice=required`, `tool_choice={function}` and
`response_format=json_schema` come back as freeform text with no error.
Output logprobs default off the same way, so `logprobs=true` returns null.
The gRPC servicer path already passes both flags. The `smg serve` launcher
for the ZMQ path never did, which makes this a production hole and not a
test-harness one — the e2e lane just happens to be where it surfaced (44
failures on the TokenSpeed ZMQ lane, every one a grammar test).
Supply both as defaults rather than launcher-owned flags: an operator who
names a grammar backend keeps theirs. Failing loud instead of defaulting
would be better, but the engine does not report its grammar backend in the
handshake, so the frontend cannot tell an unenforceable constraint from an
enforceable one.
Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughTokenSpeed workers now receive default ChangesTokenSpeed backend defaults
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
There was a problem hiding this comment.
Clean, well-motivated fix. The _backend_arg_defaults helper correctly separates overridable defaults from launcher-owned flags stripped by _filter_backend_args. The three test cases cover the key scenarios (defaults applied, operator override wins, equals-form honored). No issues found.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bindings/python/tests/test_serve.py (1)
873-881: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win🟡 Nit Use a non-default value for the override test.
After the default changes to
llguidance, passnonehere. This verifies that a different operator value is preserved and that the default is not duplicated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bindings/python/tests/test_serve.py` around lines 873 - 881, Update test_build_zmq_command_defaults_yield_to_the_operator to pass the non-default grammar backend value "none" in the build_command arguments, then assert that "none" is preserved while confirming --grammar-backend appears only once and the other default remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@bindings/python/tests/test_serve.py`:
- Around line 873-881: Update
test_build_zmq_command_defaults_yield_to_the_operator to pass the non-default
grammar backend value "none" in the build_command arguments, then assert that
"none" is preserved while confirming --grammar-backend appears only once and the
other default remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bb5576de-30f1-4021-8103-0652c25e6c7e
📒 Files selected for processing (2)
bindings/python/src/smg/serve.pybindings/python/tests/test_serve.py
Problem
TokenSpeed defaults
grammar_backendtononeand then drops structured-output constraints without complaint. The gateway does its part correctly —apply_tokenspeed_constraintmaps all four constraint kinds onto the wire — so the request arrives intact at an engine that quietly ignores it.The user-visible result is that
tool_choice=required,tool_choice={function}andresponse_format=json_schemareturn freeform text with a 200. Output logprobs default off the same way, sologprobs=truereturns null.This is a production hole, not a test-harness one. The gRPC servicer path passes
--grammar-backend xgrammarand--enable-output-logprobsexplicitly, with a comment naming this exact failure mode; thesmg serveZMQ launcher never did. Anyone running TokenSpeed over ZMQ today gets silently unenforced constraints.The e2e lane is just where it surfaced: on run 31243936303 the
e2e-1gpu-chat-zmq (tokenspeed)lane loggedgrammar_backend='none'at startup and produced 44 failures — every one a grammar test (38 function-callingrequired/specific, 4json_schema, 2regex).tool_choice=autopassed throughout, so parsing was never the problem.Change
TokenspeedWorkerLaunchernow supplies both flags as defaults rather than launcher-owned flags, via a newWorkerLauncher._backend_arg_defaultshelper. The distinction matters:_filter_backend_argsstrips flags SMG owns outright, whereas these are engine defaults SMG needs for the wire to behave as the API promises but an operator may legitimately override. Pass--grammar-backend llguidanceand you keep yours; the other defaults stay.The e2e harness delegates to this launcher (
_build_tokenspeed_zmq_cmd), so the ZMQ lane picks the fix up with no harness change.What this does not fix
Failing loud would be better than defaulting. The frontend cannot do that today:
EngineCoreReadyResponsecarries no grammar-backend field, so it cannot distinguish an unenforceable constraint from an enforceable one. That needs a handshake addition on the TokenSpeed side (#48 territory), and is worth doing — a silently-ignored constraint is a correctness bug that looks like a model quality problem.Test Plan
pytest tests/test_serve.py -k Tokenspeed→ 11 passed, including three new cases: defaults are applied, an operator override wins and leaves other defaults intact, and the--flag=valueform is honored.ruff check/ruff format --checkclean.e2e-1gpu-chat-zmq (tokenspeed)lane on this PR: those 44 failures should clear.Checklist
cargo +nightly fmtpasses (no Rust changes)cargo clippy --all-targets --all-features -- -D warningspasses (no Rust changes)