Repository navigation
fix(router): account for all output blocks in response chunks - #14613
Conversation
|
👋 Hi bzsuni! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughOutput block tracking now counts every crossed scheduler block. Request handling enqueues one update per crossed block. Tests cover boundary crossings, decay, disabled tracking, chunk invariance, scheduler load, cleanup, and configurable router setup. ChangesOutput block accounting
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change corrects output-block accounting across scheduler boundaries without changing default behavior or wire formats. Regression coverage verifies boundary crossings, cleanup, decay, and disabled tracking, so it is merge-ready. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
6607d6c to
fbceccb
Compare
fbceccb to
b3c24a7
Compare
b3c24a7 to
c5fc0c5
Compare
jthomson04
left a comment
There was a problem hiding this comment.
Reviewed c5fc0c5. No new findings from the source review. Both earlier review comments are addressed. Tests were not run.
|
/ok to test c5fc0c5 |
Track the number of block boundaries crossed by each response chunk and enqueue an update for every new block. Previously, a multi-block token delta advanced the tracker but added only one block to the scheduler load. Add regression coverage using the real request guard and scheduler across output chunk sizes, partial prompt blocks, and disabled tracking. Preserve decay weighting and verify request cleanup. Signed-off-by: bzsuni <bingzhe.sun@daocloud.io>
c5fc0c5 to
9664347
Compare
|
/ok to test 9664347 |
|
Non-blocking follow-up: with output tracking and Tracked in #15446. |
Overview:
With
--router-track-output-blocksenabled, the router undercounts output blocks when a response chunk crosses more than one block boundary.This makes its local decode-load estimate depend on how the backend groups tokens into responses.
OutputBlockTrackeradvances its total to include all newly crossed blocks, butRequestGuardsends only oneAddOutputBlockupdate. Later chunks cannot recover the missing blocks because the tracker has already advanced.The fix carries the number of new blocks in
OutputBlockUpdateand enqueues an update for each through the existing booking-checked scheduler path.It leaves the decay formula, cleanup, and local-only load updates unchanged. Output-block tracking remains disabled by default; no backend or wire-format changes are needed.
Details:
The regression uses the actual request guard and scheduler, without GPU inference.
For a 16-token prompt, 48 output tokens, and block size 16, single-token chunks count 4 blocks before and after the fix. Grouping the same tokens as
[1, 32, 15]exposes the bug.SGLang's real output accumulator also produces this layout with
stream_interval=32in the CPU producer probe.Recorded output for the grouped case:
Before — the regression fails:
After — the same regression passes:
Where should the reviewer start?
Related Issues
🚫 This PR is NOT linked to an issue:
Summary by CodeRabbit
Bug Fixes
Tests