Repository navigation
test: add KL guards for HiCache buffer-only host memory mode - #36341
alphabetc1 wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95b67f852e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| assert ( | ||
| min_cached <= actual <= expected | ||
| ), f"{label}: expected cached_tokens in [{min_cached}, {expected}], got {actual}" |
There was a problem hiding this comment.
Require proof that a storage load-back occurred
When storage writes succeed but the buffer-only prefetch/read path is broken or disabled, evicted requests can fall back to the always-resident 512-token prefix, and this lower bound explicitly accepts that result. The subsequent KL replay also succeeds because the missing suffix is recomputed, so all three helpers can remain green without consuming any data from the file backend—the behavior this module is intended to guard. Track an aggregate storage-read/load-back signal, or require at least one request to restore more than the warm prefix.
Useful? React with 👍 / 👎.
|
/rerun-test test_unified_radix_cache_kl_buffer_only.py |
|
Results for 🚀 |
|
@alphabetc1 can we use Inkling to cover both SWA and mamba with KL=0? |
Support for buffer-only + Mamba is in progress: #36345 |
Motivation
--hicache-host-memory-mode buffer_onlyshipped in #34798 and was fixed again in#35769 with CPU unit tests only. Grepping the tree, the only non-production
references are
test/registered/unit/mem_cache/test_unified_radix_cache_unittest.pyand a benchmark script — there is no e2e test that even launches a server in
this mode, let alone a KL one. Every HiCache KL test under
test/registered/radix_cache/unified_radix_tree/runs the host tier as a cache.That matters because buffer-only is a different read path, not the same code under
a flag: completed storage fetches park as op-owned host bounces and are consumed at
prefill admission via a device alloc, a layer-gated H2D and a plain tree insert.
Nothing currently exercises it end to end.
This is the "Unified memory" line of #34899, applied to the one host memory mode
that had zero coverage.
What this adds
Two classes,
base-b/2-gpu-large,est_time=400(measured 201s + 128s):TestUnifiedFullBufferOnlyTestUnifiedSWABufferOnlyStorage is the
filebackend against a per-class tempdir. Thresholds areinherited from the same model's cache-mode test (
kl_full.py0.0025,kl_swa.py0.03) rather than calibrated here, so the claim under test is"staging KV out through the host buffer and back is no worse than keeping it in
the host tier."
Measured on 2x H200 (SM90):
The FULL number sits where the same config reads in cache mode on the same box
(7.00e-04), which is the comparison these classes exist to make. The run wrote 738
files to the file backend, so the storage path is reached rather than assumed.
TestUnifiedFullBufferOnlyalso passes the mixin's gsm8k at 0.965 (threshold 0.93).Why no bit-exact (KL == 0) variant
buffer_onlyaccepts FULL and FULL+SWA trees only — Mamba is fenced off inUnifiedRadixCache.init_hicache, andtree_componentsis derived from the modelarchitecture in
registry.py, so no server arg can drop it. The shrunken Inklingcheckpoint that the bit-exact harness in
test_unified_radix_cache_kl_hybrid_bitexact.pyrelies on is a hybrid SSM, so itcannot run in this mode at all. Lifting that fence is a feature change, not a test
change; until then these classes gate on a threshold.
What this does NOT guard
Reverting #35769 (buffer-mode load-back ownership races) leaves all six cases
green, at 9 interleaved branches and again at 16. The harness covers the read
path but does not reproduce that interleaving. This is stated in the module
docstring so a green run here is not misread as evidence that load-back ownership
is sound.
Note on the SWA gsm8k case
It is skipped, with the reasoning in its docstring. The pool cap is what makes the
KL cases reach storage at all, and it holds only ~3 gpt-oss reasoning traces, so
the mixin's
parallel=128queues: 200 questions ran past 20 minutes against 114sfor the FULL class. Cutting to 40 makes it fast but not a gate — gpt-oss answers
the 10-shot format badly enough that 7.5-20% of outputs fail to parse, and 40
questions measured 0.275 and 0.625 on the same build. The baseline
kl_swaconfig scores 0.625 at 40 questions too, below its own 0.7 threshold. The FULL
class carries the accuracy gate instead.
Happy to split the SWA accuracy coverage into a separate large-pool class if
reviewers would rather not lose it.
🤖 Generated with Claude Code
CI States
Latest PR Test (Base): ❌ Run #32866260339
Latest PR Test (Extra): ❌ Run #32866260058
Latest PR Test (AMD ROCm 7.2): ❌ Run #32866262253