[Bugfix][KV Offloading] Isolate queued store compute events - #165
Conversation
8e0f700 to
7803a82
Compare
|
The core CUDA implementation looks correct for event isolation. One portability note: the inherited get_finished() uses torch.cuda.current_stream(), so NPU adapters must provide an accelerator-specific mapping or override this path. This is tracked in paired vllm-ascend-hust#154. |
Ascend Benchmark Result
|
CubeLander
left a comment
CubeLander
left a comment
There was a problem hiding this comment.
The per-submission event isolation is the right fix for delayed dequeue: each queued store now retains the dependency recorded for its own scheduler step. The focused regression passes locally. There is one small required-check blocker below; I also left the paired preemption-drain correctness finding on vllm-ascend-hust#154.
| """Record submissions without consuming their wait events.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| self.wait_events = [] |
There was a problem hiding this comment.
The current required pre-commit run fails here with mypy [var-annotated]: Need type annotation for "wait_events". Please give the list an explicit type, for example self.wait_events: list[object | None] = [], and rerun the exact-head pre-commit check. The event-isolation behavior itself looks sound.
Current handoff status
|
Allocate a distinct compute-completion event for each queued store so delayed dequeue cannot observe a later record on a reused event. Cover two queued steps without consuming either wait event. Signed-off-by: Shuhao Zhang (Tony) <shuhao_zhang@hust.edu.cn>
Signed-off-by: Shuhao Zhang (Tony) <5267690+ShuhaoZhangTony@users.noreply.github.com>
09dcfcc to
2521456
Compare
moonandlife
left a comment
moonandlife
left a comment
There was a problem hiding this comment.
结论：✅ benchmark通过后即可合入
评估
| 维度 | 评价 |
|---|---|
| 正确性 | 修复方式规范——每个异步提交应拥有自己的完成事件。旧代码跨步骤复用单个事件，在延迟出队时会触发数据竞争。 |
| 测试覆盖 | mock backend 精确复现了竞争条件；断言同时验证了对象身份（is not）和调用次数（assert_called_once_with）。 |
| 回归风险 | 极低。生产代码仅改 4 行，其余为测试文件。 |
| 硬件验证 | 已在真实 910B2 上通过配对验证。 |
| 项目风格 | 符合现有规范——SPDX 头、类型注解、fixture 模式。 |
|
我已检查：该 PR 只修复 simple_kv_offload store compute event 复用问题，并补了延迟队列回归测试；pre-commit、smoke、regression、ascend-benchmark 均为 green。\n\n阻塞点不是代码问题，而是分支保护要求 1 个有写权限的 approving review；当前 token 是 PR 作者，GitHub 不允许自审。请王胜或其他 maintainer approve 后即可合并。 |
4 participants
Purpose
The store path queued a reusable compute-completion event into the background copy backend. If dequeue is delayed across scheduler steps, recording that same event again can move the dependency observed by an earlier queue item.
This change allocates one event for every queued store. The new regression test submits two steps while the backend deliberately does not consume either wait event, then verifies the submissions retain distinct events recorded on the compute stream.
Paired vllm-ascend-hust current revision:
64f31cf631099a9c93abfcf4d83e63e0c85bf9cd(product code tested at7feed154b6817aa607738185bb4cf6915e14c359; the final commit adds only the evidence manifest).This does not modify or rewrite the merged vllm-ascend-hust #148 branch or its authorship.
Duplicate check: related upstream vLLM vllm-project#46278 established compute-to-store ordering, but the current open PR and issue sets contain no change that fixes reusable-event lifetime across delayed dequeue. This PR is not a duplicate.
Test Plan
Run the delayed-dequeue unit test and focused Ruff/format checks on the two changed files.
The paired exact revisions passed a real Ascend 910B2 tensor lifecycle gate: two queued D2H copies drained during concurrent shutdown, copied bytes and H2D reload matched exactly, and health, stream synchronization, thread exit, idempotent shutdown, and late-submission rejection passed. A Qwen3-0.6B smoke completed a first deterministic request and engine shutdown; forced reload was not claimed because
reset_prefix_cachecould not satisfy its precondition. See the machine-readable evidence. This is correctness/smoke evidence, not an online-serving or performance result; this PR makes no performance claim.Test Result