[BugFix] Reuse partial attention pages below their published boundary - #676
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesPartial attention cache rewind
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change enables reuse of earlier prompt prefixes from partial attention-page cache entries while preserving requested-length limits and existing rewind behavior. Current coverage and validation indicate no remaining merge-blocking risk. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Independent validation completed on four RTX PRO 6000 Blackwell GPUs at stock memory clocks, using the exact vLLM and B12X source boundary from voipmonitor/vllm:jovian-judgement-community-20260906-r27 plus PR heads #674 and #676. Tested PR head: 74cf920. Correctness evidence:
A separate cache-geometry A/B used the same patched code and serving configuration, changing only the recurrent-state page:
This performance difference is not caused by the lookup change; both arms contain this PR. It establishes an important deployment boundary: the 256-token recurrent-state geometry enables fine DFlash replay hits but must not replace the 2,048-token production default without addressing its prefill cost. With the production 2,048/2,048 split geometry, a 2,048-token exact replay has no common hybrid hit and an 8,192-token replay retains 6,144 tokens, because the recurrent group remains the limiting cache group. Conclusion: the cache lookup fix is correct and mergeable. I recommend merging this PR while keeping fine recurrent-state pages an explicit, separately qualified cache-reuse mode rather than a default launcher change. |
|
This PR is still required; it has not been replaced by #708. The shared R29 image contains its partial-attention-page lookup proof, and #708 consumes those authenticated prefixes when exporting recurrent checkpoints. Lookup and checkpoint export solve different parts of prefix reuse. I reran all 12 partial-page rewind cases in the composed serving source: they pass, including DCP1, DCP4, replicated groups, speculative rewind and divergent-prefix rejection. Keeping #676 open as the canonical merge target; image inclusion does not mean it has merged into Jovian Judgement. |
2e87294
into
local-inference-lab:dev/jovian-judgement
|
Included in Source validation: replaying all 32 R35 review heads on the pinned base exactly reproduces the released Docker's vLLM tree; all 6,870 installed tracked files match. JJ additionally preserves Luke's DS4.1 work and #734. The final composition passed 247 focused checkpoint/scheduler, sampler/warmup and native GPU tests. This is combined-source evidence, not a fresh performance or full-model qualification for this individual PR. Publication-history clarification: the individual merge linked above is in JJ's first-parent history. It replaces the receipt's archived wrapper-merge reference; GitHub's historical merge SHA may still identify that archive. See #731 for component review order and qualification limits. |
…lished boundary Preserve the reviewed source head 74cf920 and its contributor history. The first parent records the ordered serving-source composition. Whole-tree equality and installed-artifact verification are publication gates. Review: #676 Assisted-by: OpenAI Codex Signed-off-by: Martin Vit <martin@voipmonitor.org>
A 2,048-token GLM-5.3 prompt under TP4/DCP4 returned correct answers but reused zero tokens on an identical warm request. The target attention page covers 8,192 tokens and publishes a partial hash at token 2,048. Replay excludes the final prompt token, so the old lookup never examines that published hash.
Allow a matching later partial hash in the same attention page to prove an earlier prefix. Cap the returned length at the requested aligned limit, then preserve the existing EAGLE rewind. This mirrors the full-page fallback. Replicated groups and DCP1 block scaling remain unchanged.
Validation so far:
Fixed-image GPU validation:
Independent source review found no blocking defect. A fresh Astra reviewer verified the changed-file hashes and checked 13680 CPU oracle cases; the old finder disagreed in 1704 cases.
Qualification scope is VRAM-only. The pre-existing per-group connector caller omits DCP/PCP arguments, so this PR makes no live connector-transfer qualification claim.
All required local CPU, GPU correctness, pressure, and independent source-review gates passed on the public head. The exact original LP27 service was restored with its unchanged container ID, image, and configuration fingerprint. LP26 remained stopped and no candidate was promoted. A separate intermittent illegal-access failure during original-image graph startup is preserved and unresolved; this lookup patch does not claim to fix it.
Rechecked current upstream work before readiness, including #644, #654, #677, #678, #482, #519, #533, #524, #670, #674, and #675. PRs #654 and #678 change alignment semantics in the same finder but retain the old search bounded by max_partial_idx. They do not contain this later-tail proof. The public head is unchanged and GitHub reports the PR mergeable.
Summary by CodeRabbit
Bug Fixes
Tests