Repository navigation
evict unused models - #168
Merged
Merged
Conversation
…luding gated models and their user copies
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Eviction can race concurrent launches and delete active downloads or prepared model weights because launch preparation and deletion lack shared per-model synchronization.
4 open issues
What changed in this PR
Moves unused-model eviction from a standalone script into the backend’s background service, with configurable scheduling and dry-run behavior.
Changes:
- Schedules eviction at startup and periodically, with dry run enabled by default.
- Adds gated-model and user-copy cleanup, completion-marker invalidation, and safer compile-cache deletion.
- Replaces script-focused tests and documentation with coverage and guidance for the background service.
| File | Description |
|---|---|
CHANGELOG.md |
Documents eviction behavior and cleanup changes. |
backend/tests/test_model_eviction.py |
Removes the script-focused tests. |
backend/tests/test_cache_management_service.py |
Adds service, cleanup, safety, and scheduling tests. |
backend/scripts/README.md |
Removes obsolete eviction-script instructions. |
backend/scripts/evict_unused_models.py |
Removes the standalone eviction script. |
backend/README.md |
Documents background eviction and configuration. |
backend/app/utils/llm_inference.py |
Clarifies completion-marker behavior in a comment. |
backend/app/services/cache_management_service.py |
Implements public, gated, and compile-cache cleanup. |
backend/app/services/background_service.py |
Schedules eviction in a worker thread. |
backend/app/config/config.py |
Adds eviction settings and interval validation. |
backend/.env.example |
Adds sample eviction configuration. |
Review effort: Balanced
|
|
||
| failed: List[str] = [] | ||
| if not dry_run: | ||
| failed = [repo.repo_id for repo in to_delete if not _unmark_complete(repo)] |
| path.name, | ||
| launched_at.date(), | ||
| ) | ||
| _delete_user_copies(path.name, dry_run) |
| shutil.rmtree(path, ignore_errors=True) | ||
| continue | ||
|
|
||
| launched_at = last_launched.get(path.name) |
…d the store entry is kept
Ralhazmy1
marked this pull request as ready for review
November 5, 2026 14:59
…documented in .env.example.
…l_eviction # Conflicts: # CHANGELOG.md
joshfactorial
requested changes
Nov 5, 2026
joshfactorial
left a comment
joshfactorial
left a comment
Collaborator
There was a problem hiding this comment.
I agree with the two open findings below and recommend fixing them before merge:
- Serialize public and gated model eviction with download and launch preparation, and recheck eligibility while holding the shared per-model claim. Otherwise a launch can start after the eviction check and have its files removed during preparation or download.
- Handle nested gated model IDs using the full store-relative ID. The current top-level directory scan won't match IDs containing
/, so those models and their user copies won't be evicted.
The cleanup-failure concern appears addressed in the current head: if user-copy deletion fails, the store entry is retained for retry.
joshfactorial
approved these changes
Nov 9, 2026
joshfactorial
left a comment
joshfactorial
left a comment
Collaborator
There was a problem hiding this comment.
After discussions, Raghid and I agreed that these issues were a minor/very rare concern, better served by a follow-up ticket (filed as #182).
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.
What
Moves model cache eviction out of the cron script (
scripts/evict_unused_models.py, now removed) and into the backend's background service.MODEL_EVICTION_INTERVALseconds (default 1 day).MODEL_EVICTION_MAX_AGE_DAYS(default 90), based onModelDeploymenthistory. Models with no launch history are never deleted.MODEL_STORE_ROOT, along with users' hard-linked copies in<workspace>/<user>/model-weights/. If a user copy can't be deleted, the store entry is kept and retried next run.COMPILE_CACHE_DIR) by file age, without following symlinks.MODEL_EVICTION_DRY_RUNdefaults totrue(log only).Deploying
Set
MODEL_EVICTION_DRY_RUN=falseHow to test
As
svcdeltallmhub, frombackend/:Dry run on the real folders (deletes nothing):
MODEL_EVICTION_MAX_AGE_DAYS=30 .venv/bin/uvicorn app.main:app --port 8005After startup, check the log for
Would evict …lines andEviction complete … (dry run, nothing deleted).Real delete in a sandbox:
store/<model>/(a model your DB launched more than 30 days ago) and a hard link to its file inws/<user>/model-weights/<model>/.MODEL_CACHE_DIR=<test>/hf MODEL_STORE_ROOT=<test>/store VEC_INF_SHARED_WORK_ROOT=<test>/ws COMPILE_CACHE_DIR= MODEL_EVICTION_MAX_AGE_DAYS=30 MODEL_EVICTION_DRY_RUN=false.Gated model cleanup … evicted=1 failed=0, and both copies gone from the test folder.Tested on dt-svc-llmaas01: both steps passed. Unit tests:
pytest tests/(189 passed).