Repository navigation
[codex] Fix diffusion worker timeout for large broadcast payloads - #4845
hsliuustc0106 merged 7 commits into
Conversation
df0ad8b to
0a0cbbb
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@david6666666 @hsliuustc0106 |
|
Can we specify it without modifying environment variables? |
Yes. This requires a slightly broader change because the timeout needs to flow through the configuration stack instead of an environment variable. I’ll add it as an OmniDiffusionConfig field, wire it through the stage config/CLI, and have the worker consume it directly. The default remains 30 seconds, with -1 meaning an indefinite wait. Would this approach be okay with you? |
There was a problem hiding this comment.
Pull request overview
This PR makes the diffusion worker’s shared-memory broadcast receive loop more robust for multi-GB diffusion/video RPC payloads by replacing the hard-coded 1s MessageQueue.dequeue timeout with a configurable, longer default.
Changes:
- Add
VLLM_OMNI_DIFFUSION_WORKER_BROADCAST_TIMEOUT_Sto configure the diffusion worker’s broadcast dequeue timeout (default 30s, with fallback behavior for invalid values). - Log the effective broadcast dequeue timeout at worker startup.
- Use the configured timeout value in the worker busy loop’s
MessageQueue.dequeue(...)call.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if timeout_s <= 0: | ||
| logger.warning( | ||
| "Invalid %s=%r; using default %.1fs", | ||
| _BROADCAST_DEQUEUE_TIMEOUT_ENV, | ||
| raw_timeout, | ||
| _DEFAULT_BROADCAST_DEQUEUE_TIMEOUT_S, | ||
| ) | ||
| return _DEFAULT_BROADCAST_DEQUEUE_TIMEOUT_S |
|
At |
acd082a to
bb32498
Compare
Thanks for pointing this out. I’ve switched to indefinite waiting and added a regression test for delayed overflow payloads. Does this look okay? |
|
The indefinite receive plus delayed-overflow regression test addresses the ordering loss. Looks good now. |
|
@david6666666 Can we merge then? |
Signed-off-by: jiangweifeng.dev <jiangweifeng.dev@bytedance.com>
Signed-off-by: jiangweifeng.dev <jiangweifeng.dev@bytedance.com>
bb32498 to
916cfa0
Compare
|
@david6666666 @hsliuustc0106 |
Signed-off-by: Wojciech Kutak <wkutak@nvidia.com>
|
https://buildkite.com/vllm/vllm-omni/builds/13418/canvas a few ci failed |
Signed-off-by: jiangweifeng.dev <jiangweifeng.dev@bytedance.com>
@hsliuustc0106 Fixed in 99bc28e. Could you please retrigger CI? Thanks! |
Signed-off-by: jiangweifeng.dev <jiangweifeng.dev@bytedance.com>
| worker_proc.wake_event = None | ||
| worker_proc._running = True | ||
|
|
||
| # Make the pre-fix implementation fail quickly: it read this module-level |
There was a problem hiding this comment.
This regression doesn't bite on the pre-fix code: the delayed release is 0.1s (line 155), well inside the old dequeue(timeout=1.0) bound, so main passes this test as-is. Also _BROADCAST_DEQUEUE_TIMEOUT_S doesn't exist on either main or this branch — raising=False patches an attribute nothing reads (leftover from the env-var revision?).
Since the SHM marker is consumed before the socket read, a finite timeout only loses the message when the delay exceeds it. Can you drop the monkeypatch and extend the delay past the old 1s bound (e.g. 1.5s) so reintroducing a finite timeout actually fails this test? Good timing to fold it in before the Buildkite retrigger — the new test has no green run on 8b07c43 yet.
|
|
||
| def recv_message(self) -> Any: | ||
| """Receive one complete broadcast message without dropping overflow data.""" | ||
| return self.mq.dequeue(indefinite=True) |
There was a problem hiding this comment.
The PR description still documents the earlier env-var revision — VLLM_OMNI_DIFFUSION_WORKER_BROADCAST_TIMEOUT_S, 30s default, invalid-value fallback, startup log — none of which is in the final diff. Can you update the Change section to match the indefinite dequeue? The root-cause and validation sections are still accurate.
There was a problem hiding this comment.
The PR description still documents the earlier env-var revision —
VLLM_OMNI_DIFFUSION_WORKER_BROADCAST_TIMEOUT_S, 30s default, invalid-value fallback, startup log — none of which is in the final diff. Can you update the Change section to match the indefinite dequeue? The root-cause and validation sections are still accurate.
Thanks, updated the PR description.
…lm-project#4845) Signed-off-by: jiangweifeng.dev <jiangweifeng.dev@bytedance.com> Co-authored-by: jiangweifeng.dev <jiangweifeng.dev@bytedance.com> Co-authored-by: Hongsheng Liu <liuhongsheng4@huawei.com>
Summary
Make the diffusion worker wait indefinitely for each complete shared-memory
broadcast message instead of abandoning a receive after the hard-coded
1-second timeout.
This fixes long video/diffusion requests whose RPC payloads go through the
vLLM
MessageQueueoverflow path. Once an overflow marker has been consumed,the worker must wait for the corresponding socket payload; retrying
dequeueafter a timeout can lose or reorder requests.Fixes #4844.
Root cause
For large video transfer requests, the serialized RPC request can exceed 2 GB.
The current worker loop calls:
If the overflow socket message is not ready within 1 second,
dequeueraises,the worker loop catches the exception and continues, and the request is lost
from the worker's perspective. The API server then waits for a result that never
comes back.
Change
Add
WorkerProc.recv_message(), which callsself.mq.dequeue(indefinite=True), and use it from the worker busy loop so aconsumed overflow marker is always paired with its complete multipart payload.
Add regression coverage for indefinite receive and for preserving request order
when an overflow payload is delayed.
Validation
Ran:
Runtime validation on a Cosmos3 V2V/transfer request:
all workers timed out at
recv_poll_start timeout_ms=1000.recv_poll_ready, received the 2.08 GB multipart payload, enteredpipeline_forward_start, and the request returned HTTP 200 with 189 frames.