fix(server): bound resident model memory with LRU eviction and idle TTL sweeper - #913
fix(server): bound resident model memory with LRU eviction and idle TTL sweeper#913rkfshakti wants to merge 2 commits into
Conversation
|
Friendly ping on this one — it fixes the ModelProvider memory leak reported in #835 (LRU eviction + idle TTL sweeper), and all CI is green on the current head. It's been quiet for about two weeks; is there anything else you'd like changed, or could someone take a look when convenient? Happy to rebase on latest main if that helps. |
Lazarus-931
left a comment
There was a problem hiding this comment.
hi @rkfshakti, i appreciate the pr!
I think this can be better designed, specifically _evict(), I think adding gc.collect() before mx.clear_cache() might solve the leak issue :)
| with self._lock: | ||
| return list(self.models.keys()) | ||
|
|
||
| def _evict(self, model_name: str) -> None: |
There was a problem hiding this comment.
i think dropping the reference does not free the weights, since nn.Module has reference cycles
| assert handle.cancelled | ||
|
|
||
|
|
||
| # -- ModelProvider memory eviction ------------------------------------------ |
There was a problem hiding this comment.
universally, we do small weights for testing.
these tests don't really reflect that your change fixes the leak since this is not even calling nn.Module
I'd say tests can be improved as well
5f526f9 to
f4a4a89
Compare
|
Thanks @Lazarus-931 — agreed, and applied. While adding the regression test for the ordering, the extra 8/8 model-provider tests green locally. Re-review when you have a moment. 🙏 |
Lazarus-931
left a comment
There was a problem hiding this comment.
small fix, otherwise gtg :)
| | `--realtime-transcription-delay-ms` | `null` | Transcription latency/quality knob for models that support it (e.g. `voxtral_realtime`) | | ||
| | `--vad-model` | `mlx-community/silero-vad` | Streaming VAD model used for server-side turn detection (`server_vad`) on `/v1/realtime` | | ||
| | `--tts-max-batch-size` | `8` | Maximum compatible TTS speech requests per continuous batch session | | ||
| | `--max-resident-models` | `1` | Max models kept loaded at once; loading another evicts the least-recently-used (LRU) | |
There was a problem hiding this comment.
shouldn't max-resident-models when 1, cause stt and tts adapters to load through same model_provider via _load_model_for_inference, so we'd have, which we don't want a server handling both.
i think it needs to be 0 by default
…TL sweeper ModelProvider caches every loaded model forever, so multi-voice/multi-model servers accumulate each distinct model's weights in GPU memory until OOM (fixes Blaizzy#835). Add: - LRU eviction: loading beyond --max-resident-models (default 1) evicts the least-recently-used model and runs mx.clear_cache(). - Idle TTL: --model-idle-ttl-seconds unloads models unused for N seconds via a background sweeper started/stopped by the app lifespan. - Thread-safe: load_model can run in FastAPI threadpool workers while the sweeper runs on the event loop, so the provider now guards its dict with a threading.Lock. - CLI flags + MLX_AUDIO_MAX_RESIDENT_MODELS / MLX_AUDIO_MODEL_IDLE_TTL_SECONDS env overrides; docs updated. Eviction now runs gc.collect() before mx.clear_cache() so cyclical model/processor references are freed before the MLX buffer cache is cleared (review feedback from @Lazarus-931). The sweeper's stale-reaping is extracted into a deterministic evict_stale(now) method. 9 new unit tests cover LRU eviction, capacity clamping, remove_model memory release, gc-before-clear ordering, and the idle sweeper.
f4a4a89 to
718e7d3
Compare
Lazarus-931 review on Blaizzy#913: with default 1, a server mixing STT and TTS adapters thrashes the shared provider (each load evicts the other's model). Make 0 = unbounded (no LRU eviction) the default; >= 1 enables the LRU. Replaces the clamp-to-1 test with an unbounded-set regression test; docs updated to explain the mixed-adapter rationale.
|
Thanks @Lazarus-931 — applied in dc2bd44. |
|
Run the following from the repository root, then commit and push any changes: python -m pip install pre-commit
pre-commit run --all-filesThe project's pre-commit configuration includes Black and the other required formatters. |
Context
Multi-voice / multi-model servers leak GPU memory until OOM:
ModelProvider(mlx_audio/server.py) caches every loaded model in an unbounded dict and nothing ever evicts it. Three voice switches in the repro add ~35GB of permanently-resident memory;DELETE /v1/modelsworks but nothing calls it automatically.Closes #835
Description
Adds two complementary, configurable eviction policies to
ModelProvider:--max-resident-models(default 1) evicts the least-recently-used model and callsmx.clear_cache()to release its weights.--model-idle-ttl-seconds(default 0 = off) unloads models unused for that long via a background sweeper started/stopped by the FastAPI lifespan.Both are also settable via
MLX_AUDIO_MAX_RESIDENT_MODELSandMLX_AUDIO_MODEL_IDLE_TTL_SECONDS; CLI flags take precedence.Changes in the codebase
ModelProvidernow tracks per-model last-use timestamps, guards its dict with athreading.Lock(safe against FastAPI threadpool workers racing the event-loop sweeper), and exposes_evict/_evict_least_recently_used/start_sweeper/stop_sweeper.app_lifespanstarts/stops the sweeper so the task is cancelled on shutdown.main()gains--max-resident-modelsand--model-idle-ttl-seconds; applied directly to the import-time provider instance.Changes outside the codebase
None.
Additional information
max_resident_models=1and TTL 0, a single-model server behaves exactly as before (its one model is never evicted).Checklist