Repository navigation
[Bugfix] Fix grouped chunk interaction latency - #8810
Conversation
Signed-off-by: maithilijoshi20 <97733343+maithilijoshi20@users.noreply.github.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR matches CODEOWNERS paths: /tests/. Code owners: @NickCao @yenuo26 Routing: @NickCao via CODEOWNERS; @yenuo26 via CODEOWNERS @maithilijoshi20, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Signed-off-by: maithilijoshi20 <97733343+maithilijoshi20@users.noreply.github.com>
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
|
@maithilijoshi20 thanks for picking this up. I validated #8810 with the #8756 reproduction on one A800 (SM80). #8754 (Self-Forcing) was applied on top of this PR ( Interaction latency. Setup:
Output. Streamed latents (AsyncOmni, Throughput, with grouping on, which is the path this PR changes. End of the 81-frame WS stream without an update, median of 5 runs after 1 warmup, same GPU:
The first chunk arrives at 1.13 s in both. I left out timings for the ungrouped runs: the PR does not change that path, and another job started on the shared GPU while they ran. Tests on this PR + #8754:
Not run: LingBot World itself (no weights here). It goes through the same runner path. One optional suggestion. From the reporter's side, this fixes #8756 in the original reproduction. AI assistance: the validation runs and this comment were done with Claude Code. |
With the boundary step left for a fresh call, a two-step chunk runs one step per call, so tests whose stubs use two-step chunks no longer reach the grouped continuation. Use three-step chunks where a test is about grouped steps: - grouped steps see the same inputs as per-step calls; - a failing step ends the grouped call. Dropping a continuation step's output passed the module before this change and now fails this test; - the interaction test queues its update after two grouped steps, and the boundary call runs exactly one step; - the call-pattern test covers two- and three-step chunks. Also make the _execute_stepwise_core docstring match where the grouped call now stops. Signed-off-by: windyskr <54269955+windyskr@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@maithilijoshi20 a small follow-up to my comment above. With the boundary step left for a fresh call, the grouping tests whose stubs use two-step chunks no longer reach the grouped continuation. For example, dropping a continuation step's output in the grouped loop still passes the whole module. I opened maithilijoshi20#1 against your branch. It:
It touches tests and one docstring only (91 passed). Feel free to merge it, cherry-pick it or ignore it. AI assistance: drafted with Claude Code. |
Keep the grouped-runner tests exercising grouped steps
|
The fix direction and implementation look correct: leaving the chunk-boundary step for the next scheduler call should remove the one-chunk interaction delay, while preserving the one-step-chunk limitation. One minor test-quality concern: |
Signed-off-by: maithilijoshi20 <97733343+maithilijoshi20@users.noreply.github.com>
|
@tzhouam PTAL |
Gaohan123
left a comment
There was a problem hiding this comment.
LGTM. Thanks. And please submit a PR to v0.32.0
Omni ReviewBot: supersededThe CI failure noted on |
Fixes #8756.
Purpose
What broke
When a streaming AR diffusion pipeline enabled
supports_chunk_step_grouping, one worker invocation ran every denoise step through the chunk boundary. An interaction submitted while that invocation was running could not be processed until after the runner had already applied the boundary and prepared the next chunk, so the interaction became active one chunk late.Reproduction
The multi-step regression is covered by a deterministic runner-level test:
Before this fix, the grouped call reaches the boundary immediately and the test cannot queue the interaction before boundary application.
Root cause
ARDiffusionModelRunner._execute_stepwise_coregrouped the final step of the current chunk. The base runner applies pending interactions and callsprepare_next_chunkas part of that final step, before control returns to the scheduler/RPC loop.Fix and scope
Stop grouped execution after reaching the penultimate step. The boundary-finishing step runs in a fresh scheduler invocation, allowing pending interaction RPCs to be handled first.
This latency improvement applies only to chunks with at least two denoise steps. A one-step chunk has no pre-boundary yield point and retains the baseline one-call interaction latency. A dedicated regression test documents that limitation.
Tests cover boundary yielding, interaction timing, the one-step scope restriction, cached continuations, grouped failures, grouped call patterns for two- and three-step chunks, and output equivalence.
Test Plan
vLLM Version: 0.31.0
vLLM-Omni Base:
233f08421Test Result
Local WSL2
91 passed, 3 deselectedacross the expanded stepwise runner and streaming interaction suites (-k "not parallel").mypy-3.10, which reports existing errors on unchanged lines in both touched files (for example runner lines 142/366 and test lines 272/1157 onupstream/main). No new mypy error is introduced by this diff.A800 reproduction (Windyskr)
generation_chunk=1with this PR; the grouped base first activates it atgeneration_chunk=2.