Repository navigation
fix(offload): refresh state images in LMCache's LRU on every tier hit and keep the index consistent - #2668
fix(offload): refresh state images in LMCache's LRU on every tier hit and keep the index consistent#2668ZLkanyo009 wants to merge 20 commits into
Conversation
…red LMCache pool StateByteCodec bounds state images with OFFLOAD_STATE_CPU_SIZE, but the images live in the same LocalCPU hot cache as paged KV and LMCache's LRU can still evict them under KV pressure. StateOffloadIndex keeps advertising those hashes, so the next request over that prefix parks a joint load whose state leg misses, and the whole prompt is recomputed. Under a budget, hold one extra MemoryObj reference per stored image. LocalCPU only evicts objects whose ref count is 1, so only the codec's own LRU retires an image (remove, then drop the reference). A put that LocalCPU refuses (key already resident, hot cache off) keeps no reference, and a re-store of a resident image is skipped. Co-authored-by: Cursor <cursoragent@cursor.com>
🏷️ CI GuideRuns automatically on every eligible PR before approval:
Heavy model tests:
|
ZhangLirong-amd
left a comment
There was a problem hiding this comment.
Thanks for the investigation; the failure mode and the measurements are clear. My concern is with the fix itself: pinning the images turns the shared pool into a static KV/state split, and the root cause stays in place.
Root cause: state images never get an LRU refresh
In LMCache's LocalCPUBackend, every lookup refreshes the KV keys it touches: contains(key, pin=True) queues the key, and touch_cache() calls cache_policy.update_on_hit on it. State keys never go through that lookup. StateOffloadIndex answers "does the tier hold this state" on the engine side, so LMCache never learns that an image was just matched. A prefix that is hit on every turn keeps its KV at the hot end and its state image at the cold end. Under pressure, the image goes first. It is not cold; it is just never touched. That is the asymmetry described in this PR's motivation.
Pinning works around this, but it costs:
- A static split that needs manual tuning. Your own c64 data show this: 32 GiB and 96 GiB are both clearly worse than 64 GiB. The right value will change with the model, the concurrency and the pool size.
- Dead reservations. A joint load needs both legs. A pinned image whose KV has been evicted can never be used again, but it keeps its share of the reservation until the codec's own LRU retires it.
- The index can still be stale. It still does not track what LMCache holds. Pinning only stops LMCache from removing state.
Preferred fix
- Refresh state together with KV. When
StateOffloadIndexreports a tier state hit for a prefix (at lookup or admission), touch that image's key in LMCache the same way the KV keys are touched (contains(key, pin=True)+touch_cache(), orcache_policy.update_on_hit). State and KV then compete on real recency in one pool, with no fixed reservation and no split to tune.max_cache_bytescan stay as a loose cap. - Keep the index consistent when an image is evicted anyway. Either hook LMCache's eviction, or check
containsbefore parking a joint load. If the image is gone, drop it from the index and fall back to a KV-only load or a plain prefill, instead of discovering the miss after the park and recomputing the whole prompt.
Fallback, only if this needs to land first as a stopgap
If you need this merged before the refresh is done, at minimum:
- Do not make the reservation the default. With
OFFLOAD_STATE_CPU_SIZEdefaulting to 32 GiB, every state-tier deployment silently gets a hard 32 GiB reservation. Put the pinning behind an explicit opt-in switch. - Validate the budget against the pool at startup. If the state budget is at or above
LMCACHE_MAX_LOCAL_CPU_SIZE(or leaves too little for KV), refuse to start or warn loudly. Otherwise pinned images can fill the whole pool and every KV save fails silently. #2562's accuracy protocol (LMCACHE_MAX_LOCAL_CPU_SIZE=32) is exactly that case.
Please treat the LRU refresh as the main fix and the opt-in reservation as the fallback.
Replace the fixed state-tier reservation with the fix the review asked for. LMCache's LocalCPU refreshes every KV chunk a lookup touches (contains(pin=True) -> touch_cache() -> cache_policy.update_on_hit), but a state hit is answered engine-side by StateOffloadIndex, so LMCache never saw an image being used. A prefix that hit every turn kept its KV at the young end and its image at the cold end, where pool pressure evicted it first, and the index kept advertising it until a joint load parked on it, missed and recomputed the whole prompt. Refresh: - StateOffloadIndex(refresh_hits=True) queues a touch for every hash could_serve votes a hit for (the one predicate every voter goes through), throttled to one per hash per 0.5 s. - The scheduler publishes them with the state loads; the K3 connector carries them as LMCacheOffloadMetadata.state_touches, each with a unique StateTouchId so a re-touch is a fresh aggregator key. - Each worker calls StateByteCodec.touch: update_on_hit under LocalCPU's cpu_lock (no pin, so nothing to unpin and no interference with the KV lookup's keys_in_request). StateByteCodec.get and a resident re-store refresh too; get_blocking alone does not. Index consistency: - Each rank reports resident/gone on the new k3_state_touch channel; failure-dominant across TP. A gone verdict forgets the hash. - record_allocation(defer_unverified=True) holds an admission that would load a CPU image not confirmed resident in the last 5 s; the scheduler returns it to waiting like an in-flight prefix. One pass later the verdict is in: a resident image loads, a gone one has left the index and the retry plans a lower boundary (HBM checkpoint, older image, or prefill) instead of parking on a miss. Bounded by OFFLOAD_LOOKUP_DEFER_S; a connector that cannot carry touches turns the refresh off. Budget: - No reservation: images hold no extra reference, and OFFLOAD_STATE_CPU_SIZE is only a cap whose codec LRU now follows use, not store order. - The worker warns at startup when the cap is not below the LMCache pool (it bounds nothing) or exceeds half of it. Tests: tests/test_state_tier_lru_refresh.py replaces the reservation tests. Co-authored-by: Cursor <cursoragent@cursor.com>
An admission that would load a CPU state image waits one scheduling pass for a touch verdict unless the image was confirmed resident recently. With a 5 s window nearly every AgentX load was held (the next turn of a prefix usually comes well after 5 s), and at c48, where the waiting queue is nearly empty, that pass showed up as +0.3 s p50 TTFT on cache-hit requests (first-turn TTFT unchanged). LMCache's LRU can only evict an image after a pool's worth of newer entries; a 208 GiB pool at c64 turns over in about 13 minutes. 60 s stays far inside that and covers the usual store-to-next-turn gap. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@ZhangLirong-amd @valarLip Thanks for the review. I reworked the PR along the lines you suggested and pushed LRU refresh on every tier hit. Index consistency. Every touch reports resident / gone on a new Reservation removed. No extra Startup check. When the worker builds the state tier, it warns if the cap is ≥ Unit tests. Accuracy (in-process
Performance, AgentX, 224 GB CPU pool, state cap 59.6 GiB,
ITL is unchanged. For reference, the earlier runs of The confirmation window was 5 s in One heads-up: #2562 changes Could you take another look? |
ZhangLirong-amd
left a comment
There was a problem hiding this comment.
Thanks, the rework addresses the root cause: state images now get refreshed in LMCache's LRU together with their KV, evicted images leave the index, and the reservation is gone. I have two issues on 4d8798f1c.
1. The verify hold can last indefinitely (state_offload.py:260)
needs_verify measures the hold from the latest touch send, not from when the admission was first held:
sent = self._touch_sent_at.get(h)
...
return now - sent <= defer_sA held request is re-probed on every pass. Each probe goes through could_serve, which puts h back in _touch_wanted. Every 0.5 s (_TIER_TOUCH_INTERVAL_S) take_touches sends h again and resets _touch_sent_at[h], so now - sent never reaches OFFLOAD_LOOKUP_DEFER_S (2 s by default). In normal operation the verdict comes back within a step or two, so this does not show up. But the 2 s bound is meant as the fallback for the case where it doesn't come back: a dropped metadata field, a nested MultiConnector path, or a rank that never reports. In that case the request is held forever, with nothing in the logs.
Please measure the bound from the first hold of h, for example a _hold_started_at[h] set on the first True and cleared when a verdict arrives or the hash is forgotten. Resending a touch should not extend it. A test where verdicts never arrive, asserting that the admission proceeds within defer_s, would lock this in.
2. Touch completions churn the shared tombstone ring (aggregator.py:161)
KVOutputAggregator._connector_completions tombstones every connector completion (lambda _key: True) in one ring of terminal_tombstone_limit (4096). That ring is shared by k3_state_source, the store reports and k3_state_load_disposition. Each touch is a new StateTouchId(hash, round). At c64, with every voted hash re-touched every 0.5 s, that is on the order of a hundred entries per second, which cycles the ring in well under a minute. The tombstones that guard store and source completions against late duplicate reports get evicted along with them.
round already makes every touch identity unique, so a touch cannot be reported twice and needs no tombstone. Please exclude StateTouchId from tombstoning, or give touches their own completion group.
…ouches Two issues from review on 4d8798f. The admission hold for a touch verdict was bounded by OFFLOAD_LOOKUP_DEFER_S measured from the latest touch send. A held request re-probes every pass, which re-queues the touch, and take_touches re-sends it every 0.5 s, so the bound never ran out: if verdicts stopped arriving (a dropped metadata field, a connector path that does not carry them, a rank that never reports), the request was held forever with nothing logged. The bound now runs from the first hold of the hash (_hold_started_at) and is cleared by any evidence (verdict, store, completed load) or by dropping the hash. When it runs out the admission proceeds as before touches existed, later admissions over that hash are not held again until evidence arrives, the release is counted as state_offload_verify_holds_expired, and the first one logs a warning. KVOutputAggregator tombstoned every connector completion in one bounded ring shared with the state store, source and load-disposition channels. A touch is a new StateTouchId(hash, round) every time, so at c64 touches cycled the ring in under a minute and evicted the tombstones that guard stores and sources against late duplicate reports. round already makes each touch unique, so touches are no longer tombstoned. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@ZhangLirong-amd Thanks, both are real. Fixed in 1. The verify hold now has a fixed bound. Tests in
2. Touches no longer take tombstones. The All four new tests fail on Validation (same setup as last round):
The difference is within run-to-run noise: the same On merge order: this conflicts with #2562 in Could you take another look? |
…tier-reserve Co-authored-by: Cursor <cursoragent@cursor.com>
|
@ZhangLirong-amd @valarLip Merged latest Conflicts (both with #2562, both in the admission path):
Verification
|
|
Thanks for the fix. A few comments:
|
… from the pool, keep could_serve pure Review round 3 on #2668 (ZhangLirong-amd): 1. `allocate` and the admission hold (`_tier_state_unverified`) now pick the state hash through one helper, `BlockManager._state_resume_hash`: the joint boundary when there is one, none for a replay, else the HBM hit. Behaviour is unchanged (`transfer.replays` never coexists with PAGE checkpoints, and `_attach_state_slots` already dropped the hash for it). 2. The confirmation window is no longer a hard-coded 60 s. It is derived from the LMCache CPU pool (`LMCACHE_MAX_LOCAL_CPU_SIZE`, read with the chunk size in `state_tier_lmcache_sizes`): a quarter of the pool's turnover at an assumed 1 GiB/s fill ceiling (4x the AgentX c64 rate), i.e. 0.25 s per GiB, clamped to [5 s, 60 s]; 52 s for the 208.6 GiB AgentX pool, the floor when the pool is unknown. No new environment variable. 3. `StateOffloadIndex.could_serve` is a pure check again. Voters register the hits they count with the new `want_touch`: the resumable scan (`PagedStateCheckpointCoordinator._reachable`) and an offered load (`BlockManager._request_state_load`). `_tier_can_serve` stays pure. 4. `OFFLOAD_LOOKUP_DEFER_S` is read once when the index is built (only when it refreshes hits) instead of on every `needs_verify`; tests pass `defer_s`. Tests: `tests/test_state_tier_lru_refresh.py` (63, +16): could_serve queues nothing, voters register touches, the defer is read once, the window's formula and bounds and the block manager feeding it the pool size, the resume-hash helper, and an end-to-end check that the hold asks about the same hash `allocate` then loads.
|
@ZhangLirong-amd thanks for the review. All four points are addressed in c54eda2 (one commit on top of 881aacf, no new environment variable). cc @valarLip 1. Duplicated hash selection in 2. Hard-coded
I didn't add a knob because the project doesn't take new environment variables, and the pool size already determines the window. A follow-up could measure the fill rate at runtime instead of assuming a ceiling, but that change is larger than this PR needs. Tests: 3.
4. Testing
Throughput and TTFT are unchanged within noise. After wins on throughput in 3 of 4 rounds, and p50 TTFT splits 2-2: rounds 1-2 favoured before and rounds 3-4 favoured after. Recomputed input is 0.142 → 0.141. Neither side had a failed state load in any round. The 52 s window holds 5% more admissions than the old 60 s (4,823 vs 4,577 over 4 rounds), as expected for a shorter window, with no measurable cost.
The PR description is updated to match ( |
|
@valar-cop review this pr |
valar-cop
left a comment
There was a problem hiding this comment.
LGTM, minor robustness points only
any perf number? gsm8k? and LMCache interface not verified here
ci green.
| lock = getattr(backend, "cpu_lock", None) | ||
| hot = getattr(backend, "hot_cache", None) | ||
| policy = getattr(backend, "cache_policy", None) | ||
| if lock is not None and hot is not None and policy is not None: |
There was a problem hiding this comment.
the presence-only fallback silently no-ops the touch but still marks verified.. add a counter and warn once?
There was a problem hiding this comment.
@valar-cop Followed in 9219a8d and refined in the two later rounds (1b311b6, 4eff581). This reply was held back and is posted together with those rounds.
How the presence-only case works now:
- A touch returns
TouchResult:REFRESHED: the image was in LocalCPU'shot_cache, andupdate_on_hitmoved it.PRESENT: outside the hot cache but still held by another backend, e.g. LocalDisk, or LMCache lacks the LocalCPU internals; see the line-46 thread.GONE.
PRESENTis counted. The worker warns once, and each such touch reaches the engine on ak3_state_touch_refreshcompletion, failure-dominant across TP. It shows up asstate_offload_touches_unrefreshedon the existingstate checkpoints:stats line.put's skip-the-store fast path applies only toREFRESHED. A disk-only image is stored again, which puts it back into the CPU tier.
I kept the "resident" verdict for PRESENT on purpose. The verdict answers "can get read this image right now", and presence answers that correctly, because get reads every backend and promotes the image into LocalCPU. Being marked verified only lets later admissions within verified_for_s skip the one-pass hold. If the image is evicted in that window, the load misses and the request recomputes (fail_load -> forget), which is how every load behaved before touches existed. So a stale confirmation costs a recompute, never wrong state. The two alternatives are both worse:
- Reporting it gone would drop a loadable image from the index.
- Not ending the hold would re-hold every admission on that hash until
OFFLOAD_LOOKUP_DEFER_Sexpires.
In our setup (in-process LocalCPU only) presence-only never happened: the GPU smoke on 1b311b6 shows state_offload_touches_unrefreshed=0 over 757 touches. With a LocalDisk backend it does happen; numbers are in the summary.
Tests in tests/test_state_tier_lru_refresh.py:
test_a_disk_only_image_is_counted_and_warned_oncetest_a_refreshed_or_gone_image_is_not_countedtest_a_disk_only_image_is_stored_back_into_the_cpu_tier
cc @valarLip
There was a problem hiding this comment.
presence-only counting as resident is fine by me: a stale confirmation costs a recompute, never wrong state. the disk run and the real hot_cache reorder cover what i asked.
lgtm. one thing: log the derived verify window once at startup, the stats line shows the holds but not the window they run against.
| return self.offload is not None and self.offload.could_serve(h) | ||
| if self.offload is None or not self.offload.could_serve(h): | ||
| return False | ||
| self.offload.want_touch(h) |
There was a problem hiding this comment.
want_touch now a hard dep on the admission path.. getattr like the rest of the file instead?
There was a problem hiding this comment.
@valar-cop Followed in 9219a8d (this reply was held back and is posted with the later rounds). _reachable now looks up want_touch with getattr(self.offload, "want_touch", None) and calls it only if it is callable, like stats in the same file. An attached offload without want_touch still has its tier hits counted; it just doesn't get the LRU refresh. I left BlockManager._request_state_load calling it directly, because the block manager's state_offload is always the StateOffloadIndex it builds itself.
Test: TestVotersRegisterTouches::test_an_offload_without_touches_still_counts_a_tier_hit attaches an offload that only has hashes / could_serve. It fails on c54eda2 with AttributeError.
cc @valarLip
There was a problem hiding this comment.
ok, and the block manager's state_offload is indeed only the index built at block_manager.py:508, so the asymmetry is safe. but then the getattr exists just for the test fake (test_an_offload_without_touches_still_counts_a_tier_hit).. give the fake a want_touch and call it directly in both voters? don't want two conventions for the same method.
also 3 ci jobs still pending (atom test / vllm / sglang), hold until those are green.
…fload without want_touch A touch that finds the image outside LocalCPU's hot cache only confirms presence; count it (unrefreshed_touches) and warn once. The verdict stays resident: get can read the image now, and a later miss degrades to a recompute. PagedStateCheckpointCoordinator._reachable registers the touch through getattr, like stats, so an attached offload without want_touch keeps its tier hits.
valar-cop
left a comment
There was a problem hiding this comment.
some changes needed
any evidence touches actually reorder the lru? unrefreshed_touches == 0? and a disk backend run for the put fast path?
ci green.
| return resident | ||
|
|
||
| def _refresh(self, key) -> bool: | ||
| backend = getattr(self._storage, "local_cpu_backend", None) |
There was a problem hiding this comment.
these lmcache internals are all getattr, if a name changes we silently fall back to contains and every load still 'succeeds'.. can we resolve once in bind_storage_manager and fail loudly?
There was a problem hiding this comment.
@valar-cop The first half is done (1b311b6): bind_storage_manager resolves the LocalCPU LRU once (_local_cpu_lru), and touches use the resolved backend with no per-touch getattr.
On "fail loudly": 1b311b6 raised at bind, but your round-3 comment on line 46 asks not to take the engine down on an LMCache bump. 4eff581 settles on loud but not fatal:
- When an internal is missing, bind logs one WARNING naming it.
- Touches then only confirm presence.
- Every touch is counted in
state_offload_touches_unrefreshedon the existing stats line.
That rules out the silent fallback you were worried about: a drifted LMCache shows touches_unrefreshed growing with touches_sent, and it is 0 on our runs. I didn't keep the raise because a lost refresh only costs a recompute on a miss, which is how loads behaved before this PR. The full reasoning and the tests are in the line-46 thread.
Checked on real LMCache in the container (0.5.6.dev139+gf22dec28, the real LocalCPUBackend with its default LRUCachePolicy; the script drives the PR's codec). get_evict_candidates(hot_cache, 1) is what LocalCPUBackend.allocate calls under memory pressure:
order before touch [1, 2, 3]
evict candidate before touch [1]
touch(1) -> REFRESHED
order after touch [2, 3, 1]
evict candidate after touch [2]
cc @valarLip
There was a problem hiding this comment.
loud-not-fatal is the right call, and the real-lmcache run answers my earlier question.
one gap in the "drift shows up in touches_unrefreshed" story: the except at connector.py:333 maps a raising touch to REFRESHED and logs a traceback per touch, so a behavioral drift floods the log and leaves the counter at 0. count it as unrefreshed, or warn once like state_object.py:225?
| if self._storage is None: | ||
| return False | ||
| key = self.key(h) | ||
| if self._touch_key(key): |
There was a problem hiding this comment.
fast path is true for any backend, so a disk-only image never gets re-promoted into cpu tier now? base it on hot_cache residency instead
There was a problem hiding this comment.
@valar-cop You're right; I confirmed it against the LMCache 0.5.6 source. With a disk backend, batched_put writes to every backend. LocalCPU then evicts from its hot_cache while LocalDisk keeps the file, so StorageManager.contains still returns true. The fast path therefore skipped the store, and the image stayed out of the CPU tier until a load promoted it back through get.
Fixed in 1b311b6. touch now returns TouchResult (GONE / PRESENT / REFRESHED), and put skips the store only on REFRESHED, meaning the image was in LocalCPU's hot_cache and its LRU entry was just moved. A disk-only image (PRESENT) goes through batched_put again. LocalCPU takes it, since the key is not in hot_cache, and LocalDisk skips it because key in self.dict, so the re-store costs one D2H and no second disk write.
Test: test_a_disk_only_image_is_stored_back_into_the_cpu_tier. The mock spills an image out of the hot cache into a disk dict, and the test checks that the second put reaches batched_put again (2 puts, not 1) and the image is back in hot_cache.
Disk-backend run: I ran it on 4eff581 with LMCache's own LMCACHE_LOCAL_DISK (1000 GB, 442 GB used) on top of the same 208.6 GiB LocalCPU pool: MI355X, AgentX c64, 900 s. LocalDisk kept every image LocalCPU evicted, so 93 of 723 touches found their image only on disk (state_offload_touches_unrefreshed=93, touch_misses=0, and the one-time warning fired). All 480 state loads completed, 0 failed, 0 holds expired, Traceback 0. There is no counter for how many of the 3,858 stores were such re-stores (stores_attempted does not split them), so the re-store itself is covered by the unit test above, not by a number from this run.
cc @valarLip
There was a problem hiding this comment.
makes sense, and the code matches what you describe: the fast path only returns early on REFRESHED (atom/kv_transfer/offload/hybrid/kimi_k3/state_object.py:177), PRESENT falls back into batched_put.
93 of 723 touches finding their image only on disk is exactly the case that broke, so that run plus the unit test is good enough for me. one thing: those re-stores are invisible in the stats line, so if they ever grow we wouldn't see it .. maybe count them next to touches_unrefreshed? rest lgtm.
There was a problem hiding this comment.
Left as is for this PR. The disk-only case already shows up as state_touches_unrefreshed, counted on each touch that finds such an image. A separate re-store count would be one more stats key, and like state_touches_unrefreshed it would only reach Prometheus through the shared llm_engine whitelist.
| # lets later loads skip the hold, where a miss degrades to a recompute. | ||
| if not self._storage.contains(key): | ||
| return False | ||
| self._unrefreshed_touches += 1 |
There was a problem hiding this comment.
unrefreshed_touches only shows once via the warning, no ongoing signal in stats.. can we expose it so we can tell refresh from presence-only?
There was a problem hiding this comment.
@valar-cop Done in 1b311b6. The count is now in the existing state checkpoints: stats line as state_offload_touches_unrefreshed, next to state_offload_touches_sent and state_offload_touch_misses. That line now separates three outcomes:
- Refreshed: the image was in LocalCPU's hot cache and its LRU entry was moved.
- Presence-only: the image is resident on another backend but its eviction order was not touched. These are counted in
touches_unrefreshed. - Gone: counted in
touch_misses.
How it is plumbed:
- Each worker reports, per touch, a second completion on a new
k3_state_touch_refreshchannel with the sameStateTouchId, next to the existing resident / gone report onk3_state_touch. KVOutputAggregatorapplies the same failure-dominant quorum, so a touch counts as unrefreshed if any TP shard only found the image present.- The scheduler connector counts the failures.
take_state_touches_unrefreshedis forwarded throughLMCacheOffloadConnectorScheduler/MultiConnectorSchedulerliketake_state_touch_verdicts. Scheduler._apply_state_touch_verdictsadds the count toStateOffloadIndex.touches_unrefreshed.- The one-time WARNING on the worker stays.
- The refresh report is not a residency verdict: it never forgets a hash or marks one verified.
Tests:
test_a_presence_only_shard_counts_the_touch_unrefreshed.test_both_touch_reports_reach_quorum_across_tp: two ranks go through a realKVOutputAggregator, oneREFRESHEDand onePRESENT. The touch stays resident and counts one unrefreshed.test_presence_only_touches_reach_the_stats_line.test_the_shells_forward_the_unrefreshed_count.
On the GPU smoke with in-process LocalCPU only: 1b311b6 (MI355X, AgentX c64, 900 s, 208.6 / 104.3 GiB) shows state_offload_touches_sent=757 state_offload_touch_misses=126 state_offload_touches_unrefreshed=0, so no touch fell back to presence. With a LocalDisk backend added (4eff581), it shows 93 of 723: those touches found their image only on disk. Details are in the summary.
cc @valarLip
There was a problem hiding this comment.
lgtm, went through the plumbing: stats line, failure-dominant quorum, shell + multi forwarding are all wired as described.
one thing in connector.py:343: result is not TouchResult.PRESENT reports refreshed=True for a GONE image, so that flag really means "not unrefreshed".. rename it (or a comment), it reads like a residency signal next to resident.
There was a problem hiding this comment.
@valar-cop Added the comment in bdef7ef, next to the (resident, refreshed) verdict map: a GONE verdict counts as refreshed, since nothing was left unrefreshed. Kept the name to avoid touching the emit site and its tests.
There was a problem hiding this comment.
checked, the comment sits on the verdict map (connector.py:130) and matches the emit site: GONE gives (False, True) and never reaches state_touches_unrefreshed at connector.py:960. not renaming is fine. resolved on my side.
- bind_storage_manager resolves LocalCPUBackend's cpu_lock / hot_cache / cache_policy.update_on_hit once and raises when one is missing, instead of a per-touch getattr that silently degraded to contains(). A storage manager with no LocalCPU backend warns once and every touch is counted. - put()'s skip-the-store fast path only applies when the image is in LocalCPU's hot cache; a disk-only image is stored again so it returns to the CPU tier. - touch() returns TouchResult (GONE / PRESENT / REFRESHED). The worker reports refreshed-ness on a new k3_state_touch_refresh channel (failure-dominant across TP) and the scheduler surfaces it as state_offload_touches_unrefreshed in the state checkpoints stats line.
valar-cop
left a comment
There was a problem hiding this comment.
some changes needed, the hold is base-scheduler only and the hard raise worries me
any real lmcache run showing touch reorders hot_cache? and perf number?
ci green.
| self.waiting.appendleft(seq) | ||
| break | ||
| self.block_manager.record_allocation(seq, hbm_probe) | ||
| if not self.block_manager.record_allocation( |
There was a problem hiding this comment.
allocate_waiting still admits with record=True, so the hold never applies there? can we route it through the probe api too
There was a problem hiding this comment.
@valar-cop I checked this, and I'd leave it as is. Routing it through the probe API would make that path worse, not better.
allocate_waitingbelongs toDecodeScheduleronly: the decode process of RapidServe's shared-cache P/D split (DecodeEngineCore). The base scheduler's admission at line 2382 already goes throughrecord_allocation(..., defer_unverified=True).DecodeScheduleroverrides_scheduleand never calls_publish_state_loads. Touches are published only from there (_publish_state_loads->_publish_state_touches). So on this scheduler a touch is never sent and its verdict never comes back. Adefer_unverifiedhold there could only wait outOFFLOAD_LOOKUP_DEFER_S(2 s) on every admission with a state hit, then admit as it does today.- The same missing
_publish_state_loadsmeans a CPU-tier state load cannot be carried on this scheduler at all. The state tier is a base-scheduler feature; the prefill side of this split (PrefillScheduler) has noBlockManager. - The
can_allocate(seq)call inallocate_waitingis identical onmain; this PR doesn't change it.
So "the hold is base-scheduler only" is true, and it's by construction: it is scoped to the only scheduler that publishes the touches the hold waits for. Making DecodeScheduler carry state-tier loads and touches would be a separate feature. I've left it out of this PR.
cc @valarLip
There was a problem hiding this comment.
ok, checked — allocate_waiting calls allocate(seq) with no cached blocks, so no state load or touch gets queued there at all, and the only other publisher is the engine's idle drain (engine_core.py:440), which won't run under load. agree, leave it as is. add one line at the call site saying the verify hold is base-scheduler only, so the next reader doesn't ask again?
| hold it. | ||
| """ | ||
| offload = self.state_offload | ||
| if offload is None or not offload.refresh_hits or not seq.has_per_req_cache: |
There was a problem hiding this comment.
missing can_load check here? on kv_producer we hold for a load that never happens
There was a problem hiding this comment.
@valar-cop Yes, real; fixed in 4eff581. _tier_state_unverified ends in StateOffloadIndex.needs_verify(h), which only checked h in self.hashes. A kv_producer (can_load=False) fills hashes from its own stores, so a stale hash held the admission for a pass, up to OFFLOAD_LOOKUP_DEFER_S, while _attach_state_slots (through _tier_can_serve -> could_serve) would never offer the load. needs_verify now asks could_serve(h), the same capability + membership predicate request_load and both voters use, so the hold and the load cannot disagree. I put the check in the index, not at block_manager.py:1592, so every caller of needs_verify gets it.
Test: TestNeedsVerify::test_a_store_only_role_holds_nothing. A can_load=False index with a stale stored hash neither holds nor queues a touch. It fails on 1b311b6.
cc @valarLip
There was a problem hiding this comment.
checked: needs_verify now goes through could_serve (atom/model_engine/state_offload.py:296), same predicate as _tier_can_serve (atom/model_engine/block_manager.py:2098) and request_load, so a store-only index can't hold. one gap left in the stale-gone guard: it compares verified_at to the latest touch send, so a gone verdict from an older touch landing after a newer store plus a newer touch still forgets a live hash.. only costs one hit, no park, so fine to leave. LGTM.
| h = int(h) | ||
| if h not in self.hashes: | ||
| return | ||
| if not resident: |
There was a problem hiding this comment.
stale gone verdict can erase an image we just stored.. check _verified_at vs _touch_sent_at before forgetting?
There was a problem hiding this comment.
@valar-cop Fixed in 4eff581, as you suggested: on a gone verdict, apply_touch_verdict now first checks whether _verified_at[h] > _touch_sent_at[h]. If so, a store of h landed (note_stored marks it verified) after the latest touch was sent, so the touch looked before those bytes were there, and the verdict is dropped without counting a miss. This also covers a hash that was forgotten and stored again while an older touch was in flight: forget clears _touch_sent_at, so any later confirmation wins.
How likely it is end to end: within one scheduler pass, _apply_state_touch_verdicts runs before take_state_reports. A verdict and a store report from the same worker step therefore already land in the safe order. The window is a store report applied in an earlier pass than a verdict from a touch that ran before the store landed. That's narrow, but the index can't rule it out from what it sees, and a wrong forget throws away a CPU hit (the next request over that prefix recomputes it). The guard is two dict reads.
Trade-off: if two touches of h are in flight (they're throttled to one per 0.5 s), a resident verdict for the older one can make a gone verdict for the newer one look stale. The image is then kept until the next touch. That case costs at most a load that misses and recomputes, the pre-PR behavior, never wrong state.
Tests in TestIndexTouches, built with a hand-driven clock:
test_a_gone_verdict_older_than_a_store_is_ignored: touch sent, then the store lands, then gone arrives; the hash stays. Fails on 1b311b6.test_a_gone_verdict_for_a_forgotten_then_restored_hash_is_ignored: fails on 1b311b6.test_a_gone_verdict_after_a_store_and_a_newer_touch_applies: store, then a newer touch, then gone; the hash is dropped.
cc @valarLip
There was a problem hiding this comment.
lgtm. checked the guard and the ordering it leans on: _apply_state_touch_verdicts runs before take_state_reports in the pass (scheduler.py:4956 vs 4959), and forget clears _touch_sent_at so a re-store always wins. the two-touches-in-flight swallow is the only cost and it's a recompute, not wrong state.
one small thing: a dropped stale verdict is silent, no counter. if you want it visible in state checkpoints: later, fine, not a blocker.
There was a problem hiding this comment.
Left as is. A dropped stale verdict means a store landed after the touch looked, so the index already holds the newer fact; a counter would only show a race that is resolved correctly. I can add it if it turns out to be useful.
| refreshed. | ||
| """ | ||
| if not hasattr(storage_manager, "local_cpu_backend"): | ||
| raise RuntimeError( |
There was a problem hiding this comment.
why raise here? a lmcache version bump should not take the engine down, degrade like the backend is None case
There was a problem hiding this comment.
@valar-cop Agreed; changed in 4eff581. This also revises my answer to your earlier comment on line 184 (fail loudly), so I'm explaining the trade-off in both threads.
What it does now:
bind_storage_managerstill resolves the LocalCPU LRU once (_local_cpu_lru); touches nevergetattragain.- If
local_cpu_backend,cpu_lock,hot_cache,cache_policyor a callablecache_policy.update_on_hitis missing, it no longer raises. It degrades exactly like thelocal_cpu_backend is Nonecase: touches only confirm presence throughStorageManager.contains, and loads and stores are unaffected. - To keep it from being silent, which was your round-2 concern:
- bind logs one WARNING naming what is missing, e.g.
LMCache's storage manager has no local_cpu_backend.hot_cache, so state touches only confirm presence ... (every touch is counted in state_offload_touches_unrefreshed); - every touch then lands in
state_offload_touches_unrefreshedon thestate checkpoints:stats line. A drifted LMCache therefore shows up astouches_unrefreshedgrowing withtouches_sent, where it should stay at 0.
- bind logs one WARNING naming what is missing, e.g.
Why degrade rather than raise: the touch only buys performance (keeping a hot image from being evicted). If it is lost, a load that misses recomputes, which is how every load behaved before this PR. That is not worth refusing to start the engine over on an LMCache bump. The two rounds asked for different things; this keeps both: the internals are resolved once at bind, as you asked in round 2, and the failure is loud in the log and the stats without taking the engine down.
Tests in tests/test_state_tier_lru_refresh.py (each fails on 1b311b6):
test_a_local_cpu_without_the_lru_internals_degrades_to_presence[cpu_lock|hot_cache|cache_policy]test_a_policy_without_update_on_hit_degrades_to_presencetest_a_storage_manager_without_local_cpu_backend_degrades_to_presence
Each checks for a single bind warning that names the missing attribute, PRESENT touches, and that every touch is counted.
cc @valarLip
There was a problem hiding this comment.
degrade instead of raise is fine by me.
but the stat is not a clean drift signal: on your own disk run 93 of 723 touches were unrefreshed in normal operation (state_object.py:220, image only on LocalDisk), so a drifted lmcache and a disk tier look identical in touches_unrefreshed. your bind warning is the real signal then, don't lean on "should stay at 0".
minor: without hot_cache the put fast path (state_object.py:177) never matches, so a re-store of a resident image packs and D2Hs again and LocalCPU refuses the duplicate. rare, but fine if you're ok with that on a drifted lmcache.
There was a problem hiding this comment.
Left as is. This only happens on an LMCache whose LocalCPU internals moved, which bind already warns about once. The cost is a duplicate pack and D2H that LocalCPU refuses, not wrong state.
…icts, no hold on a store-only tier - A storage manager missing local_cpu_backend or one of its LRU internals no longer raises at bind: touches only confirm presence, bind warns once naming what is missing, and every touch is counted in state_offload_touches_unrefreshed. - apply_touch_verdict ignores a gone verdict when the hash was confirmed (stored) after its latest touch was sent; the touch looked before those bytes landed. - needs_verify asks could_serve, so a store-only role (kv_producer) does not hold an admission for a load it never makes.
|
@valar-cop thanks for the three rounds. This one comment answers all of them, because my round-1 and round-2 replies were held back and are posted now together with round 3. cc @valarLip What changed since c54eda2 (head is now 4eff581; no new env vars, merge-only):
Every inline comment has its own reply. The only one I did not change is Does a touch reorder LMCache's hot_cache on real LMCache?
Disk backend (
Perf.
GSM8K (c54eda2; nothing numeric changed since). Same stack and node, real MTP 3 verification, FP8 KV, 208.6 / 104.3 GiB:
LMCache interface (0.5.6):
Tests / CI on 4eff581.
|
|
@ZLkanyo009 ok, the direct hot_cache test plus touches_unrefreshed 0 / 93 is the evidence i wanted, and degrading instead of raising on a missing internal is the right call. two things left: tp > 1 and gfx942 never ran, and ci is still pending (atom test, vllm, sglang). can we get at least one tp>1 smoke before merge? lgtm apart from that. |
valar-cop
left a comment
There was a problem hiding this comment.
not ready yet.. need tp>1 evidence and clarify the codec doc
any tp>1 run? gfx942? and please re-run ATOM Test, it was cancelled
ci green.
| # Confirmed after the latest touch was sent (its store landed since, | ||
| # or the hash was forgotten and stored again): the touch looked | ||
| # before those bytes were there, so its verdict is stale. | ||
| if self._verified_at.get(h, -1e18) > self._touch_sent_at.get(h, -1e18): |
There was a problem hiding this comment.
single timestamp cannot attribute the verdict to the touch.. carry a round id instead of comparing wall clock?
There was a problem hiding this comment.
@valar-cop Leaving the timestamp guard as is. The case it misses is the one you described in r4426150719: a gone verdict from an older touch that lands after a newer store and a newer touch. The index then forgets a hash whose image is still there, and the next request over that prefix recomputes instead of loading. Nothing parks and no wrong state is restored, so the cost is one lost hit.
Attributing a verdict to its send needs the touch's generation to come back with it. The worker side already has one (StateTouchId(prefix_hash, generation), renamed from round to match the sibling ids), but the scheduler reduces the TP quorum to {hash: resident} in take_state_touch_verdicts, and StateOffloadIndex would have to keep a generation per hash next to _touch_sent_at. That changes the verdict interface across the connector, the scheduler and the index for a case that costs one recompute, so I'm not doing it in this PR.
In the TP2 + EP2 run (summary comment) the PR side had 55 touch misses in total, and a load that missed after a resident verdict would show up as loads_failed, which stayed at 0.
cc @valarLip
There was a problem hiding this comment.
fine by me, that's the case i already called one lost hit in r4426150719, and the connector reduces to {hash: resident} at connector.py:726, so a generation would have to ride through it. not worth that for one recompute, and loads_failed at 0 covers the park case. leave it.
| logger = logging.getLogger("atom") | ||
|
|
||
|
|
||
| class TouchResult(enum.IntEnum): |
There was a problem hiding this comment.
docstring says get promotes it back, but get never inserts into hot_cache? which is true...
There was a problem hiding this comment.
@valar-cop The production comment was right about the behavior, but it didn't say where the promotion happens, and the fake's comment was wrong. Both are reworded in 8e9e0c9.
StateByteCodec.getcallsstorage_manager.get(key). In LMCache 0.5.6,StorageManager.get(lmcache/v1/storage_backend/storage_manager.py, lines 437-461) walks the active backends. On a hit from any backend other than LocalCPU, PD or Maru, it callsLocalCPUBackend.submit_put_task(key, memory_obj), which inserts the key intohot_cacheand callscache_policy.update_on_put. So a disk-only image that is read does go back into LocalCPU. LMCache does it, not ATOM, and theTouchResult.PRESENTcomment now says thatStorageManager.getis what writes it back.- The fake's disk only stores presence (
self.disk[key] = True) and itsgetreads only the hot cache, so it never modeled the read-back that its comment claimed. The comment now says it is presence only, used bycontains; no test loads a disk-only image.
No behavior change.
cc @valarLip
There was a problem hiding this comment.
lgtm, the PRESENT comment now names the write-back (StorageManager.get -> submit_put_task) and nothing claims ATOM does it, and the fake comment matches what the fake actually models. can't read lmcache source from this checkout so i'm taking your file/lines as the citation. the code is consistent: get re-touches after unpack at atom/kv_transfer/offload/hybrid/kimi_k3/state_object.py:340, so a promoted image does get refreshed.
| # The LMCache chunk size in tokens, read where the config is rather than | ||
| # off the connector object. | ||
| self._joint_chunk_tokens = 0 | ||
| lmcache_pool_bytes = 0 |
There was a problem hiding this comment.
lmcache stauff should be here
There was a problem hiding this comment.
@valarLip I read this as: LMCache-specific code shouldn't live in block_manager.py. If you meant something else, please correct me.
Done in 77d081e. BlockManager no longer reads anything LMCache-specific for this PR:
lmcache_pool_bytesand thestate_tier_lmcache_sizes(config)call are gone. The chunk-size probe is back to main's one line,self._joint_chunk_tokens = state_tier_chunk_tokens(config).StateOffloadIndexnow takes the confirmation window itself,verified_for_s=..., in seconds, instead of LMCache'spool_bytes.BlockManagerpassesstate_tier_verified_for_s(config)and knows nothing about the pool.- The LMCache read lives in
atom/model_engine/state_offload.py, next to main'sstate_tier_chunk_tokens. Both now share one reader,_lmcache_engine_config(config), which does thebuild_lmcache_configcall and themultisub-config unwrap that was inline in main'sstate_tier_chunk_tokens.state_tier_verified_for_sturnsLMCACHE_MAX_LOCAL_CPU_SIZEinto the window through the existingtier_verified_for_s. - Behaviour is unchanged:
- Same values: 256 tokens and 25 s for a 100 GiB pool in the test; 52 s at 208.6 GiB.
- Same gate: both reads happen only when a state tier is hosted (
kv_offload_enabled). An engine without offload, or without LMCache installed, still neither imports LMCache nor logs a traceback; main'sTestJointChunkProbeIsGatedtests pass unchanged. - With an unreadable LMCache config, the chunk probe still logs its one traceback, and the window falls back to the 5 s floor.
- What's left in
block_manager.pyfrom this PR is one import and thatverified_for_s=argument.
LMCache-specific code that was already in block_manager.py on main (99ebf63), and that this PR does not touch:
- The chunk-size probe in
__init__(state_tier_chunk_tokens(config)into_joint_chunk_tokens), and thekv_offload_enabledgate around it. - The joint KV boundary (
_joint_kv_boundaryand its helpers), which aligns to LMCache's chunk grid and reportslmcache_within_hbm. - The tier-hit reach that reads LMCache's
seq.offload_joint.kv_prefix_tokens, aroundcompressed_hit. - Assorted comments that name LMCache.
Moving those behind a backend-neutral interface would change main's code and the joint-load path, so I left them out of this PR. Do you want that done in a separate PR?
Tests:
tests/test_state_tier_lru_refresh.py::TestVerifiedWindow::test_the_block_manager_sizes_the_window_from_the_lmcache_pool: now stubs the LMCache config reader, so both values go through the real helpers.- New
test_an_unreadable_lmcache_config_gets_the_floor. - Full CPU suite: 10131 passed, 3 failed. The same 3 fail on
main: the inductor C++ build fails on the CPU host. - black 26.10.1 / ruff clean.
- No GPU run: nothing on the admission or scheduling path changed.
There was a problem hiding this comment.
@valarLip done in 8d40782. This PR adds nothing LMCache-specific to block_manager.py: one import, the verified_for_s= argument and the admission hold. The pool-size read lives in state_offload.py, and it now shares one reader, _lmcache_engine_config, with main's state_tier_chunk_tokens, as described above. That reader had been dropped in a later round and is back. Main's existing LMCache code in block_manager.py is unchanged; I can move it in a separate PR if you want.
|
ci triage for 8d40782:
|
BlockManager no longer reads LMCache's CPU pool size: StateOffloadIndex takes the confirmation window (verified_for_s) instead of pool_bytes, and state_offload.state_tier_verified_for_s derives it from LMCACHE_MAX_LOCAL_CPU_SIZE, next to state_tier_chunk_tokens, which is restored as on main. Both share one LMCache config reader. Same values, same gating (only when a tier is hosted).
valar-cop
left a comment
There was a problem hiding this comment.
a few small things..
tp>1 run of the touch/verdict path?
ci green.
| # to carry state touches. | ||
| refresh_hits=True, | ||
| # How long a confirmed image is trusted. | ||
| verified_for_s=state_tier_verified_for_s(config), |
There was a problem hiding this comment.
log this window once at startup? otherwise the holds/expiries line has no reference
There was a problem hiding this comment.
@valar-cop Not adding a startup line for this. The window is a fixed function of one value LMCache already logs at startup: max_local_cpu_size appears in LMCache's Creating LMCacheEngine with config line. The window is clamp(0.25 s × max_local_cpu_size GiB, 5, 60), and the offload README gives that formula; 208.6 GiB gives 52 s.
The other input is the hold bound, OFFLOAD_LOOKUP_DEFER_S: 2.0 s by default, documented in docs/environment_variables.md. So the holds / expiries counters can already be read against both numbers from the startup log and the config. Nothing is wrong or unsafe without the extra line, so I'm keeping this PR to fixes only.
For reference, the TP2 + EP2 run in the summary used 208.6 GiB per worker, which gives 52.15 s.
cc @valarLip
There was a problem hiding this comment.
ok, taking lmcache's config log line as given since i can't read lmcache here. formula is in the README and the pool size is in the config it prints, so the window is derivable, and the unreadable-pool path already logs the 5 s floor at atom/model_engine/state_offload.py:664. dropping it.
| touch_id.prefix_hash, | ||
| exc_info=True, | ||
| ) | ||
| result = TouchResult.REFRESHED |
There was a problem hiding this comment.
a raising touch counts as refreshed here, so drift looks healthy.. warn once and count it unrefreshed?
There was a problem hiding this comment.
@valar-cop Fixed in 8e9e0c9. A touch whose codec call raises is now reported as resident but unrefreshed, the same verdict as TouchResult.PRESENT:
- The image stays indexed, because the exception says nothing about the bytes.
- The touch is counted as unrefreshed, so a drifted LMCache that raises on every touch shows up instead of looking healthy. After the independent review the count is
state_touches_unrefreshedin the K3 connector'sget_statistics()(024e3e8) rather than a separate take method on three connectors. - Only the first failure on each worker logs a traceback, instead of one per touch.
Test: TestConnectorTouches::test_a_failing_touch_is_unrefreshed_and_logged_once (three failing touches over two steps: all (True, False), one record with exc_info). It fails on 77d081e. The offload README's touch paragraph says it too.
This is the same gap you raised in r4426148581.
cc @valarLip
| logger = logging.getLogger("atom") | ||
|
|
||
|
|
||
| class TouchResult(enum.IntEnum): |
There was a problem hiding this comment.
nothing here promotes disk-only back to hot_cache, reword to what get actually guarantees? same for the fake
There was a problem hiding this comment.
@valar-cop Same point as r4426260647, answered there: LMCache's own StorageManager.get writes a hit from LocalDisk back into LocalCPU via LocalCPUBackend.submit_put_task (0.5.6, storage_manager.py lines 437-461), and nothing in ATOM does. 8e9e0c9 says that in the TouchResult.PRESENT comment. It also fixes the fake's comment, which claimed a read-back the fake does not model: its disk only answers contains.
cc @valarLip
There was a problem hiding this comment.
checked, both comments now say what the code does: _refresh only returns PRESENT off contains and get goes straight to storage_manager.get (state_object.py:232, :313), so naming LMCache's write-back instead of ours at state_object.py:30 is right, and the fake's comment no longer claims a read-back it doesn't model.
resolved on my side.
A touch whose codec call raised was reported as refreshed, with a traceback per touch: a drifted LMCache flooded the log while state_offload_touches_unrefreshed stayed at 0. It now keeps the image indexed (resident), reports it unrefreshed, and only the first failure logs a traceback. Also name where a disk-only image goes back into LocalCPU (LMCache's StorageManager.get write-back), and fix the test fake's comment, which claimed a read-back it does not model.
… components The block manager and the scheduler now only call the index: - `StateOffloadIndex.hold_admission` makes the hold decision and counts it; the block manager's `_resume_hash_off_hbm` only says which image an admission would load from off-HBM. - `request_load` registers its own touch, so `_request_state_load` no longer does. - `publish_touches` / `collect_touch_verdicts` replace the scheduler's `_publish_state_touches` / `_apply_state_touch_verdicts`. - `_reachable` calls `want_touch` directly (the getattr shim for an index without touches is gone; every index has them). tests/test_state_tier_lru_refresh.py is split into the existing files: codec and connector touches -> test_lmcache_offload_connector.py, index and budget -> test_state_offload_index.py, window sizing -> test_block_manager.py, admission hold and voters -> test_state_checkpoint.py.
No behaviour change. - `StateOffloadIndex` reads `OFFLOAD_LOOKUP_DEFER_S` itself (`verify_defer_s` and the test-only `defer_s` argument are gone); `stop_refreshing` is folded into its one caller, `publish_touches`. - The verify window is one constant, 0.25 s per GiB of pool, taken in GiB. - `state_tier_budget_warning` drops the argument only its message used. - `StateTouchId` drops a round check no input can trip, and `enqueue_state_touches` drops an empty-list guard that answered "cannot carry touches" for a call that never happens. - Comments, docstrings and the offload README say the constraint, not the derivation. - Tests: copy-pasted cases parametrized, duplicates merged, function-local imports moved to the top.
Per the independent review of the cleanup: - The presence-only touch count rides KimiK3OffloadScheduler.get_statistics() instead of its own take method forwarded by the shell and multi. - An offered load no longer queues a touch: the load is its own verdict and the codec's get refreshes the LRU. - needs_verify folds into hold_admission, its only caller; refresh_hits and verified_for_s become one verified_for_s argument; the replay check reuses tier_resume_replays. - The codec warns once per cause; StateTouchId.round is renamed generation; the touch methods join StateOffloadFace; the budget warning takes GiB. - Tests share the offload clock in conftest and the K3 scheduler helper, and cover the touch state leaving with its hash.
Appended after the last test they collided with #2636's appended tests on a merge; the block moves above TestStateLoadsAndStoresRunInSeparateLanes.
|
@valarLip This push is an overall cleanup of the PR, not another point fix. Since the version you reviewed on 11-01 (
Behaviour is unchanged apart from the raising-touch fix and one dropped duplicate touch (below). Size (added lines against the merge base, blank lines not counted): source 464 → 438, tests 1052 → 903, comments and docs 312 → 164. In total +2103/−62 → +1709/−69. Moved out of the shared files
Removed or folded
Reviewed and kept
Verification
The PR description is rewritten to say why, and its file list matches this diff. |
| self.waiting.appendleft(seq) | ||
| break | ||
| self.block_manager.record_allocation(seq, hbm_probe) | ||
| if not self.block_manager.record_allocation( |
There was a problem hiding this comment.
please add one comment here or at allocate_waiting, the hold is base-scheduler only since DecodeScheduler never publishes touches
There was a problem hiding this comment.
@valar-cop Done in 3ccc2e9: the comment at the hold now says it is base-scheduler only, since DecodeScheduler never publishes touches and so never holds.
There was a problem hiding this comment.
checked, the comment is there at scheduler.py:2387 and the hold claim is right: _schedule is overridden (scheduler.py:5390) and allocate_waiting never passes defer_unverified, so a decode admission can't hold. resolved on my side.
one wording question: the idle drain still calls _publish_state_loads on a decode engine (engine_core.py:441), so can a decode engine publish touches when idle? if so, 'never publishes touches' is a bit strong, 'never on this path' covers it.
ci still in progress on this head, hold until green.
There was a problem hiding this comment.
@valar-cop Yes, it can: the idle drain reaches _publish_state_loads and so publish_touches, and allocate_waiting -> can_allocate -> _reachable registers touches. Fixed in bdef7ef: the comment now gives the real reason, that allocate_waiting never passes defer_unverified, so a decode engine never holds.
There was a problem hiding this comment.
checked, the comment is right now: on a decode engine allocate_waiting goes can_allocate -> _reachable -> want_touch (page_unit_checkpoint.py:1081) and the idle drain's _publish_state_loads does publish those touches (scheduler.py:3871), but neither path calls record_allocation, so the hold can't fire. resolved on my side.
pre checkin still in progress, hold until it's green.
Reverts the hunks that reworded, reflowed or refactored code the feature does not need to change; the behaviour is unchanged. - types.py: StateStoreOperationId docstring back to main. - _offload_common.py: no new StateOffloadFace abstract methods (the shell and multi forwards already use getattr). - kimi_k3/state_object.py: drop the _cache_keys comment and the get() miss-pop; get() keeps only the LRU refresh. put() keeps its original key line. - kimi_k3/connector.py: max_cache_bytes=state_tier_cpu_bytes() as on main. - state_offload.py: original class, forget, could_serve and state_tier_cpu_bytes docstrings; state_tier_chunk_tokens no longer refactored (state_tier_verified_for_s reads the pool size itself). - block_manager.py: record_allocation keeps its original shape, allocate is untouched; _resume_hash_off_hbm mirrors allocate's choice instead of sharing a new _state_resume_hash. - scheduler.py: original _publish_state_loads docstring and lines; the touch calls are inserted only. - tests: conftest.py and _FakeStorage back to main; the clock fixture lives in the two files that use it; original test module docstring restored; the new block sits above the existing section header.
…rning - block_manager.py: _state_resume_hash is back, shared by allocate and the admission hold as asked in review, so the two cannot drift apart. record_allocation keeps its original shape. - Drop the startup state-cap / pool-size warning; it belongs to the removed reservation, not to the hit refresh. - tests: drop the four codec tests that pass on main unchanged (they test _put, which this PR does not change) and the fake LocalCPU knobs only they used. - scheduler.py: say the hold is base-scheduler only.
|
Update Diff vs. main (merge-base
Reverted, by file:
Also, per @valar-cop's note on Not changed (seen, left as on main):
Verification on
Pending:
|
|
Update Conflict: only The diff against the new main is identical to the previous one: 19 files, +1558/−25, with the same files and the same changed lines. Nothing from main leaks into it. Checked against #2906 (lmcache_mp lookup thread, transfer thread, same-step saves); nothing here needed to change:
Verification on
Pending:
|
… imports - scheduler.py: DecodeScheduler does publish touches when idle; it never holds because allocate_waiting never passes defer_unverified. - kimi_k3/connector.py: say that a GONE verdict counts as refreshed. - state_offload.py: name loads_deferred_for_verify in the stats comment; an expired hold means verdicts are lost or late (e.g. a stalled step), not necessarily lost. - tests: import the touch channels with the other kimi_k3 names at the top instead of inside test functions.
|
Update Diff vs. main (merge-base The review-pr pass found 0 must-change, 0 worth-addressing and 4 smaller items. No correctness problems. Changed:
Follow-ups, not in this PR:
Verification on
Pending:
|
- K3 worker keeps each touch's TouchResult (a plain Enum, compared by identity) and turns it into the two completions in `_emit_touch_verdicts`, instead of a (resident, refreshed) tuple built up front. With no tier, every touch is GONE before the loop. - `state_tier_chunk_tokens` and `state_tier_verified_for_s` share one reader, `_lmcache_engine_config`; the import, the `multi` unwrap and the build move into it unchanged. - Conversions done once: touch ids are ints where `enqueue_state_touches` makes them; the later int()/bool()/float() calls are gone. `-inf` instead of a -1e18 sentinel. - Comments state constraints only; the measurement behind the window constants is in the PR description, and the offload README no longer repeats them. - Tests check emitted completions and pool contents rather than private state; `offload_clock` lives in conftest; STALE and the window floor come from `tier_verified_for_s`; no function-local imports. - New tests for the wiring the existing ones missed: a touch-only step is work, `start_load_kv` touches, the scheduler applies verdicts, and both scheduler shells (plain and multi) forward touches and verdicts. No behaviour change: same values and branches. Checked with 34 mutants of the touch path (all killed), the CPU suite (same 3 failures as main), GPU unit tests (1863 passed; the 2 engram failures are on main too) and Kimi-K3 TP2+EP2 GSM8K 0.978 on the previous head e9481f9bd. Limit: no GSM8K rerun on this commit, and that run sent no touches.
|
Update Diff vs. main (merge-base
What changed, by kind:
Not changed:
Verification on
Pending:
|
Why
State images (Qwen3.8 / Kimi-K3 recurrent state,
StateByteCodec) share LMCache's LocalCPU pool and its LRU with paged KV. A KV lookup refreshes the chunks it hits, but a state hit is answered by ATOM'sStateOffloadIndex, so LMCache never saw an image used: a prefix that hit every turn kept its KV young and its image old, pressure evicted the image first, and the index kept advertising it until a joint KV + state load parked on it, missed, and recomputed the whole prompt. Before this PR, AgentX c64 with a 224 GiB pool recomputed 2.8% of all input tokens that way (88 of 3,786 state loads failed; https://github.com/wuhuikx/LLM-Booster/issues/157).What
LMCacheOffloadMetadata.state_touches, oneStateTouchId(prefix_hash, generation)each). Every rank moves the image to the young end of LocalCPU's LRU (StateByteCodec.touch:cache_policy.update_on_hitundercpu_lock).getrefreshes too, so an offered load needs no touch, and a re-store of an image that is still hot is skipped.k3_state_touchwhether the image is still there, failure-dominant across TP like a joint load. A gone image leaves the index. An admission that would load an image not confirmed withintier_verified_for_s(0.25 s per GiB ofLMCACHE_MAX_LOCAL_CPU_SIZE, clamped to 5–60 s) is held one pass for that verdict (StateOffloadIndex.hold_admission, viaBlockManager.record_allocation(defer_unverified=True)), at most the existingOFFLOAD_LOOKUP_DEFER_S. It then plans a lower boundary instead of parking on a miss. Why that window: a confirmed image is at LocalCPU's young end, so it survives until a whole pool of new entries arrives. AgentX c64 filled about 0.25 GiB/s per GPU; the window assumes 1 GiB/s and trusts a quarter of that turnover. 5 s is well above a touch round trip; 60 s is the largest window tried at c48 / c64. It cannot ridelookup_pending(the lmcache_mp deferral that shares this bound): that check runs before the tier lookup and the joint decision that pick the hash to verify.No new environment variables. If LMCache's LocalCPU internals are missing, or an image is held only by another backend (LocalDisk), a touch only confirms presence, warns once and is counted in the K3 connector's
get_statistics()asstate_touches_unrefreshed; loads still work. Touch ids are not tombstoned byKVOutputAggregator, so they cannot push store / source tombstones out of its ring.The tier logic lives in
state_offload.py(index) andkimi_k3/(worker).block_manager.pyandscheduler.pyonly call it:record_allocationgains a keyword-onlydefer_unverified=False(other callers are unchanged) and askshold_admissionabout the hash_state_resume_hashpicks (the same helperallocateuses), and the scheduler callspublish_touches/collect_touch_verdicts.The vLLM plugin also uses
StateByteCodec(plugin/vllm/kv_transfer/connector.py), so it gets the codec-level changes only:getrefreshes the LRU, a hot image is not stored twice, and binding warns once if LMCache's LocalCPU internals are missing. It has no engine-side touches or admission hold.Not changed
Seen, left as on main:
OFFLOAD_STATE_CPU_SIZEstays a cap with no reservation and no startup check against the pool size. Thestate_tier_cpu_bytesdocstring still says the cap is "carved from" the pool.StateByteCodec.get()keeps a missed key in its own cap LRU until the next put or touch drops it.StateOffloadFacegets no touch methods; the K3 scheduler is the only implementer, and the shell and multi connectors forward withgetattr.StateOffloadIndex(plugin/vllm/kv_transfer/kda_state.py), so the LRU asymmetry remains there. Fixing it needs touches in the plugin's metadata, run on the plugin worker, with verdicts carried back through vLLM's connector output, andtests/plugindoes not run in CI. Left for a separate change.Tests
In the existing component files:
test_state_offload_index.py(touch throttling, verdicts, the hold and its bound, the window),test_lmcache_offload_connector.py(codec refresh against a fake LocalCPU LRU, connector round trip, TP quorum),test_state_checkpoint.py(admission hold, voters, resume hash, scheduler retry),test_offload_shell.py(the scheduler shells' forwards),test_block_manager.py(window from the pool),test_kv_aggregator.py(tombstones). 34 mutations of these mechanisms (the cleanup that drops a hash's touch state with it, and the wiring: a touch-only step is work for the worker,start_load_kvtouches, the scheduler applies verdicts, the plain andmultischeduler shells forward touches and verdicts) were each checked to turn a test red.Accuracy and performance
TP2 + EP2 on MI355X (
-tp 2 --enable-expert-parallel, MTP 3, FP8 KV,LMCACHE_MAX_LOCAL_CPU_SIZE=208.6,OFFLOAD_STATE_CPU_SIZE=104.3,--max-num-seqs 64), AgentX c64 for 1800 s, same cards. Main is the deployment stack without this PR.3ccc2e9fe, and 0.978 (stderr 0.004) one9481f9bd, the simplification pass before its review fixes, which change no behaviour. 0.9773 on the previous revision (paired stderr 0.0026). GSM8K makes no state loads, so these differences are run-to-run noise.Earlier, single GPU (MI355X, MTP 3, FP8 KV,
LMCACHE_MAX_LOCAL_CPU_SIZE=208.6): GSM8K 1319 questions 0.9795 (before: 0.9795). Split CPU retrieval (pass 2 answers every question again with its prefix loaded from the CPU tier: 90% of prompt tokens and 1,273 state images, 0 failed): 0.9735 against 0.9750 in pass 1, 16 of 1319 answers differ (paired stderr about 0.003). AgentX c64, 3600 s, 4 card-swapped rounds against the previous revision: 128,070 → 128,694 tok/s/GPU, TTFT p50 3.28 → 3.35 s, 0 failed state loads either way. Against the reservation revision: c64 +2.0% tok/s/GPU, 0 of 3,894 state loads failed.