feat: process-wide model weight cache (v0.1.5) - #22
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved cache concurrency and cleanup defects, plus stale downstream lock metadata, block approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a process-wide LRU cache for locally loaded model weights, with reuse controls, cleanup APIs, tests, documentation, and v0.1.5 release metadata.
Changes:
- Adds shared model caching and configuration.
- Integrates caching across supported forecasters.
- Updates tests, documentation, changelogs, and package versioning.
File summaries
| File | Summary |
|---|---|
uv.lock |
Updates lock metadata; critical (1 vote): downstream GIFT-Eval lock metadata remains stale. |
tests/helpers.py |
Updates test forecasters for cache options. |
tests/core/test_model_weight_cache.py |
Adds cache behavior and cleanup tests. |
pyproject.toml |
Bumps version to 0.1.5; critical (2 votes): GIFT-Eval frozen installation may reject stale metadata. |
mkdocs.yml |
Adds cache documentation navigation. |
foundationforecast/models/toto.py |
Integrates Toto caching; moderate (1 vote): cleanup can trigger network I/O for unforecast remote models. |
foundationforecast/models/tirex.py |
Integrates TiRex model caching. |
foundationforecast/models/timesfm.py |
Integrates TimesFM caching; moderate (1 vote): prediction_length causes unnecessary cache misses. |
foundationforecast/models/timegpt.py |
Integrates TimeGPT caching. |
foundationforecast/models/tafsut.py |
Integrates TAFSUT caching. |
foundationforecast/models/tabpfn.py |
Integrates TabPFN caching. |
foundationforecast/models/t0.py |
Integrates T0 caching. |
foundationforecast/models/sundial.py |
Integrates Sundial caching. |
foundationforecast/models/patchtst_fm.py |
Integrates PatchTST-FM caching. |
foundationforecast/models/moirai.py |
Integrates Moirai caching. |
foundationforecast/models/flowstate.py |
Integrates FlowState caching. |
foundationforecast/models/chronos.py |
Integrates Chronos caching. |
foundationforecast/core/model_weight_cache.py |
Implements the LRU cache; critical (2 votes): cache operations are unsynchronized; moderate (1 vote): zero capacity bypasses cleanup. |
foundationforecast/core/gluonts_forecaster.py |
Adds cached predictor support. |
foundationforecast/core/forecaster.py |
Adds shared cache APIs. |
foundationforecast/core/cached_forecaster.py |
Provides cache context helpers; moderate (1 vote): zero-capacity loads bypass release_model. |
foundationforecast/_foundation_forecast.py |
Adds cleanup integration; moderate (2 votes): fallback_model is excluded from cleanup. |
foundationforecast/__init__.py |
Exports cache configuration. |
docs/model-weight-cache.md |
Documents cache usage and configuration. |
docs/model-hub.md |
Links to cache documentation. |
docs/changelogs/v0.1.5.md |
Adds v0.1.5 release notes. |
docs/changelogs/index.md |
Registers the new changelog. |
docs/api/models/utils/forecaster.md |
Documents forecaster cache APIs. |
Review details
Suppressed comments (7)
foundationforecast/core/cached_forecaster.py:28
- When
max_cached_modelsis 0, this returns a freshly loaded model directly, bypassing thefinallyincached_model_context;set_max_cached_models(0)therefore never callsrelease_modelfor forecast-created models. That leaves the CUDA/MPS allocator cleanup path unused, contrary to the documented “always load + release” behavior. Handle zero-capacity as an uncached load with the same try/finally release path.
yield get_model_weight_cache().get_or_load(cache_key, loader)
foundationforecast/core/model_weight_cache.py:38
- When the cache is full, the new model is loaded before the least-recently-used model is evicted. With the default capacity of 1, switching models therefore keeps both checkpoints resident during
loader(), so loading a second large GPU model can OOM before the eviction runs. Evict and release the old entry before loading a replacement (while preserving the old entry if the load fails, if that behavior is required).
model = loader()
self._cache[key] = model
while len(self._cache) > self._max_cached_models:
_, evicted = self._cache.popitem(last=False)
release_model(evicted)
foundationforecast/core/model_weight_cache.py:64
del modelonly deletes the local parameter inrelease_model; it cannot drop the caller'smodel/evictedreference. The subsequentempty_cache()calls therefore run while the model is still held (andclear()still retains entries until after its loop), so eviction andclear_model_cache()may not actually release GPU allocations as documented. Drop the owning references before invoking the device-cache cleanup.
def release_model(model: Any) -> None:
del model
try:
import torch
foundationforecast/core/model_weight_cache.py:54
clear_prefix()uses an unboundedstartswith, so clearing a forecaster whose prefix isT0:repoalso clears cached keys such asT0:repo2. This can makeclear_model_cache()evict an unrelated forecaster's weights; match the prefix exactly or require the separator after it.
keys = [key for key in self._cache if key.startswith(prefix)]
for key in keys:
self.clear(key)
foundationforecast/core/model_weight_cache.py:30
- When
max_cached_models == 0, this returns a freshly loaded model without placing it in the cache. However,cached_model_context()still takes its cached branch forreuse_loaded_model=True, so it never enters thefinally: release_model(...)path; the documented zero-cache mode therefore bypasses model cleanup. Treat zero capacity as the non-cached context path and add a regression test.
def get_or_load(self, key: str, loader: Callable[[], T]) -> T:
if self._max_cached_models == 0:
return loader()
foundationforecast/models/timesfm.py:310
_load_predictor()and itsModelConfigdo not useprediction_length, so including it in this cache key creates a separate evaluator for every horizon. With the default one-entry LRU, alternatinghvalues reloads the same TimesFM 3 checkpoint each time; omit this argument from the v3 key (or cache only the underlying weights).
def _model_cache_key(self, prediction_length: int) -> str:
kwargs_key = tuple(sorted((self.kwargs or {}).items()))
return (
f"{self._model_cache_prefix()}:"
f"{self.context_length}:{self.batch_size}:"
f"{prediction_length}:{kwargs_key}"
foundationforecast/models/toto.py:152
clear_model_cache()calls_model_cache_prefix(), soFoundationForecast(clean_cache=True)invokes_is_toto2()for Toto instances that have not forecast yet. For a remote repo this performshf_hub_download(config.json), making cleanup perform network I/O and potentially fail before that model is run. Keep prefix resolution side-effect-free or clear only keys that have already been created.
def _model_cache_prefix(self) -> str | None:
version = "v2" if self._is_toto2() else "v1"
return f"{type(self).__qualname__}:{self.repo_id}:{version}"
- Files reviewed: 27/28 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical issues can permit duplicate concurrent checkpoint loads and retain evicted weights during device-cache cleanup, risking out-of-memory failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
foundationforecast/core/model_weight_cache.py:97
- The singleton is initialized with an unsynchronized check-and-create. If the first forecasts arrive concurrently, two
ModelWeightCacheinstances can be constructed and the later assignment can discard the first, so requests may load duplicate checkpoints and the process-wide cache loses one. Protect initialization with a module-level lock or initialize the cache eagerly.
if _model_weight_cache is None:
_model_weight_cache = ModelWeightCache()
return _model_weight_cache
- Files reviewed: 29/31 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved concurrency, eviction, and failure-path cleanup issues may cause duplicate loads or retained GPU memory.
Review details
Suppressed comments (5)
foundationforecast/core/model_weight_cache.py:112
- This clear path only removes entries already in
_cache; an in-flight load is neither cancelled nor invalidated. Ifclear_model_cache()runs while another thread is loading the same key, that loader inserts the model afterclear()returns, leaving weights cached despite the clear request. Coordinate clearing with_inflightloads or use a generation/invalidation check.
def clear(self, key: str | None = None) -> None:
with self._lock:
if key is None:
models = list(self._cache.values())
self._cache.clear()
foundationforecast/core/model_weight_cache.py:148
release_model()only deletes its helper parameter, while callers still retain the object in themodels/evictedlists (and the non-cached context manager still has its local)._empty_device_cache()therefore runs before the tensors become unreachable, so an LRU eviction orclear_model_cache()can return without actually flushing freed GPU blocks. Drop the owning references before performing device cleanup, or move cleanup out of this helper.
def release_model(model: Any) -> None:
del model
_empty_device_cache()
foundationforecast/core/model_weight_cache.py:106
- A waiter only rechecks
_cacheafter the event is signaled. Withmax_cached_models=1, another key can finish between the original load's insertion and this lock acquisition, evicting this key; the waiter then loops and invokesloader()a second time, so concurrent requests for the same key can load duplicate models under contention. Returninflight.modelwhen the load completed before retrying the cache lookup.
cached = self._cache.get(key)
if cached is not None:
self._cache.move_to_end(key)
return cached
foundationforecast/core/model_weight_cache.py:68
- The LRU entry is evicted and released before
loader()runs. If the new checkpoint download or initialization raises, the previously warm model is lost even though no replacement was loaded, so the next request must reload it. Defer eviction until the successful insertion path below (or restore the evicted entries when loading fails) to preserve a usable cache on transient load errors.
evicted = self._evict_if_needed_locked()
inflight = _InflightLoad()
self._inflight[key] = inflight
is_loader = True
else:
is_loader = False
for model in evicted:
release_model(model)
foundationforecast/core/multi_model.py:30
- With the shared cache, a model that raises before
res_df_modelis produced keeps its successfully loaded weights cached, but_call_modelsonly invokes_clean_model_cache()after the try/merge block. Thusclean_cache=Truedoes not release weights when a primary model fails without a fallback (or when the fallback also fails), so an unsuccessful multi-model run can retain GPU memory. Put the cleanup in afinallyfor each model attempt while preserving fallback handling.
models_to_clear = list(self.models)
if self.fallback_model is not None:
models_to_clear.append(self.fallback_model)
for model in models_to_clear:
- Files reviewed: 29/31 changed files
- Comments generated: 0 new
- Review effort level: Lite
Reuse loaded checkpoint weights across repeated forecast() calls via a global LRU cache (default max_cached_models=1). All local weight-loading models accept reuse_loaded_model and expose clear_model_cache(). Co-authored-by: Cursor <cursoragent@cursor.com>
Document reuse_loaded_model, set_max_cached_models(), clear_model_cache(), and FoundationForecast clean_cache for long-lived GPU workers. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Add thread-safe LRU eviction before load, bounded prefix cleanup, and zero-capacity release paths. Include fallback models in clean_cache, drop Toto/TimesFM v3 cache-key pitfalls, and refresh the editable gift-eval lock metadata so CI benchmarks run against current source. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Use per-key in-flight loading so concurrent misses share one loader, evict outside the lock before loading, and protect cache singleton creation with double-checked locking. Co-authored-by: Cursor <cursoragent@cursor.com>
Document PR #23 after rebasing feat/model-weight-cache onto main. Co-authored-by: Cursor <cursoragent@cursor.com>
fdd86c9 to
1a7cb85
Compare
Fix pre-commit toml-sort-fix failure after rebasing onto main. Co-authored-by: Cursor <cursoragent@cursor.com>
Merging this PR will improve performance by ×2.6
Performance Changes
Tip Curious why performance improved? Comment Comparing |
|
@codspeedbot explain why performance improved |
Why performance improvedShort answer: the benchmarks are no longer paying a checkpoint load on every What changed in the measured pathOn model = cls.from_pretrained(self.repo_id, device_map=..., torch_dtype=self.dtype)
try:
yield model
finally:
del model
torch.cuda.empty_cache()Since Why that shows up so strongly in the benchmarks
The ranking matches load weight vs. inference weight:
Two things worth noting
No code changes were needed for this request, so the branch is untouched. Flamegraphs were not available for these walltime benchmarks (callgraph generation reported a missing root frame), so the above is based on the walltime distributions and the diff of the measured code path. Summary
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot. |
Summary
max_cached_models=1) so local weight-loading forecasters reuse loaded checkpoints across repeatedforecast()calls instead of reloading on every request.Forecaster._cached_model()helper; every forecaster acceptsreuse_loaded_modeland exposesclear_model_cache().set_max_cached_models()from the package root;FoundationForecast(clean_cache=True)clears cached weights after each model in multi-model runs.Test plan
uv run pytest tests/core/test_model_weight_cache.py tests/core/ tests/test_foundation_forecast.pyuv run ruff check foundationforecastv0.1.5onmainto trigger PyPI release workflowMade with Cursor