fix(frontend): emit SGLang stream role once - #12741
Conversation
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test b16bd34 |
WalkthroughChangesSGLang role emission
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/src/dynamo/frontend/tests/test_sglang_processor_unit.py (1)
3183-3199: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the first reasoning delta contains the role.
roles == ["assistant"]proves only that one role was emitted. It still passes if a later delta contains the role. Record emitted deltas and assert that the first returned delta has"role": "assistant"and later deltas do not. Apply the same assertion to the existing required-tool test to cover the parser path.Suggested assertion
- roles = [] + emitted_deltas = [] ... - if "role" in delta: - roles.append(delta["role"]) + emitted_deltas.append(delta) ... - assert roles == ["assistant"] + assert emitted_deltas + assert emitted_deltas[0]["role"] == "assistant" + assert all("role" not in delta for delta in emitted_deltas[1:])The PR objective requires role emission on the first returned choice across content, finish-only, reasoning, and parser-based streams.
🤖 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 `@components/src/dynamo/frontend/tests/test_sglang_processor_unit.py` around lines 3183 - 3199, Update the streaming test around post.process_output to retain each returned delta, then assert the first emitted delta contains role "assistant" and all subsequent deltas omit role instead of only checking roles == ["assistant"]. Apply the same first-versus-later delta assertions to the existing required-tool test, covering both content and parser-based stream paths.
🤖 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 `@components/src/dynamo/frontend/tests/test_sglang_processor_unit.py`:
- Around line 3183-3199: Update the streaming test around post.process_output to
retain each returned delta, then assert the first emitted delta contains role
"assistant" and all subsequent deltas omit role instead of only checking roles
== ["assistant"]. Apply the same first-versus-later delta assertions to the
existing required-tool test, covering both content and parser-based stream
paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 75989a35-f2f8-48f5-b3d3-0aaa17d9c8c5
📒 Files selected for processing (2)
components/src/dynamo/frontend/sglang_prepost.pycomponents/src/dynamo/frontend/tests/test_sglang_processor_unit.py
This comment has been minimized.
This comment has been minimized.
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test 578823a |
rmccorm4
left a comment
There was a problem hiding this comment.
LGTM, thanks @xianlubird
dyn-3691-extract-shared-target-pid-cuda-customstorage-operation-layer * 'main' of https://github.com/ai-dynamo/dynamo: (65 commits) fix(frontend): emit SGLang stream role once (#12741) docs(fern): promote v1.3.1 to current release (#12752) fix(docs): remove duplicate unscoped community-rail CSS rules (#12615) feat(operator): migrate CRD storage to v1beta1 (#11904) fix: synchronize self-benchmark capacity across DP ranks (#12021) chore(deps): bump dynamo-tokenizers to 1.8.0 (#12707) fix(frontend): preserve split UTF-8 characters (#12688) docs: align Kubernetes build selector with CLI (#12729) fix(frontend): preserve completion backend error status (#12706) fix(operator): replace snapshot pods after GMS restart (#11286) refactor(media): rename installer module, drop --packages per review fix(media): harden installer against three pre-redesign review findings fix(media): verify installs in a fresh interpreter; teach --pip-args= form test(serve): install test-time decoders at the validated bounds feat(media): explicit installer for additional media decoders docs(spica): correct kv_load_ratio support guidance (#12714) feat(operator): add experimental grove.forceScalingGroup for single-node components (#11772) fix(vllm): declare entry-stage engine_input_source in GLM-Image NIXL config (#12709) chore: bump trtllm to v1.3.0rc23 (#12532) perf: remove trtllm postprocessing workers from the args as post processing workers are not effective in dynamo (#12592) ... Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Summary
delta.roleonly on the first choice produced by the SGLang streaming postprocessor.assistantrole when generation finishes before producing any content.Background
PR #12639 established the chat-stream contract at the HTTP boundary: the first emitted delta for a choice carries
role: assistant, while later deltas omit the role. The SGLang Python postprocessor builds streaming choice dictionaries outside the native Rust postprocessing path, so it still attached the role to every content-bearing delta and relied on the HTTP layer to remove the repeats.That arrangement also left an edge case at the source. When SGLang finishes immediately without generating a token, the postprocessor returns a finish-only choice with an empty delta. The HTTP deduplication logic can remove duplicate roles, but it cannot recover a role that was never emitted.
This change tracks role emission in the per-request SGLang postprocessor and attaches the role only when a choice is actually returned. Buffered UTF-8 sequences and parser-buffered output do not consume the initial role. If the first returned choice is a finish-only choice, it still carries
role: assistant; any later content or terminal choices omit it.Validation
black --checkon the modified Python filesisort --check-onlyon the modified Python filesruff checkon the modified Python filespython3 -m py_compileon the modified Python filesgit diff --checkThe targeted pytest module was not run locally because the current virtual environment does not include pytest or the SGLang test dependencies.
Summary by CodeRabbit
Bug Fixes
Tests