Repository navigation
Conversation
6ae80a6 to
71fbca5
Compare
71fbca5 to
a66ca47
Compare
WalkthroughThe KV router now propagates eligible-worker cache counts, tracks bounded canonical cache history, records cache-loss metrics through request lifecycles, and attaches validated cache counters to final vLLM generation chunks. ChangesKV cache-loss accounting
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR adds bounded cache-loss telemetry, but the current implementation still performs the feature’s memory and hashing work by default and serializes prompt hashing on a shared routing lock; incomplete worker payloads and capacity-limited multi-output history can also produce missing or nondeterministic telemetry. These are concrete runtime and correctness issues, so merge should wait for fixes or explicit owner acceptance. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
318a5d4 to
39ab0ae
Compare
|
Two labels these series need:
Number of series: × phases present (1 aggregated, 2 disaggregated) × served models — the same shape as the existing — Neelay + 🤖 |
|
Suggestion for a follow-up (or here if cheap): record F4/F5 in all routing modes, not just KV. F2/F3 genuinely need the KV index — non-KV modes never hash the prompt or query it, so "best eligible cached prefix" has no meaning there. But F4/F5 come from the worker and don't depend on how it was chosen; the guard sees the final chunk in every mode, yet today Why it's worth it: F5 under round-robin versus F5 under KV routing is the direct measurement of what KV routing buys — the most useful comparison this family could enable, and impossible today. It changes F5's population relative to F3's, so raising it rather than asking for it. @jh-nv — Neelay + 🤖 |
|
In reply to your comment:
|
|
In reply to your comment:
|
|
In reply to your comment:
|
|
In reply to your comment:
|
|
/ok to test 519c001 |
nnshah1
left a comment
There was a problem hiding this comment.
Full pass over the current head (57cef28e3) — built it, ran clippy with -D warnings and the PR's Rust + Python tests locally (all 23 pass; clippy fails only on new_kv). Substantive points are inline; the small items are grouped here as one thread.
[before merge — names and tests; the rest are suggestions]
Tests
metrics.rs:1680-1682—assert!(!output.contains("cache_loss")),"funnel","stage="assert the absence of strings that don't appear anywhere in the code. Delete.request_guard.rs:1041-1042— the comment oncounter_overflow_marks_the_observation_incompletedescribes revision history;githas it. Delete.kv_cache_hit_metrics_export_counters_with_matching_input_labelsis two tests (kv counters;input_sequence_tokenslabels) in one 50-line body — split or trim.- 22 tests for ~250 lines of logic. Overlaps:
worker_outcomes_can_exceed_router_observationsandworker_values_may_exceed_router_estimate_and_prompt_lengthare the same assertion with different numbers;successful_stream_emits_the_latched_hit/stream_error_discards_a_buffered_hitrestate what the e2ecomplete/cancelledtests prove through the real guard;raw_overlap_is_specific_to_the_selected_dp_ranktests three-map addition. Suggested keep set: parse rejection, overflow, the three e2e attempt tests,completion_keeps_selection_phase, one selector max test, andtest_kv_cache_hit_engine_data_marks_missing_worker_counters_incomplete.
Naming
- "worker-stage" is the old name for what is now the kv cache-hit report:
protocols.rs:1093,types.rs:156,new_with_worker_stage_tracking/track_worker_stages(selector/mod.rs:158-162),mod worker_stage_testsand three test names. new_with_worker_stage_trackingexists for one test (mod.rs:672); that test can setrequest.mode = ScheduleMode::Tracked { .. }like the test above it. Delete the constructor.selector/mod.rs:141-145— the comment says "preserves F2", which is defined nowhere in the code. One line does it: "max over the same eligible setrow()already visits; no second traversal."KV_PHASE_LABEL's doc comment says it matches the stage metrics' label; those use a literal"phase". Either promote toprometheus_names::labels::PHASEand use it in both places, or drop the comment.
Shapes
observe_kv_worker_hit(&self, phase, model, (lookup, reused): (Option<u64>, u64))destructures a tuple in the parameter list, andkv_worker_hit_forreturnsOption<(Option<u64>, u64)>. Pass and returnWorkerReport— the enum exists for exactly this. (Moot for the innerOptionif the lookup deferral is taken.)kv_tokens_counterusesunwrap_or_else(|_| panic!(..)); every sibling uses.expect(..).kv.rs:416-428—if !is_query_only { ..zip..map.. } else { None }→(!is_query_only).then(|| ..).flatten(), or gatekv_routeonce whereis_query_onlyis decided.
— Neelay + 🤖
|
[blocking] The Separately, CI on this head is red only on — Neelay + 🤖 |
|
In reply to the grouped review, addressed in e0cc7a9:
Also addressed the merge-conflict comment: merged main at 4948984 and preserved its CacheSnapshot/builtin-policy refactor while retaining raw-token telemetry. Validation: 22 focused Rust tests and five AST-isolated Python cases pass; clippy passes for both affected Rust crates with all targets and --no-default-features. Formatting, docs lint, and Fern configuration pass. Fern broken-links still reports the unrelated recipe link already present on main. Full GPU/backend CI needs a maintainer to approve this head. |
nnshah1
left a comment
There was a problem hiding this comment.
Approving on e0cc7a94d. Both review rounds are addressed: kv_* naming with phase/model labels, local_/external_* wire keys, Option<KvHitTracking> on the guard, selected_raw_cached_tokens produced by selection_result, one shared metrics build path, PreprocessedRequest::phase(), and the vLLM adapter reading only RequestOutput.num_cached_tokens with the catalog stating no stock backend populates the lookup series yet. Merge conflict with #14785 resolved; clippy and pre-merge are green on this head.
— Neelay + 🤖
|
Ready for another review of @ai-dynamo/dynamo-backend-vllm-codeowners @ai-dynamo/dynamo-rl-codeowners @ai-dynamo/dynamo-docs-codeowners — GitHub still lists these codeowner review requests as outstanding. Could you review the current head? Since that approval: deferred lookup/tier/outcome telemetry; reduced the worker payload to Current-head Pre Merge passes, including Rust tests and Clippy. The description now also records the two-worker GPU smoke and shadow/dashboard checks, explicitly distinguishing the combined F1 integration build and compatibility backport from standalone PR validation. Full upstream backend/GPU coverage remains separate. The previous approval was on |
|
Updated the funnel docs in b5212f5: a plain-English F0–F4 table with metric names, enablement, and the longer-window timing note. Removed implementation details from the page; the PR description retains the review context. F1 uses the renamed metric in #14889. Docs lint and Fern configuration pass; the local broken-links check still reports the unrelated DeepSeek recipe link. |
Refresh against main and preserve projected scheduling loads and output-block tracking. Signed-off-by: Chanh Nguyen <chanhnguyen@gmail.com>
|
Refreshed against main in a5fe0fe and resolved the scheduler/request-guard conflicts. Preserved the new projected-load and output-block tracking paths, plus the existing F2–F4 semantics and simplified docs. Formatting and diff checks pass locally; full CI is running: https://github.com/ai-dynamo/dynamo/actions/runs/36813596696 |
Allow the nested cache-reuse report in the response dictionary without inferring only integer/list values. Signed-off-by: Chanh Nguyen <chanhnguyen@gmail.com>
|
Fixed the full-CI mypy failure in 465ce8f: explicitly typed the heterogeneous response dictionary so the nested cache-reuse report is accepted. This is a one-line typing-only change. The isolated mypy reproduction passes; fresh full CI: https://github.com/ai-dynamo/dynamo/actions/runs/36816830163 |
✅ Dynamo PR CI passed — run 36816830163 (attempt 1) on
|
| Framework | Build | 1-GPU amd64 | 1-GPU arm64 | Multi-GPU amd64 | Deploy | Snapshot |
|---|---|---|---|---|---|---|
| vLLM | ✅ 6 | ✅ 1 | ✅ 1 | ✅ 1 | ✅ 4 | ✅ 4 |
| SGLang | ✅ 3 | ✅ 1 | ✅ 1 | ✅ 1 | ⏭️ | ⏭️ |
| TRT-LLM | ✅ 3 | ✅ 1 | ✅ 1 | ✅ 1 | ⏭️ | ⏭️ |
| Other | Jobs |
|---|---|
| changed-files | ✅ 1 |
| deploy-operator | ✅ 1 |
| dynamo-runtime | ✅ 8 |
| frontend | ✅ 3 |
| frontend (amd64) | ✅ 1 |
| frontend (arm64) | ✅ 1 |
| Operator | ✅ 1 |
| planner | ✅ 5 |
| triton-runtime | ✅ 2 |
⏭️ 7 other components not run (skipped by change detection or an upstream result)
allure-report, DGDR Deploy Test, dynamo-sidecar, Helm Chart Tests, Operator Integration, Power Agent, sidecar-runtime
Posted automatically by Devin for run 36816830163. Updated on every full-CI run of this PR.
PeaBrane
left a comment
There was a problem hiding this comment.
Superseded by #15443, which carries this PR's two commits unchanged (authorship preserved via cherry-pick -x) plus SGLang and TensorRT-LLM producers, and drops the report from disaggregated decode workers, whose engines count transferred prefill KV as cached. Please hold this one so the two don't merge separately. Review feedback for the router consumer is welcome on #15443.
|
Closing as superseded by #15443, which incorporates this PR’s router cache-reuse telemetry with authorship preserved and consolidates the backend reporting changes. Further review and integration will continue there. The replacement PR is still open; this closure does not indicate it has merged. |
Summary
Adds always-on cache-reuse counters for each KV-routing attempt: raw cached prefix on the best eligible worker (F2), raw cached prefix on the selected worker/rank (F3), and worker-reported reused tokens (F4, formerly F5). Routing decisions and tier weights are unchanged.
Start review in
request_guard.rs,metrics.rs, and the vLLM generation/prefill response paths inhandlers.py.Metrics and compatibility
All names below have the
dynamo_component_router_prefix and carryphase(aggregated,prefill, ordecode) andmodellabels.input_sequence_tokens_sum(existing histogram sum)kv_best_eligible_cached_prefix_tokens_totalkv_selected_cached_prefix_tokens_totalkv_worker_reused_tokens_totalengine_data.kv_cache_hitobject contains onlyprompt_tokensandreused_tokens. Unknown fields are accepted, so optional tier/lookup data can be added later. The payload is new in this PR; no shipped wire key changes.RequestOutput.num_cached_tokens. We retain a separate per-attempt report because disaggregated decode overwrites completion usage with prefill usage for client accounting.Interpretation
router_kv_hit_rate. Values are uncapped.Related issues
F1 history: #14889. Incorporates the request-metrics structure from Neelay's reference draft #15173.
Validation
dynamo-llm, all targets, with--no-default-features -- -D warnings.RequestOutputclass: five payload cases, three dedicated-prefill cases, and one generation/KV-transfer case. Surrounding backend dependencies are mocked; the full vLLM pytest suite was not run locally. GPU integration checks are described below.git diff --checkpass. Direct Flake8 passes with existing W503 findings excluded. The pre-commit wrapper could not fetch its isort environment because GitHub git transport returned HTTP 503.51b78ad5b5, including Rust tests, Clippy, pre-commit, and docs checks. This is not a claim that the full backend/GPU matrix ran.