Studio: serialise GGUF reload and inherit unsloth-run extra args
Closes #5401. Three related GGUF reload bugs reproduced against `unsloth studio run -m unsloth/Qwen3-0.6B-GGUF --gguf-variant Q4_K_M --top-k 20 --seed 42`: 1. The `POST /api/inference/load` already-loaded short-circuit only compared `model_identifier` and `hf_variant`. A same-(model, variant) Apply that flipped `cache_type_kv` / `speculative_type` / `chat_template_override` / `max_seq_length` / `llama_extra_args` returned `status="already_loaded"` and the new setting silently never reached llama-server. 2. The frontend chat-settings Apply path POSTs `/unload` then `/load` without round-tripping `llama_extra_args`. Every reload after `unsloth run --some-flag X` quietly dropped `--some-flag X` from the spawned `llama-server` command line. 3. `LlamaCppBackend.load_model` released `_lock` between Phase 1 (kill) and Phase 3 (spawn) so two concurrent loads each passed Phase 1 with `self._process is None`. Both ran Phase 2 (download), both reached Phase 3, and the Phase 3 defensive `_kill_process()` from #5171 collapsed them to one survivor only after both `subprocess.Popen` calls had landed. For the 86 GB MoE in #5161 / the model in #5401 the overlap window was tens of seconds, long enough to OOM the host. With a 0.6B model the pgrep timeline showed two simultaneous PIDs for 3.3 s on `main`. Fix: `studio/backend/core/inference/llama_cpp.py` * Add `self._serial_load_lock = threading.Lock()`. The whole body of `load_model` runs under this lock so two concurrent `/api/inference/load` requests are strictly sequential. The fine-grained `_lock` and the Phase 3 defensive `_kill_process()` from #5171 are kept as a second layer. `/unload`, `/status`, and `/load-progress` are unaffected because they only touch the fine-grained lock or read properties. * Add `self._extra_args` plus an `extra_args` property, written inside `load_model` whenever the caller supplies a non-`None` value. `unload_model()` deliberately does not reset it so the route layer can inherit the args across the frontend's `/unload` + `/load` gap. `studio/backend/routes/inference.py` * Add `_request_matches_loaded_settings(request, llama_backend)` that compares `max_seq_length`, `cache_type_kv`, `speculative_type`, `chat_template_override`, and `llama_extra_args` between the incoming request and the live backend. Same-(model, variant) requests whose runtime settings differ now fall through to a real reload instead of returning `already_loaded`. A missing `llama_extra_args` field on the request is treated as "inherit current", so the short-circuit still fires when the only difference is the frontend not echoing the CLI flags back. * GGUF load branch inherits `llama_extra_args` from `llama_backend.extra_args` when the request omits the field, re-validates through `validate_extra_args`, and forwards the result to `load_model(...)`. An explicit `[]` from the caller is still honoured as "clear". Verified end to end against a live `unsloth studio run` instance: | Scenario | Before | After | | --------------------------------------------------------------- | --------- | ------------------------------------------------------------------------ | | `/load` same (model, variant, settings) | 1 PID, `already_loaded` | unchanged | | `/load` same model, variant, new `cache_type_kv=q8_0` ctx=8192 | `already_loaded`, settings dropped | `loaded`, `/status` reports the new settings, new server has `-c 8192 --cache-type-k q8_0 --top-k 20 --seed 42` | | Frontend Apply `/unload` + `/load`, new settings, no `llama_extra_args` field | Drops `--top-k 20 --seed 42` | Preserves `--top-k 20 --seed 42` | | `/unload` + two parallel `/load` | Two PIDs for 3.3 s | Max simultaneous count = 1 across the full pgrep timeline | | `/load` with `llama_extra_args=[]` (explicit clear) | n/a | `loaded`, new server has no `--top-k` / `--seed` | | `/load` with `llama_extra_args=["--top-k","30","--seed","7"]` (override) | n/a | `loaded`, new server has the supplied flags | `pytest studio/backend/tests` is green except for one pre-existing terminal-width-sensitive assertion (`test_studio_api.py::test_help_output`) and the pre-existing `test_studio_api.py` fixture errors that fail on unmodified main too. No new regressions.
This commit is contained in:
Daniel Han
parent
739ebeea82
commit
f9cbec3b60
2 changed files with 741 additions and 583 deletions
File diff suppressed because it is too large
Load diff
|
|
@ -401,6 +401,59 @@ def _validate_native_mmproj_companion(
|
|||
) from exc
|
||||
|
||||
|
||||
def _normalise_settings_str(value: Optional[str]) -> Optional[str]:
|
||||
"""Lowercase + strip a settings string, mapping blank/None to None."""
|
||||
if value is None:
|
||||
return None
|
||||
if isinstance(value, str):
|
||||
stripped = value.strip().lower()
|
||||
return stripped or None
|
||||
return value
|
||||
|
||||
|
||||
def _request_matches_loaded_settings(
|
||||
request: LoadRequest, llama_backend: LlamaCppBackend
|
||||
) -> bool:
|
||||
"""Return True iff the request's runtime settings match the loaded server.
|
||||
|
||||
The /api/inference/load short-circuit at the call site additionally checks
|
||||
model identifier + GGUF variant + ``is_loaded`` before delegating here.
|
||||
We compare every setting that ends up on the ``llama-server`` command
|
||||
line so a same-(model, variant) Apply request that *did* change a
|
||||
runtime setting falls through to a real reload instead of silently
|
||||
returning ``status="already_loaded"`` (issue #5401).
|
||||
"""
|
||||
# max_seq_length == 0 means "use model default" -- treat as a match
|
||||
# against the running effective context length.
|
||||
backend_ctx = llama_backend.context_length or 0
|
||||
if request.max_seq_length and request.max_seq_length != backend_ctx:
|
||||
return False
|
||||
if _normalise_settings_str(request.cache_type_kv) != _normalise_settings_str(
|
||||
llama_backend.cache_type_kv
|
||||
):
|
||||
return False
|
||||
# Speculative dropdown sends "default" / "off" / explicit type names.
|
||||
# The backend stores the type the user selected (or None when not
|
||||
# speculatively decoding). Treat None and "off" as equivalent.
|
||||
req_spec = _normalise_settings_str(request.speculative_type)
|
||||
backend_spec = _normalise_settings_str(llama_backend.speculative_type)
|
||||
if (req_spec or "off") != (backend_spec or "off"):
|
||||
return False
|
||||
if (request.chat_template_override or None) != (
|
||||
llama_backend.chat_template_override or None
|
||||
):
|
||||
return False
|
||||
req_extra = list(request.llama_extra_args) if request.llama_extra_args else []
|
||||
backend_extra = list(llama_backend.extra_args) if llama_backend.extra_args else []
|
||||
# ``llama_extra_args is None`` on the request means "inherit current",
|
||||
# i.e. the frontend Apply path that does not round-trip the field. We
|
||||
# only fall through to a real reload when the caller *explicitly*
|
||||
# supplied a non-empty list and it differs from what is loaded.
|
||||
if request.llama_extra_args is not None and req_extra != backend_extra:
|
||||
return False
|
||||
return True
|
||||
|
||||
|
||||
def _resolve_model_identifier_for_request(
|
||||
request: LoadRequest | ValidateModelRequest,
|
||||
*,
|
||||
|
|
@ -474,6 +527,13 @@ async def load_model(
|
|||
and llama_backend.hf_variant.lower() == request.gguf_variant.lower()
|
||||
and llama_backend.model_identifier
|
||||
and llama_backend.model_identifier.lower() == model_identifier.lower()
|
||||
# Identifier + variant alone are insufficient: a same-model
|
||||
# Apply that changed KV / ctx / spec / template / extra
|
||||
# args used to silently short-circuit and the user's
|
||||
# change was dropped. Require every runtime setting that
|
||||
# ends up on the llama-server command line to also match
|
||||
# before reporting "already_loaded". See issue #5401.
|
||||
and _request_matches_loaded_settings(request, llama_backend)
|
||||
):
|
||||
logger.info(
|
||||
f"Model already loaded (GGUF): {model_log_label} variant={request.gguf_variant}, skipping reload"
|
||||
|
|
@ -608,6 +668,33 @@ async def load_model(
|
|||
)
|
||||
unsloth_backend.unload_model(unsloth_backend.active_model_name)
|
||||
|
||||
# Inherit ``llama_extra_args`` from the previous load when the
|
||||
# incoming request omits the field. The chat-settings Apply
|
||||
# path on the frontend does not round-trip these flags (see
|
||||
# use-chat-model-runtime.ts), so without this, every reload
|
||||
# after ``unsloth run --some-flag X`` quietly drops the
|
||||
# user's ``--some-flag X`` from the spawned llama-server.
|
||||
# An explicit empty list is still honoured as "clear them".
|
||||
if request.llama_extra_args is None and llama_backend.extra_args:
|
||||
inherited = list(llama_backend.extra_args)
|
||||
try:
|
||||
extra_llama_args = validate_extra_args(inherited)
|
||||
except ValueError:
|
||||
# Stored args came from a prior validated load; revalidation
|
||||
# should not normally fail, but if it does (e.g. managed-flag
|
||||
# rules changed) fall back to "no extras" rather than 400.
|
||||
logger.warning(
|
||||
"Stored llama_extra_args failed revalidation; "
|
||||
"loading without them: %s",
|
||||
inherited,
|
||||
)
|
||||
extra_llama_args = []
|
||||
else:
|
||||
logger.info(
|
||||
"Inheriting llama_extra_args from previous load: %s",
|
||||
extra_llama_args,
|
||||
)
|
||||
|
||||
# Route to HF mode or local mode based on config
|
||||
# Run in a thread so the event loop stays free for progress
|
||||
# polling and other requests during the (potentially long)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue