Repository navigation
fix(mtp): stop per-turn KV/SSM memory bloat (prefix-cache retention + no-op cache clearing) - #47
Merged
Merged
Conversation
- Add tape_rollback.py with TapeRecorder, TapeBuffer, LayerTape - KB-scale tape instead of MB-scale snapshots (5000x reduction) - Update cache_patch.py GatedDeltaNet patch to record per-position tape - Remove K clamping for SSM caches in generator.py - Add bit-exactness verification utilities - Add unit tests in test_mtp_tape_rollback.py - Add benchmark script bench_mtp_k3.py - Add implementation status docs in docs/tape-rollback-status.md Gate test: verify K=3 output is bit-exact vs K=0 baseline. Expected: 81% acceptance at K=3, ~2.2x speedup vs K=1. Refs: #44 MTP speculative decoding
…y ratchet) The vendored MTP _step wrapper kept one suspended mtp_generate_step generator per uid in _state and only cleaned it on uid REUSE. mlx-lm allocates a fresh uid per request under normal serving, so every completed request leaked its generator, whose frame pins that turn's entire cache set: the (disk-restored, dequantized bf16) attention KV, a full GatedDeltaNet SSM state set (~151MB), the K-position rollback tape, and the MTP-head KV over the full context. Idle active Metal memory ratcheted 0.2-0.35GB per turn on a growing conversation until OOM (~88GB pinned at 40K context on a 72GB baseline). The existing cleanup helpers freed nothing on the attention layers: cache.state = None hits mlx-lm KVCache's state setter, which unpacks self.keys, self.values = v and raises TypeError on None; every caller swallowed it with a bare except. _cleanup_uid also referenced a _clear_gb_cache defined in a sibling scope (latent NameError, unhit because uid reuse never fires). Fix: - _release_kv_cache_fields: nulls the real per-type storage (KVCache/ QuantizedKVCache keys/values/offset, ArraysCache .cache slots, rollback_state tapes), with an identity skip-set so caches still owned by the live GenerationBatch are never touched. - _cleanup_uid_storage: closes the generator and frees the caches its frame pins (reached via gi_frame before close); on mid-stream handoff to plain decode only the generator-owned MTP-head KV and rollback tapes are freed, backbone caches stay live. - _purge_stale_states: sweeps _state for uids no longer in the live batch; wired into _mtp_step and, via the _mtp_purge_stale hook on BatchGenerator, into Scheduler._cleanup_finished so storage returns at request completion. Measured on the production launcher (force-spec-decode, MTP K=2, disk-kv-restore): before, idle memory +1.98GB over 8 small turns with SSM (1,64,128,128) arrays growing 72->396 and bf16 KV (1,2,N,256) arrays 48->248, monotonic. After, per-turn counts and idle memory return toward baseline between turns.
Second retainer behind the MTP-era per-turn memory ratchet (found by gc referrer walk: leaked SSM/KV arrays chain to vllm_mlx.prefix_cache.CacheEntry inside the PrefixCacheManager trie). Scheduler._cleanup_finished stores every finished request's full cache into PrefixCacheManager, which evicts by ENTRY COUNT only (default 100). On hybrid models an entry is a non-shareable full cache set (36 GatedDeltaNet SSM states + 12 bf16 attention KV layers) that reaches hundreds of MB to GB at long context after the disk-restore path dequantizes int4 to bf16 — 100 entries is effectively unbounded, so idle resident memory ratchets one cache set per turn until the process OOMs. --cache-memory-mb was accepted but only ever consumed by MemoryAwarePrefixCache, which this serving path never instantiates. PrefixCacheManager now tracks per-entry resident bytes (mlx-lm cache .nbytes, with an ArraysCache fallback) and evicts LRU-first while the total exceeds max_bytes; the scheduler wires max_bytes from cache_memory_mb. Stats expose entries/total_bytes/max_bytes.
…r owns A hybrid (non-trimmable) cache entry is a full resident cache set (36 GatedDeltaNet SSM states + bf16 attention KV) that can only hit by exact/shorter prefix via deepcopy. With the disk-checkpoint tier on, the same entry is persisted quantized at completion and served by kv-restore, so the resident bf16 copy in PrefixCacheManager bought no hits on multi-turn chat (re-templated history diverges from the stored prompt+output key) while ratcheting idle memory one full cache set per finished request. Skip the in-memory store only when BOTH hold: the cache has non-trimmable layers AND kv_disk_checkpoint_interval > 0. Pure-attention models and disk-tier-off deployments are unchanged.
Adds the missing regression coverage for the PR #47 memory-leak fix and greens the tape-rollback / MTP-inject tests that were failing on the branch. Regression tests - test_prefix_cache.py: byte-cap eviction on PrefixCacheManager. Asserts eviction fires at max_bytes, _total_bytes stays equal to the sum of surviving entries across evictions, and pinned entries are never evicted. - test_mtp_state_release.py: _release_kv_cache_fields nulls KVCache keys/values/offset and ArraysCache .cache slots, drops rollback tapes, and identity-skips caches in skip_ids. MTP-inject code fixes (surfaced by the previously-red tests) - qwen3_5_inject: guard the post-quantize logger.info so it no longer dereferences quant_info["bits"] when the base model is FP (_detect_base_quantization returns None) -- was an unconditional NoneType subscript crash on every unquantised model. - qwen3_5_inject: run _fix_mtp_norm_double_shift only after confirming num_mtp_layers >= 1, so a rejected / non-MTP model is never mutated. Tape-rollback fixes - test_mtp_tape_rollback: fix a self-sabotaging mock __getitem__ that nulled the slot before returning it (reads always yielded None). - tape_rollback.estimate_tape_bytes + docstrings: replace the fictional "KB-scale" figures with the real GatedDeltaNet state size. The fp32 recurrent state is 64*128*128*4 B per layer; across 36 linear-attention layers that is 150,994,944 B (~151 MB), matching scheduler.py. Minor - scheduler: hoist gc.collect()+mx.clear_cache() out of the per-uid _cleanup_uid_storage loop; _purge_stale_states now reclaims once after the whole loop (reclaim flag; single-uid callers unchanged).
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.
Problem
The MTP model (
Qwen3.5-122B-A10B-oQ4-mtp) bloats memory over a long conversation until it OOMs: ~88GB pinned at only ~40K tokens (~16GB above the ~72GB baseline), ratcheting per turn and never returning. Pre-MTP the same box reaches 192K context clean, so the retention is entirely MTP-path machinery.Root cause (two retainers, both measured live)
Leaked MTP generator state.
_install_mtp_vendored's per-uid_stateonly cleaned up on uid REUSE, but mlx-lm allocates a fresh uid per request, so every finished request left a suspendedmtp_generate_stepgenerator whose frame pins that turn's full cache set (12 attention KVCache, 36 GatedDeltaNet ArraysCache SSM states, rollback tape, MTP-head KV over the whole context).In-memory prefix cache retention (the dominant one).
--enable-prefix-cachedefaults ON and was active regardless of the launcher flag (dropping--enable-prefix-cachejust falls back to the default).Scheduler._cleanup_finishedstores every finished request's full extracted cache intoPrefixCacheManager, which evicts by ENTRY COUNT (100) only, not bytes;--cache-memory-mbwas accepted but never enforced (MemoryAwarePrefixCachewas never instantiated). On this hybrid model each entry is a full bf16 cache set, so the disk-restored, dequantized bf16 KV was retained turn over turn.Latent no-op bug:
cache.state = Noneon an mlx-lmKVCachehits thestatesetter (self.keys, self.values = v), which raisesTypeErroronNone, swallowed by a bareexcept. So the existing cleanup (_clear_gb_cache,_cleanup_uid, and the prior_statereap) freed nothing on the 12 attention KV layers. A companion_clear_gb_cacheNameError was dead code that never fired because uid reuse never happens.Fix
_release_kv_cache_fields()nulls the real per-type storage (KVCachekeys/values/offset, ArraysCache.cacheslots,rollback_statetapes) with an identity skip-set protecting caches still owned by the live batch._cleanup_uid_storage()reaches the suspended generator frame (gi_frame.f_locals) to releasemodel_cache/mtp_cachebeforeclose()._purge_stale_states()sweeps_statefrom_mtp_stepand_cleanup_finished. Fixes the NameError. Mid-stream handoff to plain decode frees only generator-owned MTP-head KV + tapes; the backbone stays live.PrefixCacheManagernow tracks per-entry resident bytes and evicts LRU-first pastmax_bytes, wired fromcache_memory_mb(the previously dead flag is now enforced)./debug/mx_arrays,/debug/mx_referrers) return 404 unlessQMLX_DEBUG_GC_SCAN=1(off by default).Files:
vllm_mlx/scheduler.py,vllm_mlx/prefix_cache.py,vllm_mlx/routes/health.py(+530/-73).Verification (on
start-qmlx.shverbatim: force-spec-decode, MTP K=2, disk-kv-restore)(1,64,128,128)arrays climb 72 -> 396, bf16 KV(1,2,N,256)48 -> 248, monotonic.Caveat: PFlash compressed the synthetic test prompts, so the largest restore-hit exercised under the fix was moderate-size. The retention mechanism removed is size-independent, so it is expected to hold at 192K, but that is not directly proven. Adversarial review recommended before merge.