Skip to content

fix(beam): finalize constrained tool calls at confirmed model EOS - #17625

Merged
luckyq merged 1 commit into
mainfrom
luckyq/beam-eos-finalize
Nov 5, 2026
Merged

luckyq merged 1 commit into
mainfrom
luckyq/beam-eos-finalize

Conversation

@luckyq

@luckyq luckyq commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Overview

Port of #17062 (release/1.6.0 Beam branch, by @neilkg) to main. Part of #17618; the stacked follow-up porting #17061 closes it.

Key Changes

  • Problem: under an installed Beam turn grammar, a complete tool body that ends at genuine model EOS can lose the call. Concrete case: the model emits a complete <rfl:parameter> body and then its EOS <|return|> instead of <|call|>. Because the Beam route retains the model's stop-token text, the parser sees the trailing <|return|> and drops the call, so the batch path returns 500 and the stream path an error. (At a bare EOF with no trailing marker the pinned parser's Drop policy already recovers the body; the retained-marker case is the one that fails.)
  • Cause: the chat stream response carried no EOS evidence, and the unified parser always finalized with finish().
  • Fix: model_eos choice indices on the chat stream response (internal; scrubbed before HTTP and in synthetic chunks), emitted only while an installed Beam turn grammar is active; the unified parser and the aggregator finalize with finish_at_eos() only under confirmed EOS and that grammar, so length limits, user stops and transport EOF never authorize strict completion; Beam parse errors surface as request errors instead of being dropped; apply_argument_framing takes the family so malformed-call rejection applies only to Beam under the turn grammar.
  • Adapted to main: crate pins are unchanged (dynamo-renderer 8.2.0, dynamo-parsers-v2 0.9.6, which carries finish_at_eos); the family-aware framing is threaded through main's new_with_constraint and the continuation-cache path; scrub_synthetic_chunk_metadata clears model_eos next to message_boundary.

Validation

Linux arm64 build: cargo fmt --check, cargo check --workspace --all-targets and cargo clippy -p dynamo-llm -D warnings pass; cargo test -p dynamo-llm passes (4639 tests, 0 failures, including the three new tests). Original validation on the release branch is in #17062.

Issues and Ongoing Work

@luckyq
luckyq requested review from a team as code owners October 29, 2026 22:43
@github-actions github-actions Bot added fix frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` labels Oct 29, 2026
@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Walkthrough

The change adds confirmed model EOS metadata to chat responses and uses it during Beam unified tool parsing. Backend stop handling now distinguishes system EOS IDs from user stops. Beam parser errors are emitted as errors rather than recovered as text.

Changes

Beam Tool EOS Handling

Layer / File(s) Summary
Distinguish system EOS from user stops
lib/llm/src/backend.rs
The decoder tracks system stop IDs separately, retains their text, and handles overlapping stop IDs and stop strings. The regression tests cover stop reasons and output token text.
Carry confirmed EOS in chat responses
lib/llm/src/protocols/openai/chat_completions.rs, lib/llm/src/protocols/openai/chat_completions/delta.rs, lib/llm/src/preprocessor.rs, lib/llm/src/http/service/metrics.rs, lib/llm/src/http/service/openai.rs, lib/llm/src/perf/logprobs.rs, lib/llm/src/preprocessor/speculative_prefill.rs, lib/llm/src/protocols/anthropic/stream_converter.rs, lib/llm/src/protocols/openai/chat_completions/tool_parser_v2.rs, lib/llm/src/protocols/openai/responses/stream_converter.rs, lib/llm/src/request_trace/*, lib/llm/tests/*
The response type adds an internal model_eos choice-index list. The delta generator sets it when EOS handling is enabled and the backend reports EOS. The chat preprocessor enables this for Beam turn grammar. Response processing clears or serializes the field as described, and affected response fixtures initialize it.
Apply EOS-aware Beam tool parsing
lib/llm/src/protocols/openai/chat_completions/aggregator.rs, lib/llm/src/protocols/openai/chat_completions/unified_parser.rs
The aggregator passes confirmed EOS state to the parser. Beam parsing applies Beam-specific framing and EOS finalization. Stored Beam parse errors are emitted as errors. Tests cover streaming and batch finalization cases.

Estimated code review effort: 4 (Complex) | ~40 minutes

Merge Risk: Minimal · up to 9be18

The identified EOS-stop overlap does not break Beam tool parsing. No actionable merge-blocking issue remains after normal checks.

Pre-merge checks: Passed 4 · Inconclusive 1

Failed Failed checks: 1 inconclusive

Check name Status Explanation Resolution
Docstring Coverage Inconclusive Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed

Check name Status Explanation
Title check Passed The title clearly identifies the main change: finalizing constrained Beam tool calls at confirmed model EOS.
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 follows the repository template's main sections (Overview, Key Changes, Validation, Issues and Ongoing Work) and gives a clear account of the problem, cause, fix, and validation. Howev…
Full details: Description check

Explanation

The description follows the repository template's main sections (Overview, Key Changes, Validation, Issues and Ongoing Work) and gives a clear account of the problem, cause, fix, and validation. However, the checklist item ("I used Dynamo's /pr-checklist skill") is absent, and the "Closes #XXXX" line is replaced by "Part of #17618", which the template's issue-link check expects to be a closing reference once release/1.7.0 is cut. The validation section also says cargo test -p dynamo-llm passes, while the PR objectives say that run was pending, so the two sources conflict on test status.

Full details: Docstring Coverage

Explanation

Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 14 functions across 19 files. (3 skipped: 3 too large.)

Resolution

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

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

@luckyq
luckyq force-pushed the luckyq/beam-eos-finalize branch from 9be18bb to de2c04c Compare October 29, 2026 22:54
@devin-ai-integration

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

Copy link
Copy Markdown
Contributor

✅ Dynamo PR CI passed — run 41565196564 (attempt 1) on 35412ebe0a

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

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

@luckyq
luckyq force-pushed the luckyq/beam-eos-finalize branch 3 times, most recently from 35412eb to 1988a96 Compare November 4, 2026 20:28

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

Thank you


No blocking correctness issues found at 1988a96. The two inline comments are nonblocking cleanup.

Validated on macOS with --no-default-features: all four focused EOS/parser regression tests passed. cargo fmt --check and git diff --check also passed. The full test suite was not rerun.

Comment thread lib/bindings/python/Cargo.lock Resolved
@luckyq
luckyq force-pushed the luckyq/beam-eos-finalize branch from 1988a96 to 686e9e9 Compare November 4, 2026 23:59
…7062)

Signed-off-by: neilkg <33635204+neilkg@users.noreply.github.com>
(cherry picked from commit f3dff14)
Signed-off-by: doxu <doxu@nvidia.com>
@luckyq
luckyq force-pushed the luckyq/beam-eos-finalize branch from 686e9e9 to 99f8465 Compare November 5, 2026 00:14
@keivenchang

Copy link
Copy Markdown
Contributor

Reviewed 99f8465d8f7e: no P0/P1 findings. 347 focused tests passed, with 10 existing tests ignored, including both review probes. Both prior human cleanup findings are addressed. P3 follow-up: the comment at unified_parser.rs:1888 still promises text fallback; qualify it for non-Beam families because Beam errors now propagate. CI is still running, and canonical parser qualification remains incomplete.

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

approved — thanks Dong for fixing the EOS handling and addressing the cleanup.

@luckyq
luckyq merged commit 4fb7b3a into main Nov 5, 2026
130 checks passed
@luckyq
luckyq deleted the luckyq/beam-eos-finalize branch November 5, 2026 01:58
ishandhanani added a commit to ishandhanani/dynamo that referenced this pull request Nov 5, 2026
Third upstream merge for the split v2 PR: the five commits past
53015f9 that landed while the second merge was validated. Four
conflicts, each placed in the owner crate, upstream behavior whole, the
stack's moves intact, no shims.

- ai-dynamo#18008 probe worker availability with an Fx set: WorkerIdSet and the
  per-set AvailableWorkerIds cache in the kernel's types.rs, the listed-
  worker walk in the kernel's filter.rs, the actor's cached availability in
  queue.rs (auto-merged); WorkerIdSet exported to hosts through
  dynamo_scheduler::host (lib/llm's LoRA availability filter) and to
  plugins through dynamo_kv_router::plugins::prelude (the builtin
  default_policy test stays prelude-only).
- ai-dynamo#18084 CRTC stale-leaf sweep trigger scaled with tree size:
  lib/router/kv-indexer concurrent_radix_tree_compressed (auto-merged).
- ai-dynamo#17900 shared fixed-lane bench primitive: lib/bench (auto-merged).
- ai-dynamo#17947 planner priority pools, ai-dynamo#17625 beam tool-call finalization:
  outside the router, auto-merged.

Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
bhargavchintam added a commit to bhargavchintam/dynamo that referenced this pull request Nov 11, 2026
main stopped compiling lib/llm/tests/streaming_tool_dispatch_http.rs when
ai-dynamo#13986 merged: its chunk() helper builds NvCreateChatCompletionStreamResponse
field by field and predates the model_eos field from ai-dynamo#17625. This is the same
one-line change as ai-dynamo#18553, so the two merge cleanly in either order.

Signed-off-by: Bindu Bhargava R Chintam <bindubhargavareddy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants