Skip to content

fix(sidecar): publish structural tags from the SGLang sidecar by default - #16494

Merged
alec-flowers merged 5 commits into
mainfrom
aflowers/sidecar-structural-tag
Oct 20, 2026
Merged

alec-flowers merged 5 commits into
mainfrom
aflowers/sidecar-structural-tag

Conversation

@alec-flowers

@alec-flowers alec-flowers commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The in-process dynamo.vllm, dynamo.sglang, and dynamo.trtllm workers publish
structural_tag = {scope: always, schema: auto} by default (--dyn-structural-tag,
#13654). The native Rust sidecars never set WorkerConfig.structural_tag, so they
registered with structural tags disabled.

With tags disabled, tool_choice=required or a named tool falls back to the legacy
guided-JSON grammar. Guided tool streaming (#12576, on by default) then releases
argument fragments as they arrive. When max_tokens truncates the output, the
client receives a tool call whose arguments are unterminated JSON, with
finish_reason=length. The in-process worker uses a structural tag instead, so the
frontend buffers the parser-native markup and hides the incomplete call.

This PR:

  • Adds --dyn-structural-tag / DYN_STRUCTURAL_TAG to CommonArgs in
    dynamo-backend-common, with the Python flag's values: true gives scope
    always with schema auto, false disables tags, and a JSON object sets
    individual fields (scope defaults to always; unknown fields are rejected).
    Unset, each sidecar applies its own default through CommonArgs::structural_tag.
  • Defaults per sidecar, each still settable with the flag:
  • Publishes the SGLang sidecar's tool_call_structural_tag_reasoning_gate and
    token_budget, matching the in-process SGLang worker:
    • Reasoning gate. The frontend already sends require_reasoning=true, so SGLang
      holds the grammar until </think>. Without the key the frontend also put an
      AnyText…</think> prefix in the structural tag. After the real </think> the
      grammar was still in its reasoning state, so Kimi K2 forced tool calls ran twice
      and leaked tool markers into content. With tags on by default this would hit
      every reasoning model, so the key ships with the default. Published when SGLang
      reports a reasoning_parser and skip_tokenizer_init is off.
    • Token budget. Without it, a prompt past the context window reached SGLang,
      which returns INTERNAL, and the client got 500 instead of 400.
  • Updates the structural-tag and runtime-configuration docs.

E2E evidence

From the Dynamo E2E tool-calling harness on GB200, sidecars on Dynamo
1.6.0.dev20261016 against the in-process nightly:

  • customer_truncated_tool_markup_hidden (stream) fails on every vLLM and SGLang
    sidecar run with unterminated arguments, and passes in-process. On 2026-10-15
    it passed on the vLLM sidecar with this change plus vllm#61892.
  • Kimi K2.6 on the SGLang sidecar: 18 Custom failures with tool markers in
    content on named/required tool choice, and one BFCL request that returned 500
    for a 10354-token prompt against a 10240-token context.

Not included

  • No legacy --dyn-enable-structural-tag / --dyn-structural-tag-scope /
    --dyn-structural-tag-schema flags or env vars; they are deprecated on the
    Python side.
  • No XGrammar version check for tool_arguments_any_order. The engine runs in a
    separate process, and the frontend already requires parsers v2 for that option.
  • The backend-common mocker example also flattens CommonArgs but still
    registers without structural tags.
  • Turning tags on by default for vLLM and TensorRT-LLM. That is a one-line change
    per sidecar once their engines are ready.

Validation

  • cargo test -p dynamo-backend-common --lib args: pass, including the new
    structural_tag_flag_overrides_the_backend_default (unset, bare flag, false,
    DYN_STRUCTURAL_TAG=off|on, against a default-on and a default-off backend) and
    structural_tag_json_keeps_always_scope_unless_set.
  • cargo test -p dynamo-sglang-sidecar -p dynamo-vllm-sidecar -p dynamo-trtllm-sidecar -p dynamo-sglang-mocker:
    all pass (TensorRT-LLM 51 + 2 ignored). New or updated: the SGLang
    worker_publishes_structural_tag_policy (on by default, false disables), the vLLM
    structural_tags_are_off_by_default, the TensorRT-LLM registration test (off by
    default, bare flag enables), and the SGLang reasoning-gate and token-budget tests.
  • CUDA_VISIBLE_DEVICES= HF_HUB_OFFLINE=1 cargo test -p dynamo-sidecar-testkit --tests -- --test-threads=2:
    1 + 22 router + 25 mocker integration pass. assert_registration checks that the
    SGLang card carries structural_tag.scope = always, token_budget and the gate,
    and that the vLLM card has no structural tag.
  • cargo clippy --all-targets -- -D warnings on the six changed crates; rustfmt
    through pre-commit.
  • Not yet re-run: the e2e truncation case and Kimi K2.6 Custom/BFCL on an SGLang
    sidecar built from this branch.

🤖 Generated with Claude Code

@alec-flowers
alec-flowers requested review from a team as code owners October 15, 2026 05:19
@github-actions github-actions Bot added fix documentation Improvements or additions to documentation labels Oct 15, 2026
@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: c0f4bf72-7b81-4299-ade8-aee2157dec5c
📥 Commits

Reviewing files that changed from the base of the PR and between 71a6887 and 5f04240.

📒 Files selected for processing: 10
  • docs/fern/pages/reference/components/runtime-configuration.mdx
  • docs/fern/pages/use-cases/tool-calling-and-reasoning/structural-tag.md
  • lib/backend-common/src/args.rs
  • lib/backend-common/src/worker.rs
  • lib/sidecar/sglang/src/engine.rs
  • lib/sidecar/testkit/tests/support/sglang.rs
  • lib/sidecar/testkit/tests/support/vllm.rs
  • lib/sidecar/trtllm/src/engine.rs
  • lib/sidecar/trtllm/src/tests/engine.rs
  • lib/sidecar/vllm/src/engine.rs

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

Common arguments now parse the dynamic structural-tag policy. The vLLM, SGLang, and TensorRT-LLM sidecars pass that policy into worker configuration. Tests and documentation cover the enabled default, disabling the option, and supported configuration inputs.

Changes

Structural-tag policy

Layer / File(s) Summary
Structural-tag argument and defaults
lib/backend-common/src/args.rs, lib/backend-common/src/worker.rs
CommonArgs accepts --dyn-structural-tag and DYN_STRUCTURAL_TAG. Boolean values enable or disable the policy, and JSON objects can set configuration fields. Tests cover defaults, overrides, and rejected inputs.
Sidecar policy propagation and documentation
lib/sidecar/{vllm,sglang,trtllm}/src/engine.rs, lib/sidecar/{vllm,sglang,trtllm}/src/tests/engine.rs, lib/sidecar/testkit/tests/support/{sglang,vllm}.rs, docs/fern/pages/reference/components/runtime-configuration.mdx, docs/fern/pages/use-cases/tool-calling-and-reasoning/structural-tag.md
The three Rust sidecars pass the configured policy into worker configuration. Tests check the default policy and disabling behavior, and registration assertions check the Always scope. Documentation describes the default and supported options.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 5f042

No actionable issue is established from the supplied evidence. The change is mergeable after normal checks; the sidecar truncation case remains worth validating.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 8 files. (2 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title accurately identifies the SGLang sidecar change and its default structural-tag behavior. It does not mention the broader vLLM, TensorRT-LLM, and shared argument changes, but it remains clear…
Description check Passed The description provides detailed context, implementation details, validation results, limitations, and an issue reference. It does not follow the template headings exactly and does not include a dedi…
Full details: Title check

Explanation

The title accurately identifies the SGLang sidecar change and its default structural-tag behavior. It does not mention the broader vLLM, TensorRT-LLM, and shared argument changes, but it remains clear and relevant.

Full details: Description check

Explanation

The description provides detailed context, implementation details, validation results, limitations, and an issue reference. It does not follow the template headings exactly and does not include a dedicated Related Issues section with a closing directive, but the required information is mostly present.

Full details: Docstring Coverage

Explanation

Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 8 files. (2 skipped: 2 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

@devin-ai-integration

devin-ai-integration Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

✅ Dynamo PR CI passed — run 39320042524 (attempt 3) on 1af9152655

Gate checks: ✅ backend-status-check · ✅ deploy-status-check · ✅ dynamo-status-check

Posted automatically by Devin for run 39320042524. Updated on every full-CI run of this PR.

Comment thread lib/sidecar/sglang/src/engine.rs Resolved
Comment thread lib/sidecar/trtllm/src/engine.rs Resolved
Comment thread lib/backend-common/src/args.rs Resolved
@alec-flowers

Copy link
Copy Markdown
Contributor Author

End-to-end check (Qwen3.6-35B-A3B, vLLM sidecar, tool-calling suites):

  • On current vLLM nightly, enabling structural tags makes every tool request through the vLLM sidecar fail with HTTP 500. vLLM's Rust gRPC server sends all structured outputs to the guidance backend, which only reads the legacy structural-tag format, not the xgrammar format tag Dynamo sends (KeyError: 'triggers').
  • With a vLLM patch that makes the gRPC path use xgrammar (Python vLLM's default), this PR fixes the target case: a forced tool call cut off by max_tokens no longer streams unterminated argument JSON. Custom and BFCL results stay within normal run-to-run variance.
  • Suggest gating the vLLM sidecar default on a vLLM release with that fix. Upstream vLLM PR to follow.

@harryskim harryskim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc-only review. The doc edits match the new parse_structural_tag behavior. Inline comments cover the TRT-LLM workaround conflict, where the note sits, and a sidecar opt-out example. A few smaller nits on lines this PR doesn't touch in structural-tag.md:

  • L115 tool_arguments_any_order: says it requires XGrammar >= 0.2.3, but per the PR description sidecars don't check the engine's XGrammar version. Worth noting that sidecars don't validate it at startup.
  • L114 reasoning_boundary: the SGLang sidecar doesn't publish tool_call_structural_tag_reasoning_gate, so auto always resolves to structural_tag there. Optional to document, but it would answer the "why doesn't backend kick in" question.
  • L122: the Rust parser also accepts 1/0, yes/no, and on/off. Fine to leave undocumented unless Python accepts them too, in which case listing them keeps the two in sync.

alec-flowers and others added 2 commits October 18, 2026 14:40
In-process vLLM, SGLang, and TensorRT-LLM workers publish structural tags
with scope `always` and schema `auto` by default. The Rust sidecars never
set `WorkerConfig.structural_tag`, so forced tool choices fell back to
guided JSON, and a truncated tool call streamed unterminated argument JSON
instead of being hidden.

Add `--dyn-structural-tag` / `DYN_STRUCTURAL_TAG` to `CommonArgs` with the
Python flag's values (true, false, or a JSON object whose scope defaults to
`always`). Unset, each sidecar applies its own default through
`CommonArgs::structural_tag`:

- SGLang: on. SGLang compiles Dynamo's structural tags natively.
- vLLM: off until vLLM's gRPC server compiles structural tags with xgrammar
  (vllm-project/vllm#61892); its guidance backend rejects them.
- TensorRT-LLM: off. The xgrammar version it pins rejects several tool-call
  tag formats (NVIDIA/TensorRT-LLM#19913).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Alec Flowers <aflowers@nvidia.com>
The in-process SGLang worker publishes two runtime keys that the native
Rust sidecar left out. The frontend makes both decisions; the sidecar
now only reports the engine facts it reads from GetServerInfo.

- tool_call_structural_tag_reasoning_gate: the frontend already sends
  require_reasoning=true, so SGLang holds the grammar until </think>.
  Without the key the frontend also put an AnyText...</think> prefix in
  the structural tag. After the real </think> the grammar was still in
  its reasoning state, so Kimi K2 forced tool calls ran twice and leaked
  tool markers into content. Published when SGLang reports a
  reasoning_parser and skip_tokenizer_init is off, the same predicate as
  publish_sglang_structural_tag_reasoning_policy.
- token_budget: without it, a prompt past the context window reached
  SGLang, which returns INTERNAL, and the client got 500 instead of 400.
  Published from the existing combined_token_limit (context minus the
  EAGLE reserve) and allow_auto_truncate, matching _get_token_budget.
  SGLang always validates total tokens, so both rejection flags follow
  allow_auto_truncate. Left unpublished when either input is unknown.

The SGLang mocker now reports allow_auto_truncate=false, SGLang's
default, so the testkit registration check covers token_budget end to
end.

Signed-off-by: Alec Flowers <aflowers@nvidia.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alec-flowers
alec-flowers force-pushed the aflowers/sidecar-structural-tag branch from 5f04240 to 4cd8400 Compare October 18, 2026 22:29
@alec-flowers
alec-flowers requested a review from a team as a code owner October 18, 2026 22:29
@alec-flowers alec-flowers changed the title fix(sidecar): publish default-on structural-tag policy from Rust sidecars fix(sidecar): publish structural tags from the SGLang sidecar by default Oct 18, 2026
alec-flowers and others added 2 commits October 19, 2026 10:24
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Alec Flowers <aflowers@nvidia.com>
…cument sidecar opt-out

The mocker example flattens CommonArgs but dropped --dyn-structural-tag.
Docs: add the SGLang sidecar opt-out, move the sidecar note under
Preserve Previous Behavior, and replace the legacy TensorRT-LLM workaround
flag, which the sidecars reject, with --dyn-structural-tag false.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Alec Flowers <aflowers@nvidia.com>
@rmccorm4
rmccorm4 requested a review from indrajit96 October 19, 2026 19:34
…ool_opt

The hand-rolled true/false match tripped dynamo-truthy's no_bool_parse_forks lint.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Alec Flowers <aflowers@nvidia.com>

@dmitry-tokarev-nv dmitry-tokarev-nv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No P0 or P1 found. This review read head 1af91526552ba71afc7a00121b82e2530f3d9abb.

  • [P2] lib/sidecar/sglang/src/engine.rs:179: no test checks the new SGLang default or the flag parser after 6e0c72b0e5. The inline comment has a tested patch.
  • [P3] PR description: the Validation section names tests that 6e0c72b0e5 removed, and "Not included" says that the mocker example registers without structural tags. Details below.
[P3] Description text that does not match this head.
  • Validation names "the new structural_tag_flag_overrides_the_backend_default" and "structural_tag_json_keeps_always_scope_unless_set". Commit 6e0c72b0e5 removed both tests.
  • Validation names "the SGLang worker_publishes_structural_tag_policy", "the vLLM structural_tags_are_off_by_default", and "the TensorRT-LLM registration test (off by default, bare flag enables)". Commit 6e0c72b0e5 removed all three.
  • Validation says that assert_registration checks "structural_tag.scope = always" on the SGLang card and that "the vLLM card has no structural tag". On this head, neither check looks at structural_tag. The SGLang check asserts token_budget and that the gate key is absent.
  • Not included says that the mocker example "still registers without structural tags". Commit 7060b1314f wires the flag there. With --dyn-structural-tag, the mocker example registers scope always with schema auto.
  • Summary says that the frontend "hides the incomplete call". In the end-to-end run below (hermes parser), the head does not hide it. The client gets one tool call with arguments closed to {"city": "Par"} and finish_reason length.
The flag: Rust matches the Python flag in 58 of 63 cases. The table lists the 5 differences.

This review ran CommonArgs and the Python parser from runtime_args.py on this head with the same 63 cases. The values were unset, a bare flag, the true and false words, mixed case, and padded spaces. They also included an empty value, each allowed JSON field, an unknown field, {"scope":null}, malformed JSON, [], null, and "true". Each value ran as the flag and as DYN_STRUCTURAL_TAG. Five more cases set both.

Input Rust CommonArgs Python flag
true values (true, 1, yes, on, any case, padded) scope always, schema auto same
false values (false, 0, no, off, any case) none same
{} and each allowed field scope always unless set, the field as given same
unknown field, {"scope":null}, malformed JSON, [], null, "true" startup error startup error
empty value, as the flag or as DYN_STRUCTURAL_TAG startup error startup error
unset sidecar default (SGLang on, vLLM and TensorRT-LLM off) on
{"tool_arguments_any_order":true} accepted, no XGrammar check XGrammar version check
bad DYN_STRUCTURAL_TAG with a valid flag (3 cases) the flag wins startup error

The unset default and the XGrammar check are described in the PR. The last row is Rust being more lenient. This review found no other difference.

Registered `structural_tag` per sidecar on this head.
Binary unset --dyn-structural-tag or true false
dynamo-sglang-sidecar scope always, schema auto scope always, schema auto none
dynamo-vllm-sidecar none scope always, schema auto none
dynamo-trtllm-sidecar none scope always, schema auto none
mocker example none scope always, schema auto none

At the merge base, every sidecar registered none, because the flag did not exist.

End to end: the head fixes the truncated forced tool call and the context overflow.

Setup: the testkit HTTP frontend, a real dynamo-sglang-sidecar process, and the SGLang mocker, with tool_call_parser set to hermes in GetServerInfo. The mocker held each request and read the guided-decoding constraint that the sidecar sent. It then replied with tokens that fit the constraint. The reply stops at max_tokens with finish_reason length.

Request Merge base 2141a9690d Head
tool_choice required, streamed Constraint json_schema. Deltas {"city": and "Par, so arguments stay unterminated. length. Constraint structural_tag. One tool call, arguments {"city": "Par"}. length.
tool_choice required, not streamed json_schema. No tool call and no content. length. structural_tag. One tool call, arguments {"city": "Par"}. length.
named tool, streamed same as required, streamed same as required, streamed
named tool, not streamed same as required, not streamed same as required, not streamed
prompt of 32778 tokens, context 32768, not streamed 500 with the mocker set to return INTERNAL, as SGLang does. 400 with the mocker default INVALID_ARGUMENT. The request reached the engine. 400 context_length, limit 32768. The request did not reach the engine.
same prompt, streamed HTTP 200 with an in-band error event (500 or 400) 400
SGLang runtime data compared with the in-process worker: the same values in the common case, and the differences are listed.

Measured on the sidecar card, through the testkit with GetServerInfo patches:

GetServerInfo tool_call_structural_tag_reasoning_gate token_budget
no reasoning parser absent {"combined_limit":32768,"reject_prompt_overflow":true,"reject_total_overflow":true}
reasoning_parser set true same
reasoning_parser set, skip_tokenizer_init: true absent same
reasoning_parser set, skip_tokenizer_init: false true same
allow_auto_truncate: true absent {"combined_limit":32768,"reject_prompt_overflow":false,"reject_total_overflow":false}

The in-process worker (components/src/dynamo/sglang/register.py, read, not run) publishes the same values in these cases. Differences found by reading the code:

  • The in-process gate has four more conditions. It needs a tokenizer and require_reasoning support in the engine. Its think_end_token must encode to at least one token. The handler must not be multimodal or diffusion-LM. The sidecar checks only reasoning_parser and skip_tokenizer_init. SGLang 0.5.22 carries require_reasoning on its native gRPC GenerateRequest.
  • In-process reject_total_overflow is validate_total_tokens and not allow_auto_truncate. The sidecar uses not allow_auto_truncate. SGLang 0.5.22 sets validate_total_tokens = True and never clears it, so the values match there.
  • When the EAGLE reserve is larger than the context, in-process publishes combined_limit 0, and the sidecar publishes no token_budget.
  • If GetServerInfo has no allow_auto_truncate, the sidecar publishes no token_budget. The same is true for adaptive speculation. The in-process worker always publishes it.

This review did not measure a user-visible effect of these differences.

Earlier review threads: each point is addressed on this head.

All 7 threads are resolved. This review agrees with each point and found each one addressed:

  • The reasoning gate key (KrishnanPrash): published since 4cd8400eeb. The deliberate break of the skip_tokenizer_init condition fails reasoning_gate_follows_the_engine_reasoning_parser.
  • TensorRT-LLM tags without xgrammar (KrishnanPrash): the TensorRT-LLM sidecar now registers none by default.
  • The mocker example drops the flag (KrishnanPrash): fixed in 7060b1314f, measured above.
  • The four docs threads (harryskim): the note moved under Preserve Previous Behavior, the TensorRT-LLM workaround uses --dyn-structural-tag false, the SGLang sidecar opt-out is listed, and runtime-configuration.mdx names the sidecar defaults. Each statement matches the measured defaults.
  • The three notes in the harryskim review body: the description lists the XGrammar check for tool_arguments_any_order under "Not included". This review measured that the sidecar flag accepts the field without a check. The note about the missing gate key no longer applies. Python also accepts 1, 0, yes, no, on and off (table above), so the two parsers agree.
Tests, merge with main, and CI.
  • Tests on this head (macOS): backend-common 161, SGLang sidecar 121, vLLM sidecar 132, TensorRT-LLM sidecar 51 (2 ignored), mocker example 20, SGLang mocker 11, router integration 22, mocker integration 25. Local router integration runs were flaky: 2 of 4 full runs on this head timed out in 1 to 5 tests, at different steps and for vLLM too. The merge base had no timeout in its one run. CI rust-tests (.) passed all 22 router and 25 mocker integration tests on Linux, on a merge of this head.
  • Merge with main 649bc1469578aeeecc30099ce706185e917479d1: no conflict. The same tests pass on the merge, except one local router timeout of the same kind.
  • CI on this head: pre-merge-status-check, deploy-status-check, rust-tests (.), rust-clippy (.) and pre-commit passed. The docs checks, the SGLang and vLLM runtime tests, and the TensorRT-LLM arm64 runtime test also passed. trtllm-runtime / Test cuda13.1, amd64 and dynamo-runtime / test / parallel cuda13.0, arm64 failed because their pytest output stopped at the same second (21:05:38Z) with no test failure. dynamo-runtime / rust-gpu was cancelled. These three jobs make backend-status-check and dynamo-status-check fail. None of these jobs runs the sidecar code that this PR changes, so this review does not relate them to the diff.
  • Require an issue link failed because the PR names no issue. 44 checks were skipped (deploy, snapshot, dev-image and Helm lanes). A skipped check is not a pass.

Comment thread lib/sidecar/sglang/src/engine.rs Resolved

@dmitry-tokarev-nv dmitry-tokarev-nv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No P0 or P1 found at head 1af91526552ba71afc7a00121b82e2530f3d9abb.

This review compared the flag with the Python flag on 63 inputs, and read the registered structural_tag of each sidecar. It also sent a truncated forced tool call and an overlong prompt through the frontend, the SGLang sidecar and the SGLang mocker. It did this at the merge base and at this head. The head fixes the unterminated arguments and returns 400 for the overlong prompt.

Open items:

  • [P2] lib/sidecar/sglang/src/engine.rs:179: no test checks the new SGLang default or the flag parser after 6e0c72b0e5. The inline comment has a tested patch.
  • [P3] The PR description names tests that 6e0c72b0e5 removed, and says that the mocker example registers without structural tags.

CI: dynamo-runtime / rust-gpu was cancelled on this head, and two runtime test jobs stopped mid-run at 21:05:38Z without a test failure.

@alec-flowers
alec-flowers enabled auto-merge (squash) October 20, 2026 00:06
@alec-flowers
alec-flowers merged commit 98fb6f8 into main Oct 20, 2026
266 of 277 checks passed
@alec-flowers
alec-flowers deleted the aflowers/sidecar-structural-tag branch October 20, 2026 00:27
erezzarum pushed a commit to erezzarum/dynamo that referenced this pull request Oct 23, 2026
…ult (ai-dynamo#16494)

Signed-off-by: Alec Flowers <aflowers@nvidia.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
JulienDarve added a commit that referenced this pull request Nov 3, 2026
….33+

The vLLM sidecar now defaults structural tags on when vLLM reports version
0.33 or later, as the in-process vLLM worker and the SGLang sidecar (#16494)
already do. vLLM 0.33.0 is the first release whose gRPC server compiles these
tags with xgrammar (vllm-project/vllm#61892); older servers send them to
guidance, which rejects them, so the default stays off there. The version comes
from ServerInfo.engine_version and is compared with pep440_rs.

Folded in from #17866.

Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation fix size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants