Repository navigation
[BDE] Phase 1: engine-level KV cache management for DreamZero (RFC #4366) - #4534
Conversation
hsliuustc0106
left a comment
There was a problem hiding this comment.
DCO and pre-commit checks failing. Fix those before requesting full review.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Review findings:
I did not run pytest locally: this active environment does not have |
| _log.info("BDE RESET [%s/%s] session KV cleared (window boundary)", pos_id, neg_id) | ||
|
|
||
|
|
||
| class BDEPipelineMixin: |
There was a problem hiding this comment.
This mixin is exported (kv_cache/__init__.py) but never actually used: DreamZeroPipeline defines its own _kv_get / _kv_create / _kv_update / _kv_commit / _kv_get_cross / _kv_reset proxies directly and doesn't inherit BDEPipelineMixin. The two have already diverged — _bde_kv_update here is missing the seq_len arg the real _kv_update in pipeline_dreamzero.py passes. Suggest deleting the mixin (avoids a misleading duplicate surface that can silently drift further), or wiring the pipeline to inherit it.
| x = self.o(x) | ||
| return x | ||
|
|
||
| def project_kv(self, context: torch.Tensor) -> tuple[torch.Tensor, torch.Tensor]: |
There was a problem hiding this comment.
Both project_kv methods (here and on WanI2VCrossAttention) are never called — _kv_populate_cross inlines the projection (ca.norm_k(ca.k(projected)).unflatten(2, (n, d)) …) rather than calling block.cross_attn.project_kv(...). Only a docstring references it. Either drop these, or call them from _kv_populate_cross so the K/V projection lives in one place.
| # BDE engine (engine-level KV cache management) unless the | ||
| # caller explicitly selected an engine backend. | ||
| bde_default = default_engine_backend_for_model(self.model_class_name) | ||
| if bde_default is not None and self.engine_backend == "default": |
There was a problem hiding this comment.
Design question on blast radius: both enrich_config (here) and from_kwargs rewrite engine_backend "default" -> "bde" for DreamZeroPipeline, so every DreamZero load now builds BDEModelRunner — even though the KV feature is off by default and the production topology (CFG-parallel + CUDA graph + torch.compile) is still unvalidated per the remaining-work doc.
Have you considered gating the routing behind the same enable switch (BDE_KV_ENABLE / the config field) so DreamZero stays on base DiffusionEngine until BDE KV is explicitly opted in? That would shrink the blast radius of a half-validated runner to zero for anyone not testing BDE. Side note: because "default" gets overridden here, there's currently no way to force base DiffusionEngine for DreamZero.
| """Resident (non-null) blocks the read path gathers the window from.""" | ||
| return resident_block_ids(self.block_table(adapter), self.null_block_id) | ||
|
|
||
| def commit_chunk(self, adapter: BDERequestAdapter) -> None: |
There was a problem hiding this comment.
Minor, but easy to misread: commit_chunk is only reached from tests in practice. In production, completed_chunks advances via adapter.on_chunk_committed() directly inside BDEKVState.update_kv_cache (gather.py), and the pipeline's _kv_commit -> BDEKVState.commit_chunk is a no-op log. A one-line docstring noting the real commit path (or collapsing the two) would help the next reader.
|
Re-review with #4425 ( Context: #4425 introduces the additive structured config layer and a Findings:
I did not run pytest locally: this active environment does not have |
| filtered_kwargs = {k: v for k, v in kwargs.items() if k in valid_fields} | ||
|
|
||
| return cls(**filtered_kwargs) | ||
| instance = cls(**filtered_kwargs) |
There was a problem hiding this comment.
Heads-up on coordination with #4425 (RFC #4021 Phase 2 structured config), which is also open and touches this exact region:
- Merge conflict is certain. [Config] Add structured VllmOmniConfig classes #4425 refactors
OmniDiffusionConfig.from_kwargshere — it extracts the legacy kwarg-normalization into a module-levelnormalize_omni_diffusion_kwargs()and keeps the tail asreturn cls(**filtered_kwargs). This block (which changes that tail toinstance = cls(...)+ the per-model routing) will conflict textually, and [Config] Add structured VllmOmniConfig classes #4425 also adds fields toOmniDiffusionConfig. Whichever lands second rebases; worth syncing order with @Acerak01-fy. - Mirror-config debt. [Config] Add structured VllmOmniConfig classes #4425's new structured
DiffusionConfigis a hand-maintained explicit mirror ofOmniDiffusionConfig's diffusion knobs (with a parity test), not a dynamicfields()enumeration — so theengine_backend/diffusion_model_runner_clsfields added here won't auto-propagate into the structured config. That's exactly the fragmentation RFC [RFC]: Unified VllmOmniConfig #4021 is trying to retire. Either mirror them into [Config] Add structured VllmOmniConfig classes #4425'sDiffusionConfig(probably under the engine/runtime group, where these belong) or note the deferral deliberately. - Env-var direction. The
BDE_KV_ENABLEstopgap + standaloneBDEKVConfiggo the opposite direction from [Config] Add structured VllmOmniConfig classes #4425's structured-config push. The stated next-step config-field enablement would land most naturally inside the newDiffusionConfigrather than as another env var / parallel dataclass.
|
Review summary — BDE Phase 1 (#4534) High-quality PR: clean engine/runner separation, honest eager-mode perf methodology, bit-exact parity evidence, and a solid 72-test CPU suite. No hard blockers from me — leaving as COMMENT. Five inline notes:
Verified: the gather memoization is correct against DreamZero's denoise cadence (base |
Address the PR vllm-project#4534 review findings, relocate BDE under a new experimental package, and add memory/memoization profiling support. Fixes (from the bde_phase1 review): - P1 session-KV leak: bound BDEModelRunner._bde_states with an LRU at the same cap as DreamZeroPipeline._states and free pool blocks on eviction via a new BDEKVState.close(), so session-id churn can no longer leak the KV pool until exhaustion. - P2 frame_seqlen source of truth: read it from the loaded transformer; the deploy-config inference is now only a drift cross-check (warns on mismatch). This surfaced a real 880-vs-220 mismatch on DreamZero-DROID. - KV pool floor: guarantee 2*(window_chunks+1)+2 blocks so the memory fraction can't under-size the pool (fixes exhaustion exposed by the corrected 880-token frame granularity). - P2 benchmark config helper: frame-granular (chunk_size=frame_seqlen, window_chunks=local_attn_size), matching the runtime. - P3: remove the drifted, unused BDEPipelineMixin (its update path was missing the required seq_len argument). Refactor: - Move vllm_omni/bde -> vllm_omni/experimental/bde; update all imports, the engine resolver, and the dynamic runner-class path string. Profiling: - BDE_KV_NO_MEMO env toggle to A/B the per-forward gather memoization. - gpu_mem_stats() worker RPC + peak-VRAM capture in bde_perf_compare. Validated on H200 (DreamZero-DROID): tests/bde 73 passing; bit-exact parity (max_abs_diff=0, PSNR=inf) for KV-on and for both memo/no-memo vs the KV-off baseline. Three-way profile confirms the memoization win (-9.25 GiB peak VRAM, -101 ms/chunk, sawtooth flattened). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Taichang Zhou <tzhouam@connect.ust.hk>
…commit_chunk docs) Close inline review findings 2-5 from hsliuustc0106: - Inline 2: delete the two unused project_kv methods in causal_wan_model.py (_kv_populate_cross inlines the identical projection); fix the stale comment. - Inline 3: gate DreamZero->BDE engine/runner routing behind BDE_KV_ENABLE via resolve_default_engine_backend(), so non-BDE deploys stay on base DiffusionEngine and base is selectable again. +4 unit tests. - Inline 4: docstrings clarifying the two commit_chunk paths (test-only vs the real advance inside BDEKVState.update_kv_cache). - Inline 5: vllm-project#4425 structured-config coordination is blocked (not in this tree); deferral marker left at the field defs + a note in the remaining-work doc. Validated on H200 (DreamZero-DROID): tests/bde 77 passing; routing gate confirmed end-to-end (KV-off -> base engine, BDE_KV_ENABLE=1 -> BDE); bit-exact parity (max_abs_diff=0, PSNR=inf) preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Taichang Zhou <tzhouam@connect.ust.hk>
Bring PR vllm-project#4534 up to date with upstream main (DreamZero CUDA graph / torch.compile / DiT step-cache vllm-project#4213, tp & cross-attn cache fix vllm-project#4154, GR00T, stepcache, etc.). Conflict resolution: - pipeline_dreamzero.py: * diffuse loop: adopt upstream's run_dit (DiT step-cache) wrapper, keep BDE's _kv_get / _kv_get_cross proxies for all KV reads. * reset: adopt upstream's reset_reason (session/inference) model; route both session and inference (window-boundary) resets through _kv_reset so the BDE pool resets too (parity). _kv_reset gains clear_video_latents. * take upstream's state.append_video_latents(video_out). * cross-attn: BDE now uses the model-local cross-attn cache (upstream vllm-project#4154 added k_img/v_img image-token caching that BDE's text-only pool lacked); _kv_get_cross returns model-local, _kv_populate_cross is a no-op. BDE's self-attn paged pool is unchanged. - causal_wan_model.py: silent auto-merge duplicated the crossattn_cache arg in _forward_blocks (upstream vllm-project#4154 and our wiring-gap fix are the same change) — de-duplicated to one param. - video_export_worker.py: keep both gpu_mem_stats (ours) and _latents_to_uint8_frames (upstream); switch gpu_mem_stats off the now-banned torch.cuda.* to torch.accelerator.* (TID251). - bde_perf_compare.py: update to upstream's new _build_observations signature. Validated on H200 (DreamZero-DROID): tests/bde 77 passing; whole package compiles; KV-off (base engine) and BDE_KV_ENABLE=1 both run on the merged code and stay bit-exact (max_abs_diff=0, PSNR=inf). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Taichang Zhou <tzhouam@connect.ust.hk>
…name) The experimental/bde imports sorted before vllm_omni.diffusion.* once the package moved under experimental/; ruff isort wants diffusion first. Fixes the pre-commit (ruff-check) CI check on PR vllm-project#4534. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Taichang Zhou <tzhouam@connect.ust.hk>
|
Inline findings:
Earlier review-comment items:
From the main rebase:
|
This PR is the joint work of the AR-Diffusion / BDE group; crediting all contributors so the squash merge carries every co-author. Co-authored-by: amy-why-3459 <221522807+amy-why-3459@users.noreply.github.com> Co-authored-by: Yangshen Deng <47519522+TKONIY@users.noreply.github.com> Co-authored-by: Wang Fuyin <wfy2003324@163.com> Co-authored-by: Honghan Zhu <nenbaying@163.com> Co-authored-by: Match_yc <57976772+matchyc@users.noreply.github.com> Co-authored-by: Hyoseop Song <lvsy@proton.me> Co-authored-by: Wang Yiwen <121547057+yiwen101@users.noreply.github.com> Co-authored-by: Suli Wang <50828270+wsl2000@users.noreply.github.com> Co-authored-by: Wu Jiazhen <83007646+asukaqaq-s@users.noreply.github.com> Co-authored-by: Yukim1 <121286183+zwhzzz0821@users.noreply.github.com> Co-authored-by: wtz2333 <wtz2333@mail.ustc.edu.cn> Signed-off-by: Taichang Zhou <tzhouam@connect.ust.hk>
test_dreamzero_state_cache_access_requires_initialization exercised the model-local get_kv_caches/update_kv_cache/get_crossattn_caches accessors that the paged self-attention backend removed (all KV is engine-owned now). Replace it with the inverse contract: those accessors must be GONE from DreamZeroState so nothing can silently bypass the engine pool. Fixes the Simple unit-test CI failure; the remaining two failures in that bucket (serve_cli mock, voxcpm2 engine_extras) reproduce on origin/main and are unrelated to this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Taichang Zhou <tzhouam@connect.ust.hk>
main independently landed DreamZero prompt/neg-embed caching as the default (state.prompt_embeds + self._negative_prompt_embeds_cache, vllm-project#4534 BDE KV-cache + v0.24 rebase), superseding W7's flag-gated embed caches. Adopt main's caching as canonical and drop the now-redundant W7 paths: - remove _encode_positive_prompt / _encode_negative_prompt helpers and their VLLM_OMNI_CACHE_{,NEG_}PROMPT_EMBED flags + _POS_CACHE_MAX - drop tests/dreamzero/test_{pos,neg}_embed_cache_equiv.py (equivalence is moot now that caching is the baseline) - keep W7's unique contributions: host-side profiling harness (_hostprof_*) wrapping the tokenize/encode/neg/prefill/diffuse stages, and the scheduler deepcopy skip (VLLM_OMNI_SCHEDULER_FRESH_BUILD) - coexist with main's DZ_PHASE_TIMING (_pt) markers Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Since PR vllm-project#4534 DreamZero's attention KV (self- and cross-) lives in the AR-Diffusion engine's paged pool, so the adapter now mirrors only what DreamZeroState still owns: frame stitching, accumulated video latents, prompt-embed cache, the incremental VAE encoder stream, counters, and reset heuristics. The dense PagedKV placeholder is removed; RFC vllm-project#4480 Phase 1 reintroduces PagedKV as a handle over the engine's session KV state instead of a buffer. Equivalence tests updated for the new surface, including the window-reset preservation semantics. Signed-off-by: Wijnand Suijlen <wijnand.suijlen@huawei.com>
…, so the contract shows where the AR self-attention KV fits: a handle wrapping the engine-owned paged state from vllm-project#4534, not a plain buffer here. Signed-off-by: Wijnand Suijlen <wijnand.suijlen@huawei.com>
…lm-project#4366) (vllm-project#4534) Signed-off-by: Wojciech Kutak <wkutak@nvidia.com>
Since PR vllm-project#4534 DreamZero's attention KV (self- and cross-) lives in the AR-Diffusion engine's paged pool, so the adapter now mirrors only what DreamZeroState still owns: frame stitching, accumulated video latents, prompt-embed cache, the incremental VAE encoder stream, counters, and reset heuristics. The dense PagedKV placeholder is removed; RFC vllm-project#4480 Phase 1 reintroduces PagedKV as a handle over the engine's session KV state instead of a buffer. Equivalence tests updated for the new surface, including the window-reset preservation semantics. Signed-off-by: Wijnand Suijlen <wijnand.suijlen@huawei.com>
…, so the contract shows where the AR self-attention KV fits: a handle wrapping the engine-owned paged state from vllm-project#4534, not a plain buffer here. Signed-off-by: Wijnand Suijlen <wijnand.suijlen@huawei.com>
Since PR vllm-project#4534 DreamZero's attention KV (self- and cross-) lives in the AR-Diffusion engine's paged pool, so the adapter now mirrors only what DreamZeroState still owns: frame stitching, accumulated video latents, prompt-embed cache, the incremental VAE encoder stream, counters, and reset heuristics. The dense PagedKV placeholder is removed; RFC vllm-project#4480 Phase 1 reintroduces PagedKV as a handle over the engine's session KV state instead of a buffer. Equivalence tests updated for the new surface, including the window-reset preservation semantics. Signed-off-by: Wijnand Suijlen <wijnand.suijlen@huawei.com>
…, so the contract shows where the AR self-attention KV fits: a handle wrapping the engine-owned paged state from vllm-project#4534, not a plain buffer here. Signed-off-by: Wijnand Suijlen <wijnand.suijlen@huawei.com>
… deploy config DreamZeroPipeline requires the AR-Diffusion engine and raises at construction when engine_backend resolves to anything else. vllm-project#4534 added engine_backend to vllm_omni/deploy/dreamzero.yaml but not to dreamzero_tp1_cfg2.yaml, so every launch through the CFG-parallel config died during worker startup with: ValueError: DreamZeroPipeline requires the AR-Diffusion engine; set engine_backend: ...ARDiffusionEngine in the deploy config (got engine_backend='default'). This broke the weekly DreamZero e2e serving test, which passes that config explicitly, and examples/online_serving/dreamzero/run_server.sh, which defaults to it. Add the missing engine_backend, plus a CPU test asserting that every shipped dreamzero*.yaml stage selects an AR-Diffusion engine. The test resolves the import path through DiffusionEngine.resolve_engine_class rather than matching a string, so a stale or typo'd path fails too. Previously only an H100x2 slow-marked e2e test covered this. Fixes vllm-project#5046 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What this does
Phase-1 of the AR-Diffusion Engine KV-cache RFC (#4366): DreamZero's persistent attention KV runs on vLLM's paged KV stack, managed at the engine level — no new attention kernel. DreamZero now runs exclusively on the engine (its model-local
cat+slice KV path is removed); the engine was validated bit-exact against that path before removing it.Design
ARDiffusionEngine(DiffusionEngine), selected by theengine_backendcommand/deploy arg. DreamZero defaults to it (engine_backend = ar_diffusion) and is engine-only — its model-local KV fallback was removed. Other diffusion models are byte-for-byte unchanged.KVCacheManager/BlockPool/SlidingWindowManageras a library (via a duck-typedARDiffusionRequestAdapter), with a chunk-granular sliding window (ChunkWindowSpec/ChunkWindowManager). No diffusion-local allocator.self_attnalready consumes;ChunkWindowManagerevicts out-of-window frames.session_id, window-boundary reset, independent CFG pos/neg adapters. KV parameters come from thear_diffusion_kv_configdeploy/command arg (defaults work out-of-box).Memoized gather. The resident window is frozen across a forward's denoise steps (KV is written back only after the last step), so re-gathering it every step is redundant. The engine gathers once per forward and reuses it (write-back invalidates it), building a vectorized slot index once and sharing it across all 40 layers. This keeps latency at parity and holds down memory — gathers drop 136 → 34 per run (~4×).
Test plan
pytest tests/ar_diffusion/— 59 CPU tests (slot mapping, pool write/gather, chunk-window eviction, orchestration lifecycle, engine-owned-KV guards, routing).dreamzero.yaml(H200, 1 GPU,enforce_eager, DiT step-caching on, CFG-parallel off). The engine matched bit-for-bit — which is what justified making DreamZero engine-only. Absolute latency is HW/config-specific; the delta is the meaningful figure. Perf/rollout harnesses are local dev tooling, out of this PR.Results (H200, 1 GPU, DreamZero, bf16)
Measured against the former model-local path (removed in this PR) — the validation that justified the removal.
Precision — bit-exact:
(1,16,30,44,80)max_abs_diff = 0, PSNR = ∞.mp4Generated video — former model-local path (left) | AR-Diffusion engine (right), visually identical:
Performance — at parity (mean ± σ over 5 runs/condition):
Memory — +4.6 GiB peak vs the model-local path. The pools are preallocated once and written in place; the only KV memory allocated during inference is the transient gathered window — gathered once per forward instead of re-gathered every denoise step, which keeps the overhead modest. Without memoization the per-step re-gather inflates the allocator reserve to +13.3 GiB.
Status
Phase-1 milestone for RFC #4366. Engine/KV selection is via the
engine_backend/ar_diffusion_kv_configcommand/deploy args (the former env-var stopgap is gone; DreamZero defaults to the engine). Remaining: CFG-parallel / 2-GPU validation; the fused paged kernel that removes the gather/copy entirely is Phase 2.Update (2026-07-04): performance & memory optimization stack
This update lands #4863's paged self-attention (@Acerak01-fy's commits, coordinated offline) plus the optimizations built on top of it:
fullgraph=True: CFG-parallel ×2 steady 613→611 ms/chunk, TP1 1104→1095 ms, paged memory win intact.Measured on H200 (
num_chunks=8, seed 0, per-chunk steady mean over 8 distinct DROID scenes): 624 ms/chunk — 2.69× vs NVIDIA upstream FA2, 4.24× vs the released v0.22.0 pair at the same 2-GPU CFG-parallel topology.tests/ar_diffusion: 72 passed (+8 GPU paged-vs-dense parity combos).Reproducing the numbers
The benchmark/profiling harness is kept off this PR to stay slim (per earlier review) and lives on the companion branch
bde-kv-phase1-bench— this PR's head plusbenchmarks/ar_diffusion/(PSNR/SSIM/LPIPS parity gates, KV-memory plateau profiler),examples/offline_inference/dreamzero/ar_diffusion_perf_compare.py(the per-chunk latency driver behind every number here), the long-rollout driver, the multi-GPU deploy configs used for the measurements (dreamzero_ardiffusion_{cfg2,tp2}.yaml— the PR itself ships only the singledreamzero.yaml; adjustdevices/parallel_configfor multi-GPU), an env-gated memory profiler (AR_DIFFUSION_MEM_PROFILE=<dir>: per-rank allocator checkpoints, per-component weight breakdown, allocation-history snapshots) backing the memory numbers, and a torch-profiler analysis skill. Example:Credits
Joint work of the AR-Diffusion / BDE group: @tzhouam @amy-why-3459 @TKONIY @Acerak01-fy @AbelSara @matchyc @loveysuby @yiwen101 @wsl2000 @asukaqaq-s @zwhzzz0821 @wtz2333 — co-author trailers are carried on the branch so a squash merge credits everyone.