Skip to content

refactor: P3 make ChoiceState grammar state explicit - #17719

Merged
rmccorm4 merged 2 commits into
mainfrom
keivenchang/DIS-3306__choice-grammar
Nov 3, 2026
Merged

rmccorm4 merged 2 commits into
mainfrom
keivenchang/DIS-3306__choice-grammar

Conversation

@keivenchang

@keivenchang keivenchang commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Overview

This refactor makes native grammar bookkeeping explicit in the OpenAI streaming adapter and tests Native state selection across Qwen, Nemotron, GLM, Kimi, GPT-OSS, DeepSeek V4, and Muse Spark.

Details

  • Store Qwen XML and GLM native adapter state in explicit variants; guided output and other native families select no adapter-specific grammar state.
  • Cover 21 family/output-mode combinations, including Native None expectations for Kimi, GPT-OSS, DeepSeek V4, and Muse Spark.
  • Keep deferred Qwen reasoning source envelopes in ChoiceRecord and use a generic stream-projection helper.

Verification

Tested commit: 49b778c.

  • cargo test --locked -p dynamo-llm --lib unified_parser: 202 passed, 10 ignored.
  • The focused Native grammar selection test passed (1/1). A negative control that assigned QwenXml to deepseek_v4 failed at the expected state assertion; the final candidate test passes.
  • pre-commit run --files lib/llm/src/protocols/openai/chat_completions/unified_parser.rs passed.
  • Running the 10 ignored probes produced 2 passes and 8 failures. The failures are six family native-progress probes, the Qwen every-split recovery-ordering probe, and the eight-family argument-progress matrix. The prior baseline comparison recorded the same failure assertions.
  • Parser qualification remains incomplete on this exact head. Gate pr17719-20261103-010039 is incomplete: lifecycle and mode coverage, split/recovery, payload ownership, shape/trust, wrapper and consumer parity, grammar-domain and semantic test ownership, comment claims, and artifact regeneration proofs remain uncertified. The eight existing native-progress and recovery probe failures remain under separate investigation in DIS-3306.

Review scope and follow-up

I’ll address P0/P1 findings in this PR. Let’s handle P2/P3 in follow-up PRs, tracked in DIS-3306. For AI reviewers, report reproducible P0/P1 findings here; record P2/P3 as follow-up work without requesting changes for them on this PR.

The existing native-progress and recovery failures and the remaining parser qualification proofs are follow-up work tracked in DIS-3306.

Where should the reviewer start?

lib/llm/src/protocols/openai/chat_completions/unified_parser.rs

Summary by CodeRabbit

  • Refactor
    • Improved consistency when processing streamed responses from supported model families, including responses delivered in pieces or ending without a complete final segment. Model-specific formatting and response metadata are now handled consistently as streams progress and finish.

Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com>
@github-actions github-actions Bot added refactor frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` labels Oct 30, 2026
@keivenchang keivenchang self-assigned this Oct 30, 2026
@keivenchang
keivenchang marked this pull request as ready for review October 30, 2026 21:43
@keivenchang
keivenchang requested a review from a team as a code owner October 30, 2026 21:43
@coderabbitai

coderabbitai Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The unified chat-completions parser now stores family-specific native grammar bookkeeping in a selected adapter state. Streaming parsing, finish recovery, deferred Qwen reasoning projection, and EOF flushing use that state. Tests cover adapter selection and updated fixtures.

Changes

Native grammar stream state

Layer / File(s) Summary
Adapter state and family selection
lib/llm/src/protocols/openai/chat_completions/unified_parser.rs
ChoiceState now stores NativeGrammarState. Native Qwen and Nemotron select Qwen XML state, native GLM selects GLM state, and other families or guided output select no adapter state. Tests check selection and initialize updated test state.
Streaming parse and finish recovery
lib/llm/src/protocols/openai/chat_completions/unified_parser.rs
Qwen opener and lexical bookkeeping and GLM truncation input now use the selected adapter state during parsing, finish handling, and parser failure recovery.
Deferred reasoning projection and flushing
lib/llm/src/protocols/openai/chat_completions/unified_parser.rs
Deferred Qwen reasoning records and event positions now use the adapter state across choice projection, detour handling, and EOF flushing. The chunk projection helper and deferred-response identifiers are renamed.

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

Merge Risk: Minimal · up to 4971b

No merge-blocking regression is established, though parser qualification remains incomplete.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly identifies the refactor and its main change: making ChoiceState grammar state explicit.
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.
Description check Passed The description is mostly complete. It covers the overview, key changes, validation results, known failures, ongoing work, linked follow-up issue, and review scope. It uses “Details” and “Verification…
Full details: Description check

Explanation

The description is mostly complete. It covers the overview, key changes, validation results, known failures, ongoing work, linked follow-up issue, and review scope. It uses “Details” and “Verification” instead of the template headings “Key Changes” and “Validation,” and it omits the pull request checklist and a “Closes #XXXX” entry, but these omissions are non-critical.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 3057891 and 4971bbf.

Files selected for processing: 1
  • lib/llm/src/protocols/openai/chat_completions/unified_parser.rs

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

@keivenchang

keivenchang commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Replying to #17719 (comment): Fixed in 49b778c: the Native selection matrix now checks None for gpt_oss, deepseek_v4, and muse_spark.

@devin-ai-integration

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

Copy link
Copy Markdown
Contributor

✅ Dynamo PR CI passed — run 41556664412 (attempt 1) on 49b778ccb2

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

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

@luckyq luckyq 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.

Summary: Behaviour-preserving refactor: the six per-family grammar fields collapse into NativeGrammarState, chosen once in ChoiceState::new by the same predicates main used; every former read/write site is either inside an already-gated block or operated on provably empty state on main. No blockers.

Non-blocking

  • lib/llm/src/protocols/openai/chat_completions/unified_parser.rs:5295 — selection matrix omits families that must map to None under Native (gpt_oss, deepseek_v4, muse_spark); a future widening of uses_qwen_xml_grammar would pass unnoticed (P3, DIS-3306).

Checked: diff read field-by-field against main (48 read/write sites); mutual exclusivity of the glm47 / qwen-xml predicates on main; no post-construction reassignment of the replaced fields on main; rename call sites and drain points unchanged; ChoiceRecord stays per-choice. Tests not run; CI green on 4971bbf.

@keivenchang

keivenchang commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor Author

Replying to #17719 (review): Fixed in 49b778c: the matrix now asserts Native None for gpt_oss, deepseek_v4, and muse_spark, and the negative control catches deepseek_v4 selecting QwenXml.

Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com>

@luckyq luckyq 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.

Re-approving at 49b778c: the selection-matrix test now asserts None for native gpt_oss / deepseek_v4 / muse_spark; test-only delta, CI green.

@keivenchang

Copy link
Copy Markdown
Contributor Author

Replying to #17719 (review): addressed in 49b778c — native_grammar_state_matches_family_and_output_mode now asserts Native None for gpt_oss, deepseek_v4, and muse_spark.

@rmccorm4
rmccorm4 enabled auto-merge (squash) November 3, 2026 17:27
@rmccorm4
rmccorm4 merged commit a48bc25 into main Nov 3, 2026
129 checks passed
@rmccorm4
rmccorm4 deleted the keivenchang/DIS-3306__choice-grammar branch November 3, 2026 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` refactor size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants