Repository navigation
Clarify vLLM P/D cached-token usage accounting - #41
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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.
OpenAI-shaped vLLM P/D usage can pass the inclusive-denominator check while D's cached-token count reflects KV transferred from P. The skill now checks the numerator's provenance and the usage-details emission flag before calling that count P-side prefix reuse. A proven remote override can still carry a real high P-side count.
The change adds a focused tag comparison and proxy contract in
references/vllm.md, a short observability caveat, and durable behavioral cases 53/54. Scripts, normalized schemas and generic inclusive accounting are unchanged. The required deferred-token ceiling is remeasured in its existing test.Evidence and relevance gate
vLLM v0.31.0 release lists PR #54222. Tagged source shows the P-side metadata write, the D-side truthy/int gate, and the remote-count override. The v0.30.0 scheduler and output processor lack that field. The proxy must carry P's
kv_transfer_paramsto D with an integerremote_prefill_cached_tokens; missing or string values leave D's local count.The CLI default is False for
--enable-prompt-tokens-details, and the chat serving gate can omit details. Absence is unobserved, not zero. Source checked on 2026-10-07 atced6857a/db9527a4.Before this change, main's observability example mapped cached tokens generically, while its vLLM reference had no P/D usage provenance or emission caveat. This is an accounting guidance update, with no pricing or production changes.
Fresh verification output
GitHub CI validation on published head
2193834passed. The local verification output above was refreshed after resolving the main-branch conflicts.The existing deferred-budget test first failed after the added guidance. The full corpus was remeasured after integration with main
bb76302, its ceiling updated, then passed. Main → combined: 370,932 → 381,258 chars; aggregateceil(chars/4)92,733 → 95,315 tokens (+2,582). The separate per-file rounded sum is 92,747 → 95,329. The content delta remains exactly +10,326 chars; the earlier old-base estimate of +2,581 differs only because of rounding. No other test logic or budget changed.Static evaluation
plugin-eval analyzefresh-main/combined andplugin-eval compare(0.1.2): score 59/D → 59/D, no new failures. Trigger estimate 152 → 152, invoked 6,700 → 6,700, deferred per-file estimate 92,747 → 95,329 (+2,582). Existing budget/structure findings remain; these are static estimates.Behavioral evaluation and independent review
Separate blind before/after Porch sessions used frozen packages, identical prompts, no answer key, and
claude-b/claude-opus-5-5/ high. Durable prompts 53/54 and the remeasured budget test are included in this patch.A separate read-only Porch review verified the source captures and contract, confirmed two wording findings, and assessed the targeted rubric. Both findings were fixed through Porch in one correction loop. Final focused independent review found no actionable findings, confirmed both corrections, independently reproduced the corpus counts, and verified all targeted scenario criteria (4/4, 5/5 and positive control 3/3). These single-sample audit answers and tagged-source checks do not demonstrate deployed behavior, measured savings/latency, rollout readiness or complete tenant/application isolation. Backports, forks, other releases and non-chat endpoints require their own evidence.
Execution plans, primary-source captures, prompts, full outputs and deviation journals are preserved locally outside the checkout and are excluded from this PR.
Main integration on 2026-10-08
Merged pinned main
bb76302into the same feature branch with merge commit2193834, preserving history. Conflicts were limited to the eval append and deferred-budget commentary/constant. All 54 cases are preserved exactly: main's 51/52 and this PR's 53/54, with shared cases unchanged. Gemini guidance matches main byte-for-byte; vLLM and observability guidance match the accepted PR byte-for-byte. Test logic is unchanged apart from the measured constant.A separate read-only
claude-bOpus 5.5 high Porch review independently verified the union, file scope, parent snapshots and combined corpus measurements. No repository findings remained. All AGENTS checks were rerun successfully on the combined tree. The earlier behavioral results above remain historical scenario observations; this conflict repair introduces no behavior change and does not manufacture production evidence.