Repository navigation
Conversation
github-actions
Bot
added
documentation
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a diagnostic tracing framework for KV block lifecycles to help identify and debug KV corruption issues. By correlating events across the scheduler, worker, and transfer paths, it allows for detailed inspection of block ownership and access patterns. The feature is opt-in and designed to minimize impact on production performance while providing the necessary evidence for root cause analysis. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Suggested PR Title:
[Ops][Feature] Opt-in KV block lifecycle tracing for AscendSuggested PR Summary:
### What this PR does / why we need it?
This PR introduces an opt-in KV block lifecycle tracing feature (`VLLM_ASCEND_KV_TRACE`) for Ascend. It allows recording and querying KV block lifetimes, request mappings, transfers, and selected forward-window byte changes to diagnose silent KV corruption, stale slot mappings, block reuse, and PD request correlation.
Key changes include:
- Core tracing logic and configuration parsing in `vllm_ascend/debug/kv_trace.py` and `kv_trace_manager.py`.
- Integration with Mooncake connector and worker model runner to capture transfer and execution events.
- A CLI tool `tools/kv_block_trace.py` for reading and analyzing trace logs.
- Extensive documentation and design specs.
- Comprehensive unit tests covering the tracing logic.
Feedback:
A critical bug was identified in `vllm_ascend/worker/model_runner_v1.py` where `self._kv_trace_caches` is assigned a list directly instead of a dictionary mapping layer names to cache tensors, which would cause an `AttributeError` when tracing is enabled. A code suggestion has been provided to construct the dictionary properly.
### Does this PR introduce _any_ user-facing change?
Yes, it introduces a new opt-in environment variable `VLLM_ASCEND_KV_TRACE` to enable KV block lifecycle tracing, along with a CLI tool `tools/kv_block_trace.py` for log analysis.
### How was this patch tested?
The patch includes new unit tests in `tests/ut/debug/test_kv_trace.py` and `tests/ut/debug/test_kv_trace_relations.py` which can be run on CPU. Real environment validation was also performed on Ascend 910B4 as documented in the validation reports.| if self._kv_trace is not None: | ||
| self._kv_trace_caches = kv_caches |
There was a problem hiding this comment.
In initialize_kv_cache_tensors, kv_caches is returned as a list of cache tensors (or tuples of tensors) from _reshape_kv_cache_tensors. However, kv_trace.py expects runner._kv_trace_caches to be a dictionary mapping layer names to their corresponding cache tensors (calling .get(layer_name) on it). Assigning the list directly to self._kv_trace_caches will cause an AttributeError: 'list' object has no attribute 'get' when tracing is enabled. We should construct a dictionary mapping each layer name to its corresponding cache tensor in order.
| if self._kv_trace is not None: | |
| self._kv_trace_caches = kv_caches | |
| if self._kv_trace is not None: | |
| self._kv_trace_caches = {} | |
| layer_idx = 0 | |
| for group in kv_cache_config.kv_cache_groups: | |
| for name in group.layer_names: | |
| if layer_idx < len(kv_caches): | |
| self._kv_trace_caches[name] = kv_caches[layer_idx] | |
| layer_idx += 1 |
github-actions
Bot
added
merge-conflicts
and removed
documentation
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Tjh-UKN
changed the title
Tjh-UKN
changed the title
6e71cfb to
7db943c
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Tjh-UKN
changed the title
cb7f26d to
b4d38b1
Compare
Tjh-UKN
changed the title
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
Bot
added
documentation
1a6906e to
55b60a9
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
| ) | ||
| from vllm_ascend.compilation.breakable_aclgraph import BreakableACLGraphWrapper | ||
| from vllm_ascend.core.kv_cache_interface import is_circular_kv_cache_spec | ||
| from vllm_ascend.debug.kv_guard_runtime import install_guard |
There was a problem hiding this comment.
MRV1 is deprecated. You don't need to change MRV1
| key, value, block_size, block_table, actual_seq_lengths_kv = self._get_fia_params( | ||
| key, value, attn_metadata, kv_cache | ||
| ) | ||
| guard = getattr(self, "_kv_guard", None) |
There was a problem hiding this comment.
Lets check whether it's necessary to add guards in attention forward.
| @@ -0,0 +1,182 @@ | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
What's your future plan to remove this patch? Do you have any upstream vLLM RFCs or PRs?
Tjh-UKN
changed the title
Tjh-UKN
changed the title
| @@ -0,0 +1,3 @@ | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
| # SPDX-FileCopyrightText: Copyright contributors to the vLLM project | |||
| """Opt-in diagnostics. Importing this package does not enable instrumentation.""" | |||
There was a problem hiding this comment.
I suggest that change the directory name to kv_trace
Tjh-UKN
changed the title
Consolidate the final KV Trace changes onto current main so CI can rebase without replaying obsolete KV guard commits. Signed-off-by: TAJh <taojiaheng1@huawei.com>
3fdd8f6 to
b44bdef
Compare
What this PR does / why we need it?
Add opt-in KV Trace address-plan monitoring in Model Runner V2. Consume native
SchedulerOutputrequest/block deltas at runner input and compare them with final per-group slot mappings and block tables. No Scheduler, allocator, BlockPool, or per-attention-layer patch is installed. Model Runner V1 has zero diff against the PR base.kv_trace/kv_trace_config.pyadditional_config.kv_trace.enabled/action.kv_trace/kv_trace_runner.pykv_trace/kv_trace_state.pykv_trace/kv_trace_plan.pykv_trace/kv_trace_runtime.pykv_trace/kv_trace_connector.pyworker/v2/model_runner.pyRequest bindings are immutable step-local snapshots. Finished/preempted request messages remove local references; this is not a claim of physical block allocation/free visibility. Public connector metadata/results are projected without wrapping transport internals. The existing Mooncake failed-receive result fix is retained.
Does this PR introduce any user-facing change?
Enable per engine:
vllm serve MODEL --additional-config '{"kv_trace":{"enabled":true,"action":"log"}}'enableddefaults to false.actionislogorraise. Normal INFO records includekv_trace.bind,kv_trace.write,kv_trace.read, and boundary records. Onlykv_trace.violationlatches a raised failure; missing observations are debug-level skipped checks and do not become coverage-gap errors.The check compares declared execution metadata. It does not prove kernel memory accesses, KV numerical correctness, allocation lifetime, or network transfer integrity, and cannot undo submitted device writes. Existing restrictions on CP, DBO, speculation, encoder/decoder and unadapted cache semantics remain. Connector support is limited to the existing Ascend Mooncake adapter.
How was this patch tested?
Package-only follow-up:
39bd3f159ae39f99ffe28b39c80d33293adb8aa0moves the feature fromvllm_ascend/debug/tovllm_ascend/kv_trace/, moves its focused tests totests/ut/kv_trace/, and updates imports and documentation. Configuration remainsadditional_config.kv_trace. The 60 focused tests and Linuxbash format.sh cipass; Mypy passes for Python 3.10/3.11/3.12. AST comparison confirms unchanged product logic apart from import paths/order. NPU functionality and performance numbers below belong to the explicitly pinned behavior revision; hardware benchmarks were not rerun for this package rename.NPU-validated behavior revision:
2883638529ac02c9ee1ef29a41a8158da9d0782a. Validated source tree:d3bcee94fc3968c0f29f72dba72dde08774e9fad.On 179.167.1.2 / Ascend 910B4, Qwen3-0.6B BF16, MRV2, TP1/PP1:
write_target_mismatch/read_target_mismatchlogged; log returned output, raise blocked normal output. These are controlled safe-address injections, not production-fault or numerical-correctness proof.bash format.sh cipassed on Linux, all configured hooks, with no formatter changes. Mypy passed for Python 3.10, 3.11 and 3.12.Performance validation completed on the delivery source: 8 fresh engines, 16 formal windows, 520/520 successful requests and 133,120 output tokens, all windows longer than 60 seconds. Normal INFO trace logging is included. Llama-3.1-8B BF16, MRV2 FULL, input/output 2048/256, concurrency 1/8, prefix caching disabled, same NPU. Order: OFF / ON / ON / OFF / ON / OFF / OFF / ON; four OFF and four ON engines. All valid windows are retained.
Throughput is total output tokens divided by total measured time; latency means are weighted by request count. CV is the sample coefficient of variation across four independent engine windows. These measurements do not establish a general 1% bound: throughput and first-token latency are separate outcomes. CPU/RSS sampler windows were invalid due to buffered client markers and are excluded from resource conclusions.
The local HTML report contains the stable functional results and original server logs. It excludes abandoned startup attempts. MLA, multi-rank TP/PP and actual Mooncake network transfer were not exercised in this run.
Checklist
Scheduler/allocator patch removed; Model Runner V1 restored to base.
Per-engine
additional_config.kv_traceand log/raise retained.Exact-source NPU functional validation and focused host checks passed.
Repository format checks and Python 3.10/3.11/3.12 typing checks passed.
Complete multi-run OFF/ON performance comparison and retain all formal windows.
Required online CI passes on the delivery head before merge.
vLLM main: vllm-project/vllm@24420b7