Repository navigation
[skip ci][Doc] Module design doc for diffusion runtime - #6440
Conversation
|
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 appears to belong to: docs/design/module/diffusion/diffusion_runtime.md. Module owners: @Isotr0py @princepride @SamitHuang @fhfuih @fhfuih, 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. |
a67ad76 to
f7e9654
Compare
SamitHuang
left a comment
There was a problem hiding this comment.
Checked this draft against current vllm_omni/diffusion on main (UniProcDiffusionExecutor, scheduler, multiproc DLO dispatch, abort/cleanup). Three accuracy issues should be fixed before this is treated as the runtime source of truth: last_verified_commit points at a doc SHA from this PR, the DLO+AllGather exception is described as scheduler policy, and PREEMPTED is drawn as a normal engine path though only tests call preempt_request().
| - diffusers.DiffusionPipeline | ||
| last_reviewed: 2026-07-16 | ||
| last_reviewed: 2026-08-25 | ||
| last_verified_commit: f7e96541dd11c21dce50fdcd9fc5b1269353ee2a |
There was a problem hiding this comment.
last_verified_commit is f7e96541, a documentation commit on this PR (and not even HEAD after 958f39c). Sibling module docs such as engine_orchestration.md point at the code tree that was checked. Point this at the diffusion runtime SHA you actually verified (UniProcDiffusionExecutor, dummy-run skip, #6413 cleanup), then update it whenever those sections change.
There was a problem hiding this comment.
There was a problem hiding this comment.
[P2]
19da238still does not contain #6439 or #6580, while lines 351-357 describe both cleanup behaviors as current. Please either merge/rebase this document after those dependencies and setlast_verified_committo a tree that actually contains them, or mark those paragraphs as pending.
I rephrased relevant paragraphs in the current doc and remove clearly affirmative tone that we does it. If this is to be merged before those two PRs, I think the current tone is more suitable (and you can open a new comment if you disagree, lest I miss the thread here)
If you plan to merge it after those two PRs, signal me to modify these paragraphs later if you feel needed
There was a problem hiding this comment.
Answering the open question above: the requirement tone is fine to keep, but at current main nothing releases this bookkeeping at abort/consumer-drop time — release happens when the late OUTPUT_READY reaches the pump, and an unconsumed result is parked in _completed_outputs by design (#6023). The orphan-after-abort case is #6413, and #6253 / #6439 / #6580 are all still open. Suggest keeping the sentence but tagging it "(pending #6253/#6439/#6580)" so it does not read as current behavior.
There was a problem hiding this comment.
Suggest keeping the sentence but tagging it "(pending #6253)" so it does not read as current behavior.
Done
958f39c to
d058fb1
Compare
Thanks for the careful review. Resolved all of them in the latest commit. Also rebased onto latest main |
d058fb1 to
62f3c98
Compare
SamitHuang
left a comment
There was a problem hiding this comment.
Re-checked the Aug 31 draft against current vllm_omni/diffusion on main (DiffusionExecutor.get_class, UniProcDiffusionExecutor, scheduler, DLO dispatch, abort/cleanup). PREEMPTED, COMPUTE_DONE ordering, and the executor-side DLO routing note from the last pass are fixed. Remaining accuracy gaps: last_verified_commit is 69 commits behind a tree that already changed runtime files, DLO concurrency is still under-specified, dummy-run skip is vaguer than the predicate in code, and INV-002 does not admit the per-rank request pick.
Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com>
Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com>
Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com>
Expand DLO DP concurrency (capacity, coalescing, homogeneity, rank pick), name max_num_seqs as the scheduler capacity knob, document the exact dummy-run skip predicate, and carve the per-rank AllGather dispatch out of INV-002. Also restore the dropped Debugging map and Known limitations sections. Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com>
Document diffusion_kv_metadata on NewRequestData, correct multiproc shutdown so completed async outputs drop with the executor, and tag abort/consumer-drop async-output reclaim as pending vllm-project#6253/vllm-project#6439/vllm-project#6580. Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com>
62f3c98 to
c69b66f
Compare
|
@SamitHuang @hsliuustc0106 Resolved your recent reviews about DLO, new KV connector's metadata, and cleanup behaviors Seems nothing to add to Cleanup PRs (6253 / 6439 / 6580) are still open — no change to the pending tags. |
…6440) Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com> Co-authored-by: Alicia <115451386+congw729@users.noreply.github.com> Signed-off-by: ZhengWG <zwg0606@gmail.com>
…6440) Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com> Co-authored-by: Alicia <115451386+congw729@users.noreply.github.com>
…6440) Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com> Co-authored-by: Alicia <115451386+congw729@users.noreply.github.com> Signed-off-by: wenjie.yan <wenjyan@outlook.com>
…6440) Signed-off-by: Huang, Zeyu <11222265+fhfuih@users.noreply.github.com> Co-authored-by: Alicia <115451386+congw729@users.noreply.github.com>
Purpose
Document diffusion runtime (from engine to model runner) in the new design module doc.
This PR does not change the draft status of this doc, so it can still receive other's completion before published.
Test Plan
NA. Doc only.