Repository navigation
[Core][Diffusion] Allocate request-scoped KV contexts for SenseNova-U1.5 B1 - #7882
MrlixiangWE wants to merge 14 commits into
Conversation
DiffusionKVRequest already describes independent request-scoped contexts, and the Worker already installs context rows, but the Scheduler rejects every request that contains one. Deduplicate shared contexts across execution sequences and reserve their native blocks in the same atomic transaction as the primary rows. Carry the cache role through Worker row resolution, reject unknown roles, and require one role per paged forward so a context cannot be consumed by the wrong attention layer. Register diffusion_kv as part of the continuous-batching module surface and cover allocation, rollback, sharing, role isolation, and native CUDA execution. Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com> Signed-off-by: MrlixiangWE <mrdanaer@gmail.com>
Keep execution-sequence requests separate from context-only native shims so sequence policies such as prefix publication and KV transfer cannot consume context allocations. Preserve reverse allocation order during request teardown and cover the ownership boundary in the manager regression. Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com> Signed-off-by: MrlixiangWE <mrdanaer@gmail.com>
|
This PR appears to belong to: docs/design/module/diffusion/continuous_batching.md, docs/design/module/diffusion/index.md. Module owners: @Isotr0py @princepride @david6666666 Routing: @Isotr0py via module of the changed files, module named in the PR description, semantic router; @princepride via module of the changed files, module named in the PR description, semantic router; @david6666666 via module of the changed files, module named in the PR description, CODEOWNERS @MrlixiangWE, 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. |
|
Self-reviewed on #7719's prefix publication still sees only primary sequences, while the A800 CUDA test compares primary and With vLLM 0.30.0, the Diffusion KV suite reports 335 passed and 2 deselected, the Scheduler suite 135 passed, and native CUDA 2 passed; model integration, two-request parity, and aggregate throughput stay in the following B1 slice. |
|
I exercised request-scoped context ownership on head |
Reject public requests whose sequence and deduplicated context rows exceed Worker adapter capacity before any native block reservation. Exercise legal context identities through native CUDA attention against the SDPA reference. Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com> Signed-off-by: MrlixiangWE <mrdanaer@gmail.com>
Exercise request-scoped context ownership across waiting and running aborts, preemption, capacity backpressure, and request-ID reuse through the Scheduler entry point. Co-authored-by: levius <2114377220@qq.com> Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com> Signed-off-by: MrlixiangWE <mrdanaer@gmail.com>
ce4a1be to
07f839e
Compare
Folded all five lifecycle cases into |
Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com> Signed-off-by: MrlixiangWE <mrdanaer@gmail.com>
Resolve the manager conflict with request paged prefix caching (vllm-project#7719): request-scoped contexts keep their own native reservations outside the prefix lookup and publication path. Test configs that build a paged_scheduler manager directly (native prefetch, HunyuanImage3 prefix lookup) now pass diffusion_kv_max_rows_per_request, which paged_scheduler configs already require. Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com> Signed-off-by: MrlixiangWE <mrdanaer@gmail.com>
|
@shihongzhi could you also take a look at this one when you have time? It is the scheduler side of B1: request-scoped KV contexts in the diffusion scheduler. Thanks! |
|
LGTM The core design is solid — I verified the transactional allocation/rollback path, the prefix-cache isolation (contexts never go through Two non-blocking nits, happy to see them in a follow-up:
|
…eir prefix free_request now releases request-scoped context shims through free_kv_blocks_in_physical_order, the same path as rollback, and a context shim always skips prefix-cache reads regardless of the hashes it carries. Co-authored-by: shihongzhi <318457+shihongzhi@users.noreply.github.com> Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com> Signed-off-by: MrlixiangWE <mrdanaer@gmail.com>
|
@shihongzhi Both done in 4c43ab0: |
|
LGTM ~ |
…ission bound test Nothing covered the context term of the empty-pool bound: without it a request whose sequences fit an empty pool but whose sequences and contexts do not gets None back instead of an admission error, and the scheduler leaves it at the head of the waiting queue on every tick. A manager test and a scheduler test now pin both the error and the queue moving on. The fail-fast hint names contexts as well as sequences. Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com> Signed-off-by: MrlixiangWE <mrdanaer@gmail.com>
Omni ReviewBot: no human activity for 7 days@MrlixiangWE this pull request has had no human commit, comment or review since 2026-09-26. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
| raise ValueError("Diffusion KV allocation requires at least one sequence") | ||
| contexts, sequence_context_ids = self._collect_contexts(requests) | ||
| num_rows = len(requests) + len(contexts) | ||
| if num_rows > self.max_rows_per_request: |
There was a problem hiding this comment.
Could admission account for aggregate Worker row capacity, not just the per-request limit? With max_num_seqs=4, two requests needing three rows each pass max_rows_per_request=4, but together exceed the Worker’s four-row pool and cause installation to fail.
Please enforce a global row budget or size the pool for concurrent demand?
There was a problem hiding this comment.
The factory already sizes the native Worker pool as max_num_seqs * diffusion_kv_max_rows_per_request (16 rows for 4 × 4), so the two three-row requests in your example take 6 of 16 rows. Worker rows are installed only for scheduled requests, at most max_num_seqs of them and each within diffusion_kv_max_rows_per_request, so installation cannot exceed the pool; a request larger than either limit fails admission before allocating blocks.
I first added an aggregate count in 77d733c, but it also counted prefetched reservations, which hold blocks and no rows, so at max_num_seqs=1 it refused every native prefetch while a request was running. 410f0ab removes it. At 2227443 with vLLM 0.32.0 the KV, Scheduler and HunyuanImage3 KV-request suites pass (548 passed) and the diffusion_kv CUDA L4 selection passes (4 passed); restoring the aggregate count turns 8 prefetch and manager tests red.
Pass the native Worker row capacity to the Scheduler KV manager and count all live sequence and context reservations before allocating any blocks. Temporary pressure waits for released rows; a request larger than the entire pool fails admission instead of waiting indefinitely. Cover deduplicated contexts, concurrent requests, release and retry, prefetch backpressure, and cancelled transfers that retain their rows until receive and Worker cleanup complete. Keep prefetch fixtures aligned with the factory's request-count times per-request-row capacity. Co-authored-by: Lancer <maruixiang6688@gmail.com> Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com> Signed-off-by: MrlixiangWE <mrdanaer@gmail.com>
Include the current native allocation attempt in rollback, even if it changes block ownership before raising. Cover all primary/context failure positions, restored capacity and reuse of the same public request id. Narrow native prefetch dependencies and type the BlockTables test contract. Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com> Signed-off-by: MrlixiangWE <mrdanaer@gmail.com>
|
@hsliuustc0106 Ready for review. |
Omni ReviewBot routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 600s (try 3 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; falling back to direct/cursor/auto). |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
Changes since the previous review
- 0 new inline finding(s); 0 finding(s) below.
CI at
65258877f040(2026-10-10T13:31:41.851073+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Note: The assigned review arm
strict/cursor/cursor-grok-4.6-highcould not complete this review, so it was produced by the fallback armdirect/cursor/auto. It is excluded from the routing experiment.
Full review analysis
PR description
This change turns request-scoped diffusion KV contexts into real Scheduler and Worker rows for the paged path. Shared contexts are deduplicated, then primary sequences and context rows are reserved in one transaction: a native allocation failure rolls back every attempted row, and prefix-cache pins stay until that rollback finishes. Admission rejects a request that can never fit the per-request row limit, the Worker row pool, or an empty block pool, and it waits when live or prefetched reservations already consume the row pool. Context shims stay out of prefix publication and KV transfer, and Worker installation plus attention dispatch reject an unregistered role, mixed roles in one forward, and a layer that reads another role's rows. The SenseNova model consumer is not in this diff.
Change flow
flowchart LR
Req["[EXISTING] Public request with kv_contexts"]:::existing
Sched["[CHANGED] Scheduler admission and row budget"]:::changed
Mgr["[CHANGED] KV manager transactional reserve"]:::changed
Shim["[NEW] Context-only native shim"]:::new
Worker["[CHANGED] Worker install and role dispatch"]:::changed
Req --> Sched --> Mgr --> Shim
Mgr --> Worker
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
Omni ReviewBot: no human activity for 13 days@MrlixiangWE this pull request has had no human commit, comment or review since 2026-10-04. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Keep the request-context GPU test under main's module-level hardware marks and run tests/diffusion/diffusion_kv/ in the FlashAttention L4 step next to main's chunked-transfer test. Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com> Signed-off-by: MrlixiangWE <mrdanaer@gmail.com>
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com> Signed-off-by: MrlixiangWE <mrdanaer@gmail.com>
|
@hsliuustc0106 this pr have resove all conflect. could you help in review and merge? |
… rows The aggregate row check from 77d733c counted every live reservation, including a prefetched or draining one, against the Worker row pool. Native prefetch needs max_num_seqs=1, where the pool holds exactly one request's rows, so the check refused every prefetch while a request was running. Worker rows are installed only for scheduled requests, at most max_num_seqs of them and each within max_rows_per_request, and the factory sizes the pool as that product; prefetched and draining reservations hold blocks only. Drop the aggregate check, keep the per-request limits, and test that prefetch is reserved beside a running request in a two-row pool. Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com> Signed-off-by: MrlixiangWE <mrdanaer@gmail.com>
Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com> Signed-off-by: MrlixiangWE <mrdanaer@gmail.com>
0z5a
left a comment
There was a problem hiding this comment.
Reviewed head: 2227443 (code reading plus surrounding Scheduler/Worker/engine paths; nothing run).
Thanks @MrlixiangWE, this is a careful slice. I read it against the cache-ownership work in #8264. I checked these and found no issues:
- Attempted rows are appended before
allocate_slots, so rollback covers a native mutation that raises. - Contexts never reach
get_computed_blocks, anddelay_cache_blocks=Truekeeps them out of prefix publication. - Context blocks are counted in the empty-pool bound.
- The request-ID reuse and drain guards hold:
_add_request_with_request_idrejects IDs still in_kv_draining_requests.
My main point is an ownership-contract question for the AR→DiT side (item 2). The rest are smaller. None of them repeat the RuixiangMa or shihongzhi threads.
1. [Low] The Worker-row invariant that replaced the aggregate gate is held only by FIFO ordering of the waiting queue. manager.py:307-311, base_scheduler.py:538
The 410f0ab comment says Worker rows are held only by scheduled requests. preempt_request keeps a request's reservation and its installed Worker rows, frees its _running slot, and puts it at the head of _waiting with appendleft. The request then resumes through scheduled_cached_reqs with the same generation.
Today that is safe only because schedule() admits the head of the queue first and breaks when the head cannot be scheduled. So a new request can never take a preempted request's slot while that request's rows are still installed. If anyone later reorders the queue (priority, skip-ahead, or a deferral that also covers already-reserved requests), the row pool overflows. For example, with max_num_seqs=1 and 2 rows per request, the Worker would raise "only 0 rows are free".
Could the comment at L307 name this dependency? Please also extend test_request_scoped_context_preemption_reuses_allocation so it adds a second request while the first is preempted and asserts that the preempted one is re-admitted first. An assertion in schedule() would also work: installed rows (running plus preempted) never exceed the pool.
2. [Medium, question] AR→DiT ownership: contexts can never be transfer destinations, but nothing rejects a request that expects one. base_scheduler.py:708, kv_connector.py:344, base_scheduler.py:394/442/636
All of the transfer paths cover sequences only:
prepare_kv_requestsstampskv_transfer_paramson sequences only.commit_kv_loadregisters onlystate.diffusion_kv_requests._loading_sequence_ids,update_kv_connector_outputand_release_request_kvall track sequence IDs only.
So a remote-prefill request that also carries kv_contexts gets context rows over freshly allocated blocks, which may hold stale data from earlier requests. Nothing loads those rows, and nothing marks them unwritten.
If the B1 consumer expects the ar_decode context to come from the AR stage, it will read garbage silently. Please choose one of these and pin it with a test:
- Document that contexts are always produced in-stage by the DiT Worker, and reject
kv_transfer_params+kv_contextsat admission withDiffusionKVAdmissionErrorfor now. - Extend those four paths to context shims before the consumer slice lands.
Related question, also for the consumer slice. A shared context is deduplicated to one row. prepare_batch rejects the same row twice in one forward, and every forward writes K/V through slot_mapping. Which forward is the single writer of a shared context, and are CFG branches expected to read it in separate forwards? A sentence in the DiffusionKVContext docstring would make that contract explicit.
3. [Low-Medium] Error taxonomy on reservation failure. base_scheduler.py:215, base_scheduler.py:368, manager.py:216/263/266
except Exception maps every reservation failure to FINISHED_ERROR and returns str(exc) to the client. That covers request-caused errors, but also invariant violations:
- "internal request IDs are already allocated" means bookkeeping leaked.
- A native allocator exception after rollback is also an invariant failure.
Two related problems:
- The conflicting-context
ValueErrorat L216 is request-caused but is not aDiffusionKVAdmissionError. - The prefetch path at L368 drops every exception at debug level.
So a leak shows up as a stream of per-request user errors with no engine-level signal. I'd suggest three outcomes:
- Request-caused failures raise
DiffusionKVAdmissionError, and the request finishes with an error. - Capacity returns
None, and the request retries. - Anything else, after rollback, is logged with
logger.exceptionand escalated through_fail_engine. Prefetch should log non-admission failures at warning or above.
4. [Test gap] Native prefetch with contexts. tests/diffusion/diffusion_kv/test_native_prefetch.py
_request() never carries kv_contexts, so the prefetch→admit and abort-during-load→drain→release_kv_drains paths are untested with context shims. Two cases would close this:
- A prefetched B with one context is admitted and reuses the same generation, including
metadata.contexts. - B is aborted while loading. Its context blocks must stay held until
release_kv_drains, then return the pool to its baseline.
5. [Nit] manager.py:245
The num_rows > self.max_num_rows check can't fire. max_num_rows comes from kv_vllm_config.scheduler_config.max_num_seqs, which vllm_config.py:272 sets to max_num_seqs * max_rows_per_request, so it is always at least max_rows_per_request. Please either assert max_num_rows % max_rows_per_request == 0 at construction, mirroring model_runner_backend.py:255, or drop the check. Its "Worker capacity" message suggests an aggregate gate that no longer exists.
Happy to re-check once item 2 is settled. I'd like the admission guard in this PR; the rest can be follow-ups.
|
Thanks @0z5a. I'll keep this PR as it is; per item:
|
Purpose
Part of #7679: add the generic Scheduler/Worker path for request-scoped KV contexts in SenseNova-U1.5 B1. The model consumer and its two-request parity and throughput validation remain the following B1 slice.
Propose
max_num_seqs * diffusion_kv_max_rows_per_request, so prefetched and draining reservations, which hold blocks only, do not count against it.Test Plan
vLLM Version: 0.32.0.
vLLM-Omni Commit:
2227443(410f0abmerged with main3266a6f).Test Result
Executed on NVIDIA A800 80GB with each worktree's import path verified.
not cudaselectordiffusion_kvCUDA L4 selection, including primary andar_decodecontext rows against SDPAmanager.pyThe native allocation regression injects an exception after real allocation at each of two primary and two context positions. All four fail before the rollback fix and pass after it, including empty ownership tables, restored free capacity and successful retry with the same request id. The
not cudarun is performed on a GPU host; it includes an existing graph test without the CUDA marker.