fix(frontend): return SGLang chat logprobs - #12820
Conversation
Signed-off-by: jain-ria <riajain@NVIDIA.com>
WalkthroughStreaming SGLang responses now carry per-token log probabilities and optional top candidates. The frontend converts these values into OpenAI-compatible ChangesStreaming log probability support
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/src/dynamo/frontend/sglang_prepost.py (1)
1146-1160: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep probability arrays aligned after EOS removal.
Line 1149 removes trailing EOS token IDs. It does not remove the matching
log_probsandtop_logprobsentries._build_openai_logprobsthen detects a length mismatch and returnsNone.Trim both probability arrays to the retained token count after validating their original alignment. Add a terminal-chunk test with generated tokens followed by EOS.
Proposed fix
if finish_reason is not None: + raw_token_count = len(token_ids) token_ids = self._strip_trailing_eos_token_ids(list(token_ids)) + if log_probs is not None and len(log_probs) == raw_token_count: + log_probs = log_probs[: len(token_ids)] + if top_logprobs and len(top_logprobs) == raw_token_count: + top_logprobs = top_logprobs[: len(token_ids)]🤖 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/sglang_prepost.py` around lines 1146 - 1160, Update the EOS handling in the response-processing flow around _strip_trailing_eos_token_ids to validate that log_probs and top_logprobs are aligned with the original token_ids, then trim both arrays to the retained token count after EOS removal. Keep _build_openai_logprobs receiving matching token and probability lengths, and add a terminal-chunk test covering generated tokens followed by EOS.
🧹 Nitpick comments (1)
components/src/dynamo/frontend/sglang_prepost.py (1)
1057-1057: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet an explicit
zip()strictness value.The project requires Python 3.10 or later, and Ruff reports B905 here. Use
strict=True.🤖 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/sglang_prepost.py` at line 1057, Update the zip call in the token iteration loop to pass the explicit strict=True argument, resolving Ruff B905 while preserving the existing enumerate behavior.Sources: Coding guidelines, Path instructions, Linters/SAST tools
🤖 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.
Inline comments:
In `@components/src/dynamo/frontend/sglang_prepost.py`:
- Around line 1156-1177: Update the incremental decoding flow around
`_incremental_decode` and `_pending_decode_ids` to retain logprob entries when
decoding returns an empty string for a deferred byte-fallback sequence. Queue
the probability data with the pending token IDs, then emit it only when those
matching IDs produce decoded text, rather than allowing `sglang_processor.py` to
clear it prematurely. Add a test covering one UTF-8 byte-fallback sequence split
across stream chunks and verify its text and logprobs are preserved.
---
Outside diff comments:
In `@components/src/dynamo/frontend/sglang_prepost.py`:
- Around line 1146-1160: Update the EOS handling in the response-processing flow
around _strip_trailing_eos_token_ids to validate that log_probs and top_logprobs
are aligned with the original token_ids, then trim both arrays to the retained
token count after EOS removal. Keep _build_openai_logprobs receiving matching
token and probability lengths, and add a terminal-chunk test covering generated
tokens followed by EOS.
---
Nitpick comments:
In `@components/src/dynamo/frontend/sglang_prepost.py`:
- Line 1057: Update the zip call in the token iteration loop to pass the
explicit strict=True argument, resolving Ruff B905 while preserving the existing
enumerate behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8458afde-f517-4bba-8d85-c456e71b19b2
📒 Files selected for processing (2)
components/src/dynamo/frontend/sglang_prepost.pycomponents/src/dynamo/frontend/sglang_processor.py
Signed-off-by: jain-ria <riajain@NVIDIA.com>
Signed-off-by: jain-ria <riajain@NVIDIA.com>
|
Thanks @jain-ria ! Please address the devin comments 🙏 |
Signed-off-by: jain-ria <riajain@NVIDIA.com>
|
🎯 Code Coverage (details) 🔗 Commit SHA: ec3b64b | Docs | Datadog PR Page | Give us feedback! |
Signed-off-by: jain-ria <riajain@NVIDIA.com>
Signed-off-by: jain-ria <riajain@NVIDIA.com>
8eba427 to
31f8c33
Compare
|
/ok to test 31f8c33 |
Signed-off-by: jain-ria <riajain@NVIDIA.com>
|
/ok to test ec3b64b |
|
Some of the more problematic looking findings from local review:
However, I don't think any of these issues are blocking to the release, so these can be followed up in separate PRs to |
rmccorm4
left a comment
There was a problem hiding this comment.
Some comments for future follow-up, not blocking to this PR: #12820 (comment)
LGTM to unblock the release fix.
Second stale generator on main, same class as the release tables in the previous commit and blocking every PR the same way: gen_python_api.py --check fails on pristine origin/main, so any PR that runs pre-merge inherits it. Cause is #12820 (fix(frontend): return SGLang chat logprobs), which moved code in components/src/dynamo/frontend/. The reference deep-links to exact source lines, so shifting sglang_processor.py and sglang_prepost.py by ~54 and ~104 lines invalidated eight of them. Nothing about the docstrings changed; the line anchors did. That is worth noting for anyone editing that package: this reference goes stale on any line-number shift in the source it documents, not only on docstring edits. #12820 had no way to know -- the generator landed with #12110 after it was already open. Output only, no hand edits. Validation: all four generators (--check on python, rust, kubernetes, plus gen_llms_tables) now pass on this branch; python and llms_tables both fail on pristine main. Signed-off-by: Dan Gil <dagil@nvidia.com>
Overview:
Fix SGLang chat-completion responses returning
logprobs.content: nullwhen logprobs were requested, including requests withlogprobs=trueandtop_logprobs=0.The SGLang backend calculated and returned token logprobs, but the frontend discarded them before postprocessing. The postprocessor also hardcoded the response’s
logprobsfield toNone.Details:
log_probsandtop_logprobswhile batching generated tokens in the SGLang frontend.tokenlogprobbytestop_logprobsnull.Where should the reviewer start?
components/src/dynamo/frontend/sglang_processor.pycomponents/src/dynamo/frontend/sglang_prepost.pyRelated Issues
Summary by CodeRabbit
New Features
Bug Fixes