{{ message }}
Repository navigation
[Bugfix][KV Offload] Restore KVCR construction through the secondary-tier factory - #58088
Merged
NickLucche merged 2 commits intoOct 7, 2026
Merged
Conversation
Accept the optional backpressure detector passed by SecondaryTierFactory and forward it to the base class. Exercise existing KVCR behavior tests through the production factory, including configured detector retention. Co-authored-by: OpenAI Codex <noreply@openai.com> Signed-off-by: Rui <1685901819@qq.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
@mkhazraee can you please review? |
Contributor
|
Adding independent two-node GB300 validation that supports this constructor fix. Reproduction and fixWith both adapter versions below, tests through the real This occurred with both default/disabled backpressure and a configured detector. Applying the same production change as this PR—accept the optional detector and forward it to the base constructor—gave 22 passing adapter/factory tests per version (20 adapter tests plus 2 factory-contract tests). The configured-detector test also checks that the resulting tier retains the detector. Two-node serving validation
TTFT in milliseconds (p50/p90 across all 10 measured requests in each run):
“Local warm” is not a pure HBM hit: it reloads one 64-token / 9 MiB block from local CPU, with zero remote delivery. Main's remote p50 was approximately 2–3% higher in these small samples; this is not a claim of exact performance parity or a measurement of this fix's overhead. Revisions and scope
Both used the same compiled vLLM installation with Python-source base Backpressure was disabled during serving; enabled-detector coverage is limited to factory tests, not congestion behavior. Typed hints were supplied directly to AsyncLLM, bypassing Dynamo routing. This covers a single-pool TP1 model, not Kimi/hybrid/TP-DCP configurations. Matching four output tokens is a correctness smoke check, not tensor-byte equality or broad model-quality validation. Validation execution and analysis were Codex-assisted. |
Contributor
thanks Or. I found the same issue and pointed to @mkhazraee recently. we will help review |
mkhazraee
approved these changes
Oct 5, 2026
Member
|
/ci run |
|
✅ @ai-yang, CI is now available for this PR.
|
|
✅ Triggered Buildkite CI #93316 for commit |
Collaborator
CI selector (shadow): no narrower answer, every step runs: 194 test steps (270 jobs) instead of 62 (78 jobs)Shadow mode: this changes nothing about what CI runs. It shows what the evidence-based selector would pick for this PR, next to today's rules. How it works. Feedback welcome: reply here if it would skip a step this change needs, or runs something unrelated. Why: fail-open: vllm/v1/kv_offload/tiering/kvcr/manager.py holds an unmodeled dynamic import; what it loads is unknowable, running everything Selector would run (194)
Would skip (today's rules run them) (0)none Would add (today's rules do not run them) (132)
AMD mirrors: would skip (0)none AMD mirrors: would add (108)
2 changed files · base |
NickLucche
approved these changes
Oct 7, 2026
michael-gpu
pushed a commit
to practical-inference/vllm-ampere-extended
that referenced
this pull request
Oct 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fixes #58087.
A standard
type="kvcr"secondary tier fails to initialize becauseSecondaryTierFactorypassesbackpressure_detector=Noneeven when backpressure is not configured, whileKVCRSecondaryTierManagerdoes not accept that argument. Consequently, the server exits before it can serve a request.Accept the optional detector in the KVCR constructor and forward it to
SecondaryTierManager. Keep the parameter at the end of the existing signature to preserve existing positional arguments. Route the existing KVCR test helper through the production factory and exercise request/load behavior with both default and explicitly configured backpressure.This restores the constructor contract introduced by #50045 after the KVCR adapter landed in #53624. Default configurations retain a
Nonedetector; explicitly configured detectors are preserved rather than silently discarded.Duplicate checks on 2026-09-22 found no existing issue or PR addressing this constructor mismatch. #57500 concerns buffer lifetime after a native startup failure and leaves this signature unchanged.
Test Plan
Run the focused suites with the optional KVCR package installed:
CUDA_VISIBLE_DEVICES="" .venv/bin/python -m pytest \ tests/v1/kv_offload/tiering/test_kvcr_tier.py \ tests/v1/kv_offload/tiering/test_factory.py \ tests/v1/kv_offload/tiering/test_backpressure.py \ tests/v1/kv_offload/tiering/test_tiering_offloading.py \ tests/v1/kv_offload/cpu/test_manager.py \ tests/v1/kv_offload/test_factory.py -qThe adapted KVCR tests use the real factory with the existing recording backend. They cover construction, configured detector retention, request hints, key/descriptor translation, load completion, and request cleanup.
Changed-file checks:
For serving validation, repeat the accompanying issue's Qwen3-0.6B startup command on the same RTX 3090 and send three deterministic
/v1/completionsrequests. Compare their generated token IDs with the primary-only reference. Run the repository's required checks on the changed files.Test Result
The published fix is commit
ab62868f6f16b4b21ced4655150833100a07cb83. Its patch is byte-for-byte identical to the validated two-file patch.Before the production fix, on vLLM source
c723a831a81cb4ff89ea6d61b2d15109306ed1bc:TypeError.[7407, 304, 279, 3283, 315, 12095, 13, 362].Environment: KVCR
0.1.0at317e62f301300c101bf9c911e020fb89b35f1583, NIXL1.3.2, Python 3.12.14, PyTorch2.13.0+cu132, Transformers5.16.1, RTX 3090, FP16. The editable vLLM checkout uses the official cu130 native artifacts frome9f169d16b9408bb9ae44f75072b91a5521d733c; the changes between that revision and the tested source contain no C/C++/CUDA source changes. Distribution metadata still reads0.1.dev648+g1fee69838.precompiled, while Python source is thec723a831checkout plus this patch.After the fix:
git diff --checkpassed.At submission, the affected constructor, factory, base class, and KVCR tests remain unchanged at upstream
496c6472cb70d5eb8681dadae238d160f5086454. The test results above use the statedc723a831baseline plus this patch.The single-GPU serving check measures startup and generation. It does not establish cross-node transfer correctness or a broad model-quality evaluation.
OpenAI Codex assisted with the investigation, implementation, and execution of the validation reported above.