Studio: opt-in OpenAI /v1 model auto-switch and idle keep-warm#6392
Conversation
The OpenAI-compatible endpoints serve whichever GGUF is loaded and ignore the request model field, so an OpenAI client that changes model never reloads. Add an opt-in setting that, when a /v1 request names a downloaded local GGUF different from the loaded one, loads it before serving by reusing the existing /load path (its dedup, tensor fallback, and threading apply). Unknown names still serve the loaded model, so drop-in compatibility is preserved and no remote download is triggered. Also add an optional idle auto-unload (TTL keep-warm): a pure-ASGI middleware tracks in-flight inference requests so a stream is never unloaded mid-response, and a lifespan loop unloads the model after the configured idle seconds. Both settings default off and live in the app_settings store, exposed via GET/PUT /api/settings/openai-auto-switch.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
for more information, see https://pre-commit.ci
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces opt-in controls for OpenAI-compatible model auto-switching and idle auto-unloading (TTL keep-warm) for local llama.cpp models. Feedback highlights several critical improvements: ensuring that newly loaded or changed models reset the activity timer to prevent premature unloading, fixing model resolution on Windows where drive letter colons interfere with variant partitioning, checking for string types during index building to avoid potential AttributeError exceptions, and comparing model variants in _maybe_auto_switch_model to allow switching between different quantizations of the same model.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def _is_idle(ttl_seconds: float) -> bool: | ||
| with _lock: | ||
| return _inflight == 0 and (time.monotonic() - _last_active) >= ttl_seconds |
There was a problem hiding this comment.
Add a helper function reset_activity_timer() to reset the _last_active timestamp when a model is newly loaded or changed. This is critical because _last_active is otherwise only updated during active inference requests. If a model is loaded after the server has been running or idle for longer than auto_unload_idle_seconds, the keep-warm loop will immediately unload the newly loaded model on its next iteration.
| def _is_idle(ttl_seconds: float) -> bool: | |
| with _lock: | |
| return _inflight == 0 and (time.monotonic() - _last_active) >= ttl_seconds | |
| def _is_idle(ttl_seconds: float) -> bool: | |
| with _lock: | |
| return _inflight == 0 and (time.monotonic() - _last_active) >= ttl_seconds | |
| def reset_activity_timer() -> None: | |
| global _last_active | |
| with _lock: | |
| _last_active = time.monotonic() |
|
|
||
| async def idle_unload_loop(poll_seconds: float = 15.0) -> None: | ||
| """Unload the loaded GGUF once idle past the configured TTL. Inert when off.""" | ||
| from utils.openai_auto_switch_settings import get_auto_unload_idle_seconds | ||
| while True: | ||
| await asyncio.sleep(poll_seconds) | ||
| try: | ||
| ttl = get_auto_unload_idle_seconds() | ||
| if ttl <= 0 or not _is_idle(ttl): | ||
| continue | ||
| from routes.inference import get_llama_cpp_backend | ||
|
|
||
| backend = get_llama_cpp_backend() | ||
| # Re-check idle just before unload to shrink the arrival race window. | ||
| if backend.is_loaded and _is_idle(ttl): | ||
| await asyncio.to_thread(backend.unload_model) | ||
| logger.info("Idle auto-unload: freed GGUF after %ss idle", ttl) | ||
| except Exception as exc: |
There was a problem hiding this comment.
Update idle_unload_loop to track model load transitions and reset the activity timer using reset_activity_timer(). This ensures that newly loaded or changed models are not immediately unloaded if the idle TTL has already elapsed since the last request or server startup.
async def idle_unload_loop(poll_seconds: float = 15.0) -> None:
"""Unload the loaded GGUF once idle past the configured TTL. Inert when off."""
from utils.openai_auto_switch_settings import get_auto_unload_idle_seconds
was_loaded = False
last_model = None
while True:
await asyncio.sleep(poll_seconds)
try:
from routes.inference import get_llama_cpp_backend
backend = get_llama_cpp_backend()
# Reset the idle timer when a model is newly loaded or changed
is_loaded = backend.is_loaded
model_id = backend.model_identifier if is_loaded else None
if is_loaded:
if not was_loaded or model_id != last_model:
reset_activity_timer()
was_loaded = True
last_model = model_id
else:
was_loaded = False
last_model = None
ttl = get_auto_unload_idle_seconds()
if ttl <= 0 or not _is_idle(ttl):
continue
# Re-check idle just before unload to shrink the arrival race window.
if backend.is_loaded and _is_idle(ttl):
await asyncio.to_thread(backend.unload_model)
logger.info("Idle auto-unload: freed GGUF after %ss idle", ttl)
was_loaded = False
last_model = None
except Exception as exc:
logger.debug("idle_unload_loop iteration failed: %s", exc)| base, _, variant = requested.strip().partition(":") | ||
| loader_id = _index().get(base.strip().lower()) | ||
| if loader_id is None: | ||
| return None | ||
| return loader_id, (variant.strip() or None) |
There was a problem hiding this comment.
On Windows, absolute paths starting with a drive letter (e.g., C:\models\model.gguf) contain a colon. Using partition(":") splits the string at the drive letter colon, resulting in base being "C" and variant being the rest of the path, which completely breaks model resolution for local paths on Windows.
We can fix this by using rpartition(":") to split from the right, and checking if the partitioned variant part contains path separators (\ or /). If it does, it means the colon was part of a Windows path rather than a variant separator.
| base, _, variant = requested.strip().partition(":") | |
| loader_id = _index().get(base.strip().lower()) | |
| if loader_id is None: | |
| return None | |
| return loader_id, (variant.strip() or None) | |
| if ":" in requested: | |
| base, _, variant = requested.strip().rpartition(":") | |
| if "\\" in variant or "/" in variant: | |
| base = requested.strip() | |
| variant = "" | |
| else: | |
| base = requested.strip() | |
| variant = "" | |
| loader_id = _index().get(base.strip().lower()) | |
| if loader_id is None: | |
| return None | |
| return loader_id, (variant.strip() or None) |
| loaded = backend.model_identifier if backend.is_loaded else None | ||
| if loaded and loaded.lower() == target_id.lower(): | ||
| return | ||
| async with _auto_switch_lock: | ||
| loaded = backend.model_identifier if backend.is_loaded else None | ||
| if loaded and loaded.lower() == target_id.lower(): | ||
| return |
There was a problem hiding this comment.
The _maybe_auto_switch_model function only compares the base model identifier (loaded.lower() == target_id.lower()) and completely ignores the requested variant. If a user requests a different variant of the currently loaded model (e.g., switching from Q4_K_M to Q8_0), the auto-switch check will falsely assume the model is already loaded and skip loading the new variant.
We should also compare the loaded variant (backend.hf_variant) with the requested variant.
loaded = backend.model_identifier if backend.is_loaded else None
loaded_variant = backend.hf_variant if backend.is_loaded else None
if loaded and loaded.lower() == target_id.lower():
if not variant or (loaded_variant and loaded_variant.lower() == variant.lower()):
return
async with _auto_switch_lock:
loaded = backend.model_identifier if backend.is_loaded else None
loaded_variant = backend.hf_variant if backend.is_loaded else None
if loaded and loaded.lower() == target_id.lower():
if not variant or (loaded_variant and loaded_variant.lower() == variant.lower()):
returnReferences
- Ensure GGUF variant lookups are case-insensitive by lowercasing both the keys built from on-disk filenames and the lookup keys (e.g., variant.quant.lower()) to prevent detection failures due to casing mismatches.
| for key in (info.id, getattr(info, "model_id", None), getattr(info, "display_name", None)): | ||
| if key: | ||
| index.setdefault(key.strip().lower(), loader_id) |
There was a problem hiding this comment.
Defensively verify that key is a string before calling .strip().lower(). If info.id, model_id, or display_name is of an unexpected type (such as None or an integer), calling string methods on it will raise an AttributeError and crash the index building process.
| for key in (info.id, getattr(info, "model_id", None), getattr(info, "display_name", None)): | |
| if key: | |
| index.setdefault(key.strip().lower(), loader_id) | |
| for key in (info.id, getattr(info, "model_id", None), getattr(info, "display_name", None)): | |
| if isinstance(key, str): | |
| index.setdefault(key.strip().lower(), loader_id) |
…load stamp Follow-ups from review of the opt-in OpenAI auto-switch path: 1. Variant-aware dedup. _maybe_auto_switch_model compared only the repo id, so requesting another quant of the loaded repo (e.g. Q4_K_M loaded, Q8_0 asked) was served by the old quant. Compare hf_variant too, matching /load dedup. 2. Streaming /v1/responses now calls the auto-switch hook. It went straight into _responses_stream and only checked is_loaded, so stream=True could serve the old model or 400. Non-streaming already routed through chat completions; the hook is idempotent once loaded. 3. resolve_local_gguf tries an exact id match before splitting a trailing :VARIANT, so local ids that contain a colon (e.g. a Windows path) resolve instead of being cut at the drive letter. 4. Idle keep-warm stamps activity on a load/swap transition. _last_active was only refreshed by inference requests, so a model loaded after the server sat idle past the TTL could be unloaded before its first request. Tests cover each case.
for more information, see https://pre-commit.ci
|
Thanks for the review. Pushed follow-ups in 98e8de4:
On the |
The new streaming-responses test passed in isolation but failed under the CI's randomized collection order with "object has no attribute 'state'": it passed a bare object() as the request and stubbed only one dispatcher, so an ordering where the real dispatcher ran hit request.state. Give the request a state and stub both dispatchers; the test still asserts the hook fires before dispatch.
for more information, see https://pre-commit.ci
…time The behavioral version executed openai_responses and relied on stubbing its callees, which a randomized collection order in CI could defeat (the real dispatcher ran and hit request attributes). Assert on the function source that the hook precedes both dispatchers instead; the hook's runtime behavior is already covered by the direct _maybe_auto_switch_model tests.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces opt-in idle auto-unload (TTL keep-warm) and automatic model switching for local llama.cpp GGUF models, allowing the backend to transparently swap models based on incoming OpenAI-compatible requests and free VRAM when idle. The feedback focuses on preventing event loop starvation by making the local model index resolution asynchronous, as it currently performs synchronous disk I/O on the request hot path. Additionally, the reviewer recommends restricting the keep-warm middleware to POST requests to avoid CORS preflight requests resetting the idle timer, defensively validating that the requested model is a string, and updating the test suite to support these asynchronous changes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def _index() -> dict[str, str]: | ||
| global _scan | ||
| now = time.monotonic() | ||
| with _lock: | ||
| ts, cached = _scan | ||
| if now - ts < _CACHE_TTL_S: | ||
| return cached | ||
| fresh = _build_index() | ||
| with _lock: | ||
| _scan = (now, fresh) | ||
| return fresh |
There was a problem hiding this comment.
Building the local model index (_build_index) performs synchronous disk I/O (_scan_models_dir and _scan_hf_cache) which can be slow depending on the size of the Hugging Face cache. Since this is called on the hot path of every incoming chat completion request, doing this synchronously on the main thread will block the ASGI event loop and starve concurrent requests. Making _index asynchronous and offloading the blocking disk scan to a thread pool using asyncio.to_thread prevents event loop starvation.
| def _index() -> dict[str, str]: | |
| global _scan | |
| now = time.monotonic() | |
| with _lock: | |
| ts, cached = _scan | |
| if now - ts < _CACHE_TTL_S: | |
| return cached | |
| fresh = _build_index() | |
| with _lock: | |
| _scan = (now, fresh) | |
| return fresh | |
| async def _index() -> dict[str, str]: | |
| global _scan | |
| now = time.monotonic() | |
| with _lock: | |
| ts, cached = _scan | |
| if now - ts < _CACHE_TTL_S: | |
| return cached | |
| import asyncio | |
| fresh = await asyncio.to_thread(_build_index) | |
| with _lock: | |
| _scan = (now, fresh) | |
| return fresh |
| def resolve_local_gguf(requested: str) -> Optional[tuple[str, Optional[str]]]: | ||
| """Return ``(loader_id, gguf_variant)`` for a downloaded local match, else None. | ||
|
|
||
| ``requested`` may be ``repo`` or ``repo:VARIANT``. An exact id match wins | ||
| first so ids that themselves contain a colon (e.g. a Windows path) still | ||
| resolve; only then is a trailing ``:VARIANT`` split off the last colon. | ||
| """ | ||
| if not requested or not requested.strip(): | ||
| return None | ||
| requested = requested.strip() | ||
| index = _index() | ||
| loader_id = index.get(requested.lower()) | ||
| if loader_id is not None: | ||
| return loader_id, None | ||
| base, sep, variant = requested.rpartition(":") | ||
| if not sep: | ||
| return None | ||
| loader_id = index.get(base.strip().lower()) | ||
| if loader_id is None: | ||
| return None | ||
| return loader_id, (variant.strip() or None) |
There was a problem hiding this comment.
To support the asynchronous index building, resolve_local_gguf should be made asynchronous and await the _index() call. Additionally, since the requested model parameter is parsed directly from untrusted client JSON payloads (e.g., in /v1/completions), we should defensively verify that requested is indeed a string using isinstance(requested, str) to prevent AttributeError crashes if a non-string value is passed.
async def resolve_local_gguf(requested: str) -> Optional[tuple[str, Optional[str]]]:
"""Return ``(loader_id, gguf_variant)`` for a downloaded local match, else None.
``requested`` may be ``repo`` or ``repo:VARIANT``. An exact id match wins
first so ids that themselves contain a colon (e.g. a Windows path) still
resolve; only then is a trailing ``:VARIANT`` split off the last colon.
"""
if not isinstance(requested, str) or not requested.strip():
return None
requested = requested.strip()
index = await _index()
loader_id = index.get(requested.lower())
if loader_id is not None:
return loader_id, None
base, sep, variant = requested.rpartition(":")
if not sep:
return None
loader_id = index.get(base.strip().lower())
if loader_id is None:
return None
return loader_id, (variant.strip() or None)| if not requested_model or not get_openai_auto_switch_enabled(): | ||
| return | ||
| resolved = resolve_local_gguf(requested_model) |
There was a problem hiding this comment.
Since resolve_local_gguf has been updated to be asynchronous to avoid blocking the event loop, it must be awaited here.
| if not requested_model or not get_openai_auto_switch_enabled(): | |
| return | |
| resolved = resolve_local_gguf(requested_model) | |
| if not requested_model or not get_openai_auto_switch_enabled(): | |
| return | |
| resolved = await resolve_local_gguf(requested_model) |
| async def __call__(self, scope, receive, send): | ||
| if scope.get("type") != "http" or not _is_inference_path(scope.get("path", "")): | ||
| await self.app(scope, receive, send) | ||
| return |
There was a problem hiding this comment.
The middleware currently tracks any HTTP request matching the inference path suffixes, including CORS OPTIONS preflight requests. This can prematurely reset the idle keep-warm timer (_last_active) even when no actual inference is performed. Restricting the tracking to POST requests ensures that only actual inference requests keep the model warm.
| async def __call__(self, scope, receive, send): | |
| if scope.get("type") != "http" or not _is_inference_path(scope.get("path", "")): | |
| await self.app(scope, receive, send) | |
| return | |
| async def __call__(self, scope, receive, send): | |
| if ( | |
| scope.get("type") != "http" | |
| or scope.get("method") != "POST" | |
| or not _is_inference_path(scope.get("path", "")) | |
| ): | |
| await self.app(scope, receive, send) | |
| return |
| def _wire(monkeypatch, *, enabled, resolves_to, backend, recorder): | ||
| monkeypatch.setattr(settings, "get_openai_auto_switch_enabled", lambda: enabled) | ||
| monkeypatch.setattr(resolver, "resolve_local_gguf", lambda _m: resolves_to) | ||
| monkeypatch.setattr(inference_route, "get_llama_cpp_backend", lambda: backend) | ||
| monkeypatch.setattr(inference_route, "load_model", recorder) |
There was a problem hiding this comment.
Since resolve_local_gguf is now an asynchronous function, the mock in _wire must return a coroutine instead of a synchronous value to prevent TypeError when awaited.
| def _wire(monkeypatch, *, enabled, resolves_to, backend, recorder): | |
| monkeypatch.setattr(settings, "get_openai_auto_switch_enabled", lambda: enabled) | |
| monkeypatch.setattr(resolver, "resolve_local_gguf", lambda _m: resolves_to) | |
| monkeypatch.setattr(inference_route, "get_llama_cpp_backend", lambda: backend) | |
| monkeypatch.setattr(inference_route, "load_model", recorder) | |
| def _wire(monkeypatch, *, enabled, resolves_to, backend, recorder): | |
| async def mock_resolve(_m): | |
| return resolves_to | |
| monkeypatch.setattr(settings, "get_openai_auto_switch_enabled", lambda: enabled) | |
| monkeypatch.setattr(resolver, "resolve_local_gguf", mock_resolve) | |
| monkeypatch.setattr(inference_route, "get_llama_cpp_backend", lambda: backend) | |
| monkeypatch.setattr(inference_route, "load_model", recorder) |
| def test_resolver_matches_and_splits_variant(monkeypatch): | ||
| monkeypatch.setattr(resolver, "_build_index", lambda: {"unsloth/b-gguf": "unsloth/B-GGUF"}) | ||
| resolver._scan = (0.0, {}) # force a rescan | ||
| assert resolver.resolve_local_gguf("unsloth/B-GGUF:UD-Q5_K_XL") == ( | ||
| "unsloth/B-GGUF", | ||
| "UD-Q5_K_XL", | ||
| ) | ||
| assert resolver.resolve_local_gguf("unsloth/B-GGUF") == ("unsloth/B-GGUF", None) | ||
| assert resolver.resolve_local_gguf("totally/unknown") is None | ||
| assert resolver.resolve_local_gguf("") is None | ||
|
|
There was a problem hiding this comment.
Since resolve_local_gguf is now asynchronous, we must run it using asyncio.run to evaluate the assertions correctly.
| def test_resolver_matches_and_splits_variant(monkeypatch): | |
| monkeypatch.setattr(resolver, "_build_index", lambda: {"unsloth/b-gguf": "unsloth/B-GGUF"}) | |
| resolver._scan = (0.0, {}) # force a rescan | |
| assert resolver.resolve_local_gguf("unsloth/B-GGUF:UD-Q5_K_XL") == ( | |
| "unsloth/B-GGUF", | |
| "UD-Q5_K_XL", | |
| ) | |
| assert resolver.resolve_local_gguf("unsloth/B-GGUF") == ("unsloth/B-GGUF", None) | |
| assert resolver.resolve_local_gguf("totally/unknown") is None | |
| assert resolver.resolve_local_gguf("") is None | |
| def test_resolver_matches_and_splits_variant(monkeypatch): | |
| monkeypatch.setattr(resolver, "_build_index", lambda: {"unsloth/b-gguf": "unsloth/B-GGUF"}) | |
| resolver._scan = (0.0, {}) | |
| assert asyncio.run(resolver.resolve_local_gguf("unsloth/B-GGUF:UD-Q5_K_XL")) == ( | |
| "unsloth/B-GGUF", | |
| "UD-Q5_K_XL", | |
| ) | |
| assert asyncio.run(resolver.resolve_local_gguf("unsloth/B-GGUF")) == ("unsloth/B-GGUF", None) | |
| assert asyncio.run(resolver.resolve_local_gguf("totally/unknown")) is None | |
| assert asyncio.run(resolver.resolve_local_gguf("")) is None |
| def test_resolver_exact_id_with_colon_wins(monkeypatch): | ||
| # A local id that itself contains a colon (e.g. a Windows path) must match | ||
| # exactly rather than being split at the drive-letter colon. | ||
| win = r"C:\models\foo.gguf" | ||
| monkeypatch.setattr(resolver, "_build_index", lambda: {win.lower(): win}) | ||
| resolver._scan = (0.0, {}) | ||
| assert resolver.resolve_local_gguf(win) == (win, None) | ||
|
|
There was a problem hiding this comment.
Since resolve_local_gguf is now asynchronous, we must run it using asyncio.run to evaluate the assertions correctly.
| def test_resolver_exact_id_with_colon_wins(monkeypatch): | |
| # A local id that itself contains a colon (e.g. a Windows path) must match | |
| # exactly rather than being split at the drive-letter colon. | |
| win = r"C:\models\foo.gguf" | |
| monkeypatch.setattr(resolver, "_build_index", lambda: {win.lower(): win}) | |
| resolver._scan = (0.0, {}) | |
| assert resolver.resolve_local_gguf(win) == (win, None) | |
| def test_resolver_exact_id_with_colon_wins(monkeypatch): | |
| # A local id that itself contains a colon (e.g. a Windows path) must match | |
| # exactly rather than being split at the drive-letter colon. | |
| win = r"C:\models\foo.gguf" | |
| monkeypatch.setattr(resolver, "_build_index", lambda: {win.lower(): win}) | |
| resolver._scan = (0.0, {}) | |
| assert asyncio.run(resolver.resolve_local_gguf(win)) == (win, None) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04bf834a48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await load_model( | ||
| LoadRequest(model_path = target_id, gguf_variant = variant), | ||
| fastapi_request, | ||
| current_subject = current_subject, | ||
| ) |
There was a problem hiding this comment.
When auto-switch is enabled, a second /v1 request for a different downloaded model reaches this load_model call even if another /v1/chat/completions, /v1/responses, or /v1/messages stream is still active. The new keep-warm middleware only protects the idle-unload loop; it is not consulted here, and LlamaCppBackend.load_model() kills the existing llama-server process during its phase-1 reload. In that concurrent-stream scenario, the first client can lose its response mid-generation just because another request named a different model, so the swap should wait for or reject while other in-flight inference is using the current server.
Useful? React with 👍 / 👎.
| found = _scan_models_dir(Path("./models").resolve()) + _scan_hf_cache( | ||
| _resolve_hf_cache_dir() | ||
| ) |
There was a problem hiding this comment.
In environments with any Hugging Face safetensors repo already present in the cache, _scan_hf_cache() still returns that repo even when it has no GGUF files, and this resolver indexes it as a resolve_local_gguf() target. A /v1/completions request whose model matches such a cached non-GGUF repo will call the auto-switch hook, unload the currently working GGUF through /load, load the transformers backend instead, and then fail the completions request because no GGUF is loaded; chat requests also silently switch to non-GGUF despite this feature being documented as GGUF-only. The index needs to exclude non-GGUF cache entries before they can trigger auto-switch.
Useful? React with 👍 / 👎.
| if current != seen_model: | ||
| seen_model = current | ||
| if current is not None: | ||
| _note_activity() |
There was a problem hiding this comment.
When idle auto-unload is enabled, /load requests bypass the middleware and this loop only refreshes _last_active when the model identifier changes. If the user reloads the same GGUF after it has been idle longer than the TTL, for example to apply context/cache/template changes, current == seen_model so the fresh server inherits the old idle timestamp and can be unloaded on the next poll before serving a request. Stamp activity on every successful load/reload or track a load generation rather than only identifier changes.
Useful? React with 👍 / 👎.
| loader_id = index.get(base.strip().lower()) | ||
| if loader_id is None: | ||
| return None | ||
| return loader_id, (variant.strip() or None) |
There was a problem hiding this comment.
For a cached HF GGUF repo, this returns any repo:VARIANT as a local match once the base repo is indexed, without checking that the requested variant's GGUF files are actually present in the cache. If only metadata or a different quant is cached, auto-switch then calls /load, and LlamaCppBackend._download_gguf() resolves and downloads the missing multi-GB variant from Hugging Face, contradicting the documented guarantee that auto-switch only uses already-downloaded models. Only return a variant when the corresponding cached GGUF snapshot files exist.
Useful? React with 👍 / 👎.
| _INFERENCE_SUFFIXES = ( | ||
| "/chat/completions", | ||
| "/completions", | ||
| "/messages", | ||
| "/embeddings", |
There was a problem hiding this comment.
This suffix allowlist skips existing llama-server consumers such as /api/inference/audio/generate and /v1/messages/count_tokens; those routes call the loaded GGUF backend but never increment _inflight. When the idle TTL has already elapsed, starting one of those requests does not refresh _last_active, so the background loop can unload the server while audio generation or strict token counting is still running. Add these routes, or track backend use more centrally, before enabling auto-unload.
Useful? React with 👍 / 👎.
… race gate Second-pass review follow-ups on the opt-in auto-switch path: 1. /v1/embeddings now calls the auto-switch hook before the loaded-state check, matching the other model-bearing OpenAI endpoints (the keep-warm middleware already treats embeddings as inference). 2. The resolver index is now GGUF-only. The local-model scanners also surface Transformers/safetensors repos; without a filter, auto-switch could unload the GGUF and route a request into the non-GGUF loader. _has_local_gguf checks a direct .gguf, a models-dir folder, and the HF-cache snapshots layout. 3. Idle keep-warm now holds an asyncio gate across the idle check and the unload, and a request bumps inflight under the same gate, so the loop can no longer unload in the window between "looks idle" and the kill. Tests cover each. Broader local-model source parity (LM Studio, Ollama, legacy caches, custom scan folders) is a follow-up; missing one of those today just falls through to the loaded model.
|
Second review pass follow-ups, pushed in 0ed5fcc:
Tests added for each. On broader local-model source parity (LM Studio, Ollama, legacy/default caches, custom scan folders): the resolver currently indexes |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ed5fccf58
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| def _is_inference_path(path: str) -> bool: | ||
| return path.startswith(_INFERENCE_PREFIXES) and path.endswith(_INFERENCE_SUFFIXES) |
There was a problem hiding this comment.
When auto-unload is enabled, this path-only predicate counts every /v1/chat/completions request as llama activity. However, openai_chat_completions() returns from the provider_id/provider_type branch before touching the local llama_backend, so a client making periodic external-provider calls while a GGUF is loaded keeps _last_active fresh and the idle loop never frees VRAM. Track actual GGUF backend use, or skip provider-routed requests, instead of treating all matching paths as local inference.
Useful? React with 👍 / 👎.
| JSON). | ||
| """ | ||
| llama_backend = get_llama_cpp_backend() | ||
| await _maybe_auto_switch_model(payload.model, request, current_subject) |
There was a problem hiding this comment.
For /v1/messages requests whose model names a cached local GGUF but that omit the required max_tokens, this hook loads/unloads the global GGUF before the handler immediately raises the 400 in the max_tokens check below. A malformed Anthropic request can therefore evict the currently loaded model even though no generation will run; validate the required fields before calling the auto-switch hook.
Useful? React with 👍 / 👎.
| """ | ||
| # Hook here so the streaming path switches too; non-streaming re-checks via | ||
| # openai_chat_completions, which is idempotent once the model is loaded. | ||
| await _maybe_auto_switch_model(payload.model, request, current_subject) |
There was a problem hiding this comment.
For /v1/responses requests whose model resolves to a cached GGUF but whose input is empty or omitted, this hook loads the new model before _normalise_responses_input() and the if not messages 400 below. A bad request can therefore evict the current model and spend load time/VRAM even though generation never starts; move the hook after the no-input validation or into the validated dispatch path.
Useful? React with 👍 / 👎.
| found = _scan_models_dir(Path("./models").resolve()) + _scan_hf_cache( | ||
| _resolve_hf_cache_dir() | ||
| ) |
There was a problem hiding this comment.
This index only scans ./models plus the active HF cache, but the local-model API also adds legacy/default HF caches, LM Studio directories, and custom/Ollama scan folders. When a user requests a GGUF id returned from one of those sources, such as an ollama/... model from a custom folder, resolve_local_gguf() returns None and the request silently falls through to whichever model is already loaded instead of switching to the requested local model. Build the index from the same sources as /api/models/local.
Useful? React with 👍 / 👎.
| return p.suffix.lower() == ".gguf" | ||
| return ( | ||
| next(p.glob("*.gguf"), None) is not None | ||
| or next(p.glob("snapshots/*/*.gguf"), None) is not None |
There was a problem hiding this comment.
This HF-cache check only matches .gguf files directly under snapshots/<rev>/, but the existing GGUF utilities explicitly support cached variants nested under quant subdirectories such as snapshots/<rev>/BF16/...gguf. In that layout, a fully downloaded local GGUF repo is excluded from the auto-switch index, so a /v1 request for its repo id falls through to the currently loaded model instead of switching. Use the same recursive GGUF discovery as list_local_gguf_variants().
Useful? React with 👍 / 👎.
…h coverage Third-pass review follow-ups on the opt-in auto-switch path: 1. The resolver is now variant-aware via list_local_gguf_variants. It indexes only the quants actually on disk, recursing snapshots and quant subdirs such as the nested per-quant folders, so a requested repo:VARIANT resolves only when that quant is local and a bare repo resolves to a concrete local quant. This fixes two gaps: the previous shallow glob rejected nested-variant GGUF repos, and a request for an uncached quant could send /load down the remote download path, breaking the local-only contract. 2. /v1/messages/count_tokens now auto-switches like its sibling /v1/messages, so a count uses the requested model's tokenizer. 3. /api/inference/audio/generate (direct GGUF TTS) is now tracked as in-flight inference, so the idle loop cannot unload the model mid-generation. Tests cover each. Two reviewer items are left as follow-ups: indexing the remaining local sources (LM Studio, Ollama, legacy/default caches, custom scan folders), which fails safe today by falling through to the loaded model; and fully serializing concurrent different-model requests, an inherent limit of the single-slot llama backend that the opt-in feature is not designed around.
|
Pushed Fixed
Each has test coverage in Deferred, with rationale
|
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d05d866497
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if get_auto_unload_idle_seconds() <= 0: | ||
| await self.app(scope, receive, send) | ||
| return |
There was a problem hiding this comment.
When auto-unload is turned on while an inference request that began with TTL=0 is still streaming, this early return means that request never increments _inflight or refreshes _last_active. The background loop starts honoring the new TTL immediately, so if the previous activity timestamp is already older than the TTL it can unload the active llama-server mid-response. Track inference paths regardless of the current setting, or stamp/start before this branch, so toggling the setting cannot kill an already-running request.
Useful? React with 👍 / 👎.
| for key in (info.id, getattr(info, "model_id", None), getattr(info, "display_name", None)): | ||
| if key: | ||
| index.setdefault(key.strip().lower(), entry) |
There was a problem hiding this comment.
When two downloaded GGUF repos share the same basename/display name (for example different orgs publishing Foo-GGUF), this indexes the short display_name and keeps whichever entry was scanned first. A /v1 request using that displayed name can therefore load a different repo than intended, which is easy to miss because the fully-qualified ids are unique but the short key is not; either skip non-unique display names or require the exact id/model_id for auto-switching.
Useful? React with 👍 / 👎.
| variants, _ = list_local_gguf_variants(path) | ||
| quants = tuple(v.quant for v in variants if getattr(v, "quant", None)) | ||
| return _LocalGgufEntry(loader_id, quants) if quants else None |
There was a problem hiding this comment.
For HF GGUF repos that need companion files, this marks the repo switchable as soon as a main quant is present, but the subsequent /load path can still call _download_mmproj() for vision models or _download_mtp() for separate draft heads. If those companion GGUFs are not already cached, a /v1 request can fetch new files from Hugging Face despite the auto-switch guarantee that only already-downloaded models are used; include companion availability in the resolver or suppress companion downloads for this path.
Useful? React with 👍 / 👎.
…itch-model # Conflicts: # studio/backend/main.py # studio/backend/routes/inference.py
…unslothai/unsloth into studio-openai-auto-switch-model
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bf12d55f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return | ||
| # Reuse the load route so its dedup, tensor fallback, and threading apply. | ||
| await load_model( | ||
| LoadRequest(model_path = target_id, gguf_variant = variant), |
There was a problem hiding this comment.
When auto-switch resolves a GGUF from the Hugging Face cache, target_id is the repo id from _scan_hf_cache, but this /load call supplies neither an hf_token nor a cache-local snapshot path. For already-downloaded private/gated GGUF repos, ModelConfig.from_identifier() treats that id as remote and detect_gguf_model_remote() returns None on GatedRepoError before using the local cache, so a /v1 request for a model the resolver just indexed can fail or fall into the non-GGUF path instead of switching. Pass a cache-local GGUF path or otherwise force cached GGUF resolution for resolver hits.
Useful? React with 👍 / 👎.
| entry = index.get(requested.lower()) | ||
| if entry is not None: | ||
| return entry.loader_id, (entry.variants[0] if entry.variants else None) |
There was a problem hiding this comment.
When a cached GGUF repo has multiple quants and the OpenAI client sends the bare repo/display name, this returns entry.variants[0]. Those variants come from list_local_gguf_variants(), which sorts by file size descending, while the normal /load path without a variant uses _pick_best_gguf() with the Q4-oriented quant preference; as a result auto-switch can unexpectedly load BF16/F32 or another largest cached quant and OOM/slow down where the same bare load would choose a smaller preferred quant. Use the existing quant preference here, or leave the variant unset while still preventing downloads.
Useful? React with 👍 / 👎.
… 500
The auto-switch hook calls resolve_local_gguf without its own guard, and
/v1/completions and /v1/embeddings pass body.get("model") through unchanged.
A non-string model (e.g. {"model": 123}) or any internal scan failure would
then raise out of the resolver and turn a request that would otherwise be
served by the loaded model into a 500, breaking the drop-in compatibility the
feature is built on.
Guard the resolver at its boundary: reject non-string input up front and wrap
the lookup so any failure returns None (fall through to the loaded model).
Add regression tests for both paths.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Ran an extended simulation pass over this branch to confirm it solves the real gap (the Found and fixed ( Simulation coverage (77 cases, all green under randomized ordering):
Compatibility notes: both settings default off, so with the feature disabled the request path is byte-for-byte unchanged. No new third-party dependency is introduced (the three new modules are stdlib plus existing backend imports). The feature adds no hardware or platform-specific code; model loading is delegated to the existing |
for more information, see https://pre-commit.ci
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
…reload-only, mmproj/stash) Four eviction/correctness fixes on the opt-in /v1 auto-switch path: - /v1/messages/count_tokens now carries the same require_vision guard as /messages, so an image count naming a text-only GGUF can't evict a loaded vision model for a swap that can't serve the request. - /audio/generate is now reload-only. A local GGUF's audio-input capability is not a cheap pre-load probe (the companion mmproj signal can't tell an audio projector from a vision one, and codec TTS ships no projector), so resolving the client model could load a text/vision-only target and evict the working audio model before the audio check fails. Only the idle-stash restore runs here; switching TTS models is an explicit /load. - The resolver no longer treats a standalone mmproj .gguf as a servable model. _scan_models_dir's standalone-file pass does not filter mmproj the way its directory scan does, so /v1/models could advertise a projector and a switch could load it over the real weights. - A non-GGUF (Transformers/Unsloth) load and a deliberate /unload now clear the idle reload stash, so a manual load/unload is never superseded by a stale idle-freed GGUF that the next /v1 request resurrects.
|
Went through the latest review. Pushed four eviction/correctness fixes and kept four items deferred with reasons. Fixed
Deferred (intentional)
Backend suite is green against the baseline (identical failure set, only the flaky orphaned-server count test differs run to run). |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bccfbcd1b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await _maybe_auto_switch_model( | ||
| _switch_model_for_payload(payload), | ||
| request, | ||
| current_subject, | ||
| require_vision = _messages_have_image(messages), |
There was a problem hiding this comment.
When stream: true and this hook auto-switches to a cached HF GGUF, the load uses the concrete snapshot path and stores the public repo id in _openai_advertised_id. The streaming helper still builds every response.*.response.model from public_model_id(llama_backend.model_identifier), so streamed Responses can report a snapshot hash/local folder name while /v1/models, chat completions, and non-streaming Responses report the advertised repo id; use _llama_public_model_id() in the streaming path after switching.
Useful? React with 👍 / 👎.
| if _already_serving(): | ||
| return |
There was a problem hiding this comment.
When the active model was loaded by its local path but the request uses an advertised alias that resolves to that same path (for example an LM Studio publisher/model id), this early return skips the assignment of _openai_advertised_id below. The request is served correctly, but /v1/models and responses continue to report the path-derived basename and can still list the alias as loaded: false; set the advertised id before returning when the resolved alias is already serving.
Useful? React with 👍 / 👎.
Two model-id reporting fixes so an auto-switched cached HF GGUF is named by its repo id everywhere, not its snapshot path: - Streamed /v1/responses envelopes now derive the model id from _llama_public_model_id (which prefers _openai_advertised_id) instead of the raw model_identifier. After an auto-switch the identifier is the snapshot path while the repo id lives in _openai_advertised_id, so the stream used to report a snapshot basename while /v1/models, chat completions, and non-streaming Responses all reported the repo id. - When an advertised alias already resolves to the loaded model (a model loaded by local path, requested by its repo or LM Studio id), the already-serving early return now records the alias as the advertised id, so /v1/models and responses report the alias and mark it loaded instead of the path-derived basename. Resolver branch only; safe lock-free because an in-flight request blocks any concurrent swap via the single-slot busy guard.
|
Went through the latest review. Both findings were real model-id reporting inconsistencies after an auto-switch; fixed both.
Both have regression tests, and the backend suite is green against the baseline. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fee0f73a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return prompt != "" | ||
| if isinstance(prompt, (list, tuple)): | ||
| return len(prompt) > 0 | ||
| return prompt is not None |
There was a problem hiding this comment.
When automatic loading is enabled, this treats any non-null prompt as valid, including objects. A malformed /v1/completions request such as {"model":"org/B-GGUF","prompt":{}} can therefore load the named GGUF before the downstream llama-server completions call rejects the invalid prompt shape, so a deterministic client error still evicts the previously loaded model; restrict the pre-switch pass to accepted prompt shapes before calling the switch hook.
Useful? React with 👍 / 👎.
| return inp != "" | ||
| if isinstance(inp, (list, tuple)): | ||
| return len(inp) > 0 | ||
| return inp is not None |
There was a problem hiding this comment.
When automatic loading is enabled, this returns true for any non-null input, including objects. A malformed /v1/embeddings request like {"model":"org/B-GGUF","input":{}} will pass this guard, run _auto_switch_from_request_body, and load the requested GGUF before llama-server rejects the invalid embeddings input shape, turning a client 400 into a persistent model eviction; validate the input shape before switching.
Useful? React with 👍 / 👎.
| await _maybe_auto_switch_model( | ||
| _switch_model_for_payload(payload), | ||
| request, | ||
| current_subject, | ||
| require_vision = _needs_vision, |
There was a problem hiding this comment.
When a chat request includes audio_base64 and names another downloaded GGUF, this hook can load that model before the later audio size/decode checks run. An oversized or undecodable audio payload will then be rejected only after the resident model has been evicted, so validate the audio field before this switch just like the other request-shape checks.
Useful? React with 👍 / 👎.
| payload.enable_tools is True | ||
| or bool(payload.enabled_tools) | ||
| or bool(payload.tools) | ||
| or bool(payload.openai_code_exec_container_id) | ||
| or bool(payload.anthropic_code_exec_container_id) |
There was a problem hiding this comment.
For local chat requests with confirm_tool_calls=true, stream=false, and mcp_enabled=true (or tools forced on by the process policy), this pre-switch guard does not fire because it only checks explicit tool fields. The request can therefore auto-switch to the named GGUF and only later hit the local-tool confirm rejection, so include every local tool-enablement path in this guard before loading.
Useful? React with 👍 / 👎.
|
@codex review |
…o/mcp confirm) Four more validate-before-switch guards so a deterministic client error never evicts the resident model on the opt-in /v1 auto-switch path: - /v1/completions rejects an object/number prompt (only a string or array is valid) before the switch, instead of loading the named GGUF and letting llama-server reject the shape afterward. - /v1/embeddings rejects an object/number input the same way. - Chat rejects an oversized audio_base64 upload (413) before the switch. The size cap is a cheap, target-independent length check; the decode itself stays post-switch to avoid decoding a valid upload twice. - The chat confirm-without-stream pre-switch guard now mirrors the tool loop's actual enablement: _effective_enable_tools (honoring a CLI --enable-tools policy) and mcp_enabled (which opens the tool loop on its own but defers to a CLI --disable-tools policy). Previously a confirm+no-stream request with only mcp_enabled slipped past and 400'd after the swap.
|
Went through the latest review. All four were the same validate-before-switch class (a deterministic client error evicting the resident model before it is rejected); fixed all four.
Each has a regression test, and the backend suite is green against the baseline. |
…itch-model # Conflicts: # studio/backend/main.py
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a271e147a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "id": public_model_id(llama_backend.model_identifier), | ||
| # Advertised repo id after an auto-switch load, else a clean public id, | ||
| # never the absolute .gguf path (which leaks the host filesystem layout). | ||
| "id": _llama_public_model_id(llama_backend), |
There was a problem hiding this comment.
When an auto-switch load records _openai_advertised_id, /v1/models now exposes that repo id here, but the legacy raw-id fallback in openai_retrieve_model still searches the catalog for public_model_id(raw). For a client that cached an older absolute GGUF path, GET /v1/models/{raw_path} will match model_id_matches(...) and then fail to find the advertised repo-id object, returning 404 even though the model is loaded; the fallback needs to resolve to the same advertised id used for the loaded entry.
Useful? React with 👍 / 👎.
| bool(_pre_parsed[2]) or _request_has_image(payload) or bool(payload.audio_base64) | ||
| ) | ||
|
|
||
| await _maybe_auto_switch_model( |
There was a problem hiding this comment.
With auto-switch enabled, this hook runs before several target-independent local chat rejections that still happen later, such as stream=true with n > 1 in the GGUF streaming branch or audio input combined with client-tool passthrough. A request naming a downloaded model B in one of those unsupported shapes will load B here and then return 400, evicting the previously loaded model A for a request that could have been rejected up front; move those cheap validations into the pre-switch block with the existing malformed-tool/confirm/audio-size guards.
Useful? React with 👍 / 👎.
| if now - ts < _CACHE_TTL_S: | ||
| return cached | ||
| fresh = _build_index() | ||
| _scan = (now, fresh) |
There was a problem hiding this comment.
The resolver cache timestamp is captured before _build_index() walks the models directory, HF caches, LM Studio folders, and custom scan roots. On installations with enough local models for that scan to exceed the 5-second TTL, the freshly built cache is already expired when it is stored, so every subsequent auto-switch request rebuilds the filesystem index instead of reusing it; stamp _scan with a fresh time.monotonic() after _build_index() returns, as the catalog cache does.
Useful? React with 👍 / 👎.
| started = False | ||
| try: | ||
| async with _unload_gate(): | ||
| _note_start() |
There was a problem hiding this comment.
The middleware increments the keep-warm in-flight count before FastAPI authentication runs, so a POST to /v1/chat/completions with no/invalid credentials still reaches _note_end() through the 401 response and refreshes _last_active even though it never touched llama.cpp. When idle unload is enabled on an exposed server, repeated unauthenticated probes can therefore prevent VRAM from being freed (and briefly trip the same in-flight guards); only successful local-inference requests, or at least non-401/403 responses, should stamp model activity.
Useful? React with 👍 / 👎.
|
/gemini review |
…ep-warm auth
Four fixes from review:
- GET /v1/models/{id} legacy raw-path fallback now maps the raw identifier to
the same public id its /v1/models entry uses. After an auto-switch load the
identifier is the snapshot path while the entry is keyed by the advertised
repo id, so a client that cached the old absolute path no longer 404s on a
model that is in fact loaded.
- stream=true with n>1 is now rejected before the switch. Only the
non-streaming GGUF path returns multiple choices, so streaming n>1 is invalid
on every local serving path; both fields are known pre-switch, so it must not
load model B only to 400 and evict model A. Non-streaming n>1 stays
post-switch where the serving path decides.
- The resolver index cache is stamped after _build_index, not with the pre-scan
timestamp. On installs with enough local models for the multi-root scan to
exceed the 5s TTL, the cache was stored already expired and every request
rebuilt it.
- The keep-warm middleware no longer stamps model activity for 401/403
responses. It runs before FastAPI auth, so unauthenticated probes used to
refresh the idle timer without touching llama.cpp; they now decrement the
in-flight count without keeping the model warm.
|
Went through the latest review. All four were real; fixed three fully and one partially (the part that can be validated up front).
Each has a regression test. The backend suite is green against the merged-main baseline (the only run-to-run differences are the pre-existing flaky live-server and GPU global-state tests, which flip independently of these changes). |
There was a problem hiding this comment.
Code Review
This pull request introduces an opt-in model auto-switching and idle auto-unload (TTL keep-warm) feature for the local llama.cpp model, allowing transparent loading of downloaded GGUFs based on incoming OpenAI-compatible requests and freeing VRAM when idle. It adds a background loop, ASGI middleware to track in-flight requests, a local model resolver, and corresponding UI settings. Feedback on the changes includes ensuring string types are passed to path resolution functions, defensively verifying string types in the model resolver index to prevent AttributeError, explicitly checking for None in database helper functions to avoid deleting falsy values, and importing timezone in studio_db.py to resolve a potential NameError.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try: | ||
| if (p / "snapshots").is_dir(): | ||
| from routes.models import _resolve_hf_cache_realpath | ||
| real = _resolve_hf_cache_realpath(p) |
There was a problem hiding this comment.
Pass str(p) instead of the Path object p to _resolve_hf_cache_realpath to maintain compatibility, as downstream path resolution functions often expect a string representation of the path.
| real = _resolve_hf_cache_realpath(p) | |
| real = _resolve_hf_cache_realpath(str(p)) |
| if rp in seen_hf: | ||
| return [] | ||
| seen_hf.add(rp) | ||
| return _scan_hf_cache(directory) |
There was a problem hiding this comment.
Pass the resolved absolute path string rp to _scan_hf_cache instead of the original directory argument (which could be a Path object or a relative path). This guarantees type safety and consistency, as some downstream functions in _scan_hf_cache expect a string.
| return _scan_hf_cache(directory) | |
| return _scan_hf_cache(rp) |
| for key in (raw_id, getattr(info, "model_id", None), getattr(info, "display_name", None)): | ||
| if key: | ||
| index.setdefault(key.strip().lower(), entry) |
There was a problem hiding this comment.
Defensively verify that key is a string before calling .strip().lower(). If any scanner returns a non-string value for id, model_id, or display_name, this will raise an AttributeError and crash the index build.
| for key in (raw_id, getattr(info, "model_id", None), getattr(info, "display_name", None)): | |
| if key: | |
| index.setdefault(key.strip().lower(), entry) | |
| for key in (raw_id, getattr(info, "model_id", None), getattr(info, "display_name", None)): | |
| if isinstance(key, str) and key.strip(): | |
| index.setdefault(key.strip().lower(), entry) |
| if entry_value: | ||
| current[entry_key] = entry_value | ||
| else: | ||
| current.pop(entry_key, None) |
There was a problem hiding this comment.
Use if entry_value is not None: instead of if entry_value: to avoid unintended deletion of entries when a falsy but valid value (such as an empty dictionary {}) is passed.
Checking for None explicitly is a Python best practice for optional parameters to prevent bugs with falsy values.
| if entry_value: | |
| current[entry_key] = entry_value | |
| else: | |
| current.pop(entry_key, None) | |
| if entry_value is not None: | |
| current[entry_key] = entry_value | |
| else: | |
| current.pop(entry_key, None) |
| current[entry_key] = entry_value | ||
| else: | ||
| current.pop(entry_key, None) | ||
| now = datetime.now(timezone.utc).isoformat() |
There was a problem hiding this comment.
Import timezone from datetime (i.e., from datetime import timezone) to avoid a NameError at runtime, as timezone is used here directly. If only the datetime class is imported, timezone will not be defined.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an opt-in model auto-switching and idle auto-unloading (TTL keep-warm) feature for local GGUF models in Unsloth Studio. It includes a background loop to unload idle models, a pure ASGI middleware to track in-flight requests, and a local model resolver to map OpenAI request model strings to on-disk GGUFs. Additionally, settings endpoints and frontend UI components are added to configure these options. The code review feedback highlights three high-severity issues in routes/inference.py where validation checks for tool_choice and tools rely on isinstance(..., dict) checks. If these payloads are parsed as Pydantic models rather than raw dictionaries, the validation will be silently bypassed. It is recommended to defensively convert these objects to dictionaries before performing the checks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| _tc = payload.tool_choice | ||
| if isinstance(_tc, dict) and _tc.get("type") == "function": | ||
| _tc_fn = _tc.get("function") | ||
| _tc_name = _tc_fn.get("name") if isinstance(_tc_fn, dict) else None |
There was a problem hiding this comment.
If payload.tool_choice is parsed as a Pydantic model (which is standard for ChatCompletionToolChoiceOption in FastAPI), isinstance(_tc, dict) will evaluate to False, silently bypassing this validation check. We should defensively convert _tc and _tc_fn to dictionaries if they are Pydantic models (using model_dump or dict) before performing dictionary-specific checks.
_tc = payload.tool_choice
_tc_dict = _tc if isinstance(_tc, dict) else (getattr(_tc, "model_dump", getattr(_tc, "dict", lambda: {})))() if _tc else {}
if isinstance(_tc_dict, dict) and _tc_dict.get("type") == "function":
_tc_fn = _tc_dict.get("function")
_tc_fn_dict = _tc_fn if isinstance(_tc_fn, dict) else (getattr(_tc_fn, "model_dump", getattr(_tc_fn, "dict", lambda: {})))() if _tc_fn else {}
_tc_name = _tc_fn_dict.get("name") if isinstance(_tc_fn_dict, dict) else None| for _tool in payload.tools or []: | ||
| if not isinstance(_tool, dict) or _tool.get("type") != "function": | ||
| continue | ||
| _name = _tool.get("name") |
There was a problem hiding this comment.
If payload.tools contains Pydantic models instead of raw dictionaries, isinstance(_tool, dict) will evaluate to False, silently bypassing the validation check. We should defensively convert _tool to a dictionary if it is a Pydantic model before performing dictionary-specific checks.
for _tool in payload.tools or []:
_tool_dict = _tool if isinstance(_tool, dict) else (getattr(_tool, "model_dump", getattr(_tool, "dict", lambda: {})))() if _tool else {}
if not isinstance(_tool_dict, dict) or _tool_dict.get("type") != "function":
continue
_name = _tool_dict.get("name")| _tc = payload.tool_choice | ||
| if isinstance(_tc, dict) and _tc.get("type") == "function": | ||
| _tc_name = _tc.get("name") |
There was a problem hiding this comment.
If payload.tool_choice is parsed as a Pydantic model, isinstance(_tc, dict) will evaluate to False, silently bypassing the validation check. We should defensively convert _tc to a dictionary if it is a Pydantic model before performing dictionary-specific checks.
_tc = payload.tool_choice
_tc_dict = _tc if isinstance(_tc, dict) else (getattr(_tc, "model_dump", getattr(_tc, "dict", lambda: {})))() if _tc else {}
if isinstance(_tc_dict, dict) and _tc_dict.get("type") == "function":
_tc_name = _tc_dict.get("name")
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
3 participants
Summary
The OpenAI-compatible endpoints (
/v1/chat/completions,/v1/completions,/v1/messages) serve whichever GGUF is currently loaded and ignore the requestmodelfield, so an OpenAI SDK client that changesmodelnever reloads. This adds an opt-in setting that switches to a different downloaded local GGUF named by the request, similar to how llama-swap routes by the request model.Behavior
modelnames a downloaded local GGUF different from the loaded one, Studio loads it before serving, reusing the/loadpath so its dedup, tensor fallback, and threading apply.auto_unload_idle_secondssetting unloads the model after it has been idle for that many seconds to free VRAM. A pure-ASGI middleware tracks in-flight inference requests so a long stream is never unloaded mid-response. Default 0 (disabled).UNSLOTH_MODEL_IDLE_TTLenv var. Unlike the stored setting (which stays gated on auto-switch), the env value is a standalone default that enables idle-unload even with auto-switch off, for headless or container deploys. An explicit UI/API value still overrides it, and the settings GET reflects the env default when nothing is stored.Both settings live in the
app_settingsstore and are exposed viaGET/PUT /api/settings/openai-auto-switch.A manual
/loador/unloadstays a deliberate action and tears down the current model immediately; only the automatic paths (idle-unload and auto-switch) defer to an active request. When Studio is driven over the API (sk-unsloth key auth), starting training is refused with 409 while a request is in flight, since training frees VRAM by unloading the chat model; the UI (session auth) still starts training and coexists or frees VRAM as before.Testing
studio/backend/tests/test_openai_auto_switch.py: flag off does not load; unknown model falls through; already-loaded does not reload; a known unloaded model switches once with the rightLoadRequest; two concurrent requests for the same model load once; load failure propagates; resolver splitsrepo:variantand matches case-insensitively; settings coercion; theUNSLOTH_MODEL_IDLE_TTLenv override (standalone default, stored value overrides, invalid ignored); API-initiated training defers to active inference while the UI does not.openaiPython SDK: setting off,model=<other>served the loaded model unchanged; setting on,model=<downloaded gguf>switched the backend to a new llama-server, andmodel=gpt-4o-mini(unknown) still served the loaded model. Idle TTL unloaded the model after the configured seconds.