unsloth/studio/backend/tests/test_llama_server_args.py
Daniel Han ab56e4a9ed
Studio: serialise GGUF reload and inherit unsloth-run extra args (#5427)
* 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.

* Studio: track requested n_ctx so Auto-slider flips trigger a reload

Review feedback on PR #5427 from gemini-code-assist.

The original short-circuit compared ``request.max_seq_length`` against
``llama_backend.context_length`` (the effective context). VRAM-fit
logic can cap the running server below what the caller asked for, so
this comparison incorrectly returns ``already_loaded`` when the user
flips the slider from an explicit length (e.g. 8192) back to "Auto"
(0): the explicit request was capped to, say, 4096, and the new "Auto"
request reads ``backend.context_length == 4096`` and decides nothing
changed.

Track the originally requested ``n_ctx`` on the backend instead and
compare against that. ``requested_n_ctx == 0`` means the last load
asked for the model's native length; ``request.max_seq_length == 0``
matches it.

Verified in the sandbox suite (now 90 tests):

- ``test_explicit_to_auto_triggers_reload`` -- loaded with explicit
  8192, then Apply with ``max_seq_length=0`` falls through to a real
  reload and the new server runs at the native 40960.
- ``test_auto_to_explicit_triggers_reload`` -- inverse direction.
- ``test_explicit_to_same_explicit_short_circuits`` -- re-Apply with
  the same explicit value still short-circuits (no needless reload).
- Existing scenarios (kv change, spec change, template change, extra
  args inherit, parallel-load stress, frontend Apply flow) unchanged.

``pytest studio/backend/tests`` still green on the same set of tests;
the pre-existing ``test_help_output`` failure and ``test_studio_api``
fixture errors are unaffected.

* Studio: tighten comments in the 5401 fix

Trim the verbose explanatory comments and docstrings introduced in
f9cbec3b and dd0b1d58 down to one-line summaries. The "why" still
points at issue #5401; the multi-paragraph rationale belonged in the
PR body, not the source. No behaviour change.

* ci: retrigger after zoo drift + IPython fixes landed in main

* ci: retrigger Mac Studio UI CI after transient fetch flake

* Studio: address six P2 followups on the 5401 reload PR

Tightens the inheritance and serial-load paths to close the six P2
findings raised by codex-connector on PR #5427 against `f9cbec3b` /
`dd0b1d58`.

1. Re-check loaded state before killing queued loads. Two duplicate
   `/api/inference/load` requests both pass the route-level
   `is_loaded` gate before the first publishes `_healthy = True`. The
   second waits on `_serial_load_lock`, enters Phase 1, and tears down
   the just-spawned llama-server for a redundant full reload. Added
   `LlamaCppBackend._already_in_target_state(...)` and a short-circuit
   at the top of the serial-lock block: if the live server already
   satisfies the kwargs, return True without killing.

2. Don't inherit CLI overrides that shadow new first-class settings.
   `unsloth run -c 4096` is a permitted pass-through; the validator
   docs explicitly call out `-c`/`--ctx-size`. Stored in `_extra_args`
   and appended after Studio's own flags, the inherited `-c 4096`
   silently won the last-wins parse against a new
   `max_seq_length=8192`. Added `strip_shadowing_flags` in
   `llama_server_args.py` (covers `-c`, `--cache-type-k/v`, `--spec-*`,
   `--chat-template*`, `--jinja`/`--no-jinja`) and the route runs the
   inherited list through it before validate + forward.

3. Restrict inherited llama args to the same GGUF model. `_extra_args`
   is deliberately preserved across `unload_model()` for the chat-
   settings Apply flow (`/unload` + `/load` with no `llama_extra_args`
   field). Now also track `_extra_args_source = (model_identifier,
   hf_variant)` so the route can refuse cross-model inheritance.
   `LlamaCppBackend.extra_args_source` exposes the tuple.

4. Persist extras only after a successful load. `_extra_args` was
   written at the top of `load_model` before Popen + health check, so
   a failed startup left bad args in place to poison the next UI
   retry. The write (along with `_requested_n_ctx`) is now deferred
   until after `_healthy = True`.

5. Ignore speculative diffs for vision loads. `load_model` silently
   gates speculative decoding on `not is_vision`, so the backend's
   `_speculative_type` stays `None` for vision models. The route's
   comparator now normalises the request's value to `"off"` when
   `llama_backend.is_vision` to avoid a no-op reload of a vision
   server every time the dropdown defaults to `default`. The
   `_already_in_target_state` helper applies the same rule.

6. Wait for the replacement server before short-circuiting. `_kill_process`
   did not clear `_healthy`; the new first-class settings
   (`_cache_type_kv`, `_speculative_type`, `_chat_template_override`)
   are written under `_lock` BEFORE Popen + `_wait_for_health`. A
   duplicate `/load` arriving during the new server's warm-up window
   could short-circuit against the not-yet-healthy replacement and the
   caller would start inference against a server that was still
   loading. `_kill_process` now sets `_healthy = False` in its
   `finally` block so `is_loaded` returns False from the moment the
   old server is killed until the new one finishes warm-up.

Tests:

- Sandbox suite under `./temp/sim_5401/` extended to 136 tests (was
  90): new unit coverage for `strip_shadowing_flags` (12 cases),
  `_kill_process` clears `_healthy`, `extra_args_source` lifecycle and
  cross-model behaviour, failed-load preserving prior extras, and the
  duplicate-load short-circuit at `load_model` level. New live
  integration cases verify shadow-strip via `pgrep` on the live
  llama-server cmdline, cross-model refusal, and PID stability across
  a duplicate-load race. All 136 pass.
- `pytest studio/backend/tests --deselect test_studio_api.py`:
  1079 passed, 46 skipped, identical to the pre-change count. The
  pre-existing `test_studio_api.py` fixture errors and the
  terminal-width-sensitive `test_help_output` are unaffected.
- Ruff: clean on the three modified files.

* Studio: tighten GGUF reload inheritance and duplicate-load guard

Re-narrow llama_extra_args to None after validate_extra_args when the
incoming request omitted the field, so the backend can distinguish
"caller omitted, inherit prior load" from "caller explicitly cleared
to []". Without this a queued duplicate /load reaches the backend as
[] and fails _already_in_target_state's exact-equality check, killing
the just-started llama-server. The pass-through validate call from
the original "forward llama-server args from unsloth studio run /
unsloth run" change is preserved as-is; only the post-pass narrowing
is new. Cross-source loads now explicitly clear extras so a model
switch can't accidentally inherit via the backend's "no opinion"
semantics.

Store the caller's hf_variant kwarg (None for local GGUF files) in
_extra_args_source instead of the derived self._hf_variant
(an extracted filename quant label like "Q4_K_M"). Same-source check
in the route is now symmetric for HF and direct-file loads.

Add gguf_path to _already_in_target_state and prefer on-disk path
identity when both backend and caller have a path. This stops the
duplicate-load guard from killing a healthy server on repeat local
loads (where hf_variant is None on the caller side but extracted on
the backend side).

Split shadow-flag stripping into per-group toggles (context / cache /
spec / template). The route now opts into stripping only the groups
whose first-class field was actually set on the incoming request, so
an inherited --chat-template-file survives an Apply that omits
chat_template_override. _request_matches_loaded_settings detects
shadowing extras on the inherit path and falls through to a real
reload so the strip can run.

Mark --spec-default, --jinja, --no-jinja as boolean inside the
shadow stripper so the value-consuming heuristic no longer eats the
following positional token.

* Studio: trim comments around GGUF reload inheritance

* Studio: cover GGUF reload inheritance and shadow-flag stripping

* Studio: drop redundant issue refs from inheritance comments

* Studio: drop redundant issue refs from inheritance comments

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Studio: key inheritance source off resolved gguf_variant

codex-connector P2 on PR #5427 cd14cae1: the inheritance gate at
``routes/inference.py:696`` compared the stored ``source[1]`` against
``request.gguf_variant``, but the HF branch loaded with
``hf_variant = config.gguf_variant`` (the *resolved* variant after
ModelConfig auto-pick). When the caller omitted ``gguf_variant`` on a
follow-up Apply, ``source[1] == "Q4_K_M"`` but
``(request.gguf_variant or "") == ""``, ``same_source`` returned False,
and the chat-settings Apply silently dropped CLI pass-through flags
for every auto-pick / local-file load.

Fix both sides of the comparison to key off ``config.gguf_variant``:

* The route compares ``source[1]`` to ``config.gguf_variant`` (the
  resolved label) rather than the request field.
* The local-mode load_model call now passes
  ``hf_variant = config.gguf_variant`` so ``_extra_args_source``
  stores the same string the route reads back. The HF branch already
  did this.

Sandbox: added test_source_records_caller_variant_not_extracted_label
to lock the storage key contract.
``pytest studio/backend/tests --deselect test_studio_api.py``:
1100 passed, identical to pre-change.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Studio: deny upstream --ui family on llama-server pass-through

The validator's web-UI block named only ``--webui`` / ``--no-webui``,
which is llama.cpp's pre-rename spelling. Current upstream
(``tools/server/README.md``) uses ``--ui`` / ``--no-ui`` plus
``--ui-config``, ``--ui-config-file``, and ``--ui-mcp-proxy`` /
``--no-ui-mcp-proxy``. Without these in the denylist a user could
``unsloth run --ui`` and enable llama-server's built-in web UI on
the port Studio's reverse proxy targets, breaking the UI surface.

Keep the legacy ``--webui`` group so the validator still rejects
old binaries that haven't been re-spelled.

Cross-referenced against the README's full flag list; this was the
only gap for the post-#5401 inheritance / shadow-strip work. Pass-
through flags from every other README category (sampling, jinja,
ctx, cache, threads, GPU, reasoning, grammar, chat-template-kwargs)
already validate cleanly; sandbox suite exercises ~60 of them in
the new ``test_08_llama_server_pass_through.py``.

``pytest studio/backend/tests --deselect test_studio_api.py``:
1100 passed, identical to pre-change.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
2026-05-17 04:16:09 -07:00

307 lines
9.7 KiB
Python

# SPDX-License-Identifier: AGPL-3.0-only
# Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. See /studio/LICENSE.AGPL-3.0
"""Unit tests for the llama-server pass-through args validator.
The validator is the security boundary between user-supplied CLI / HTTP
input and the llama-server subprocess command. These tests pin the
denylist behavior so the boundary doesn't quietly regress when new
managed flags are added.
"""
from __future__ import annotations
import pytest
from core.inference.llama_server_args import (
is_managed_flag,
strip_shadowing_flags,
validate_extra_args,
)
# ── Pass-through (allowed) ───────────────────────────────────────────
@pytest.mark.parametrize(
"args",
[
# Sampling
["--top-k", "20"],
["--top-p", "0.9", "--min-p", "0.05"],
["--seed", "-1"], # negative value, not a flag
["--temp", "0.0"],
["--repeat-penalty", "1.05"],
["--mirostat", "2", "--mirostat-lr", "0.1"],
["--xtc-probability", "0.05", "--xtc-threshold", "0.1"],
["--dry-multiplier", "0.5"],
# Tier-2 knobs that map to LoadRequest fields
["--cache-type-k", "q8_0"],
["--cache-type-v", "q8_0"],
["--chat-template-file", "/tmp/tpl.jinja"],
["--chat-template-kwargs", '{"reasoning_effort":"high"}'],
["--spec-type", "ngram-mod"],
["--spec-default"],
# Reasoning controls
["--reasoning-format", "deepseek"],
["-rea", "auto"],
# Soft-managed flags the user may want to override on the CLI;
# llama.cpp's last-wins parsing means these win over Studio's
# auto-set version.
["-c", "131072"],
["--ctx-size", "8192"],
["--parallel", "1"],
["-np", "8"],
["--flash-attn", "off"],
["-fa", "on"],
["--no-context-shift"],
["--context-shift"],
["--jinja"],
["--no-jinja"],
["-ngl", "-1"],
["--gpu-layers", "32"],
["-t", "16"],
["--threads", "32"],
["-fit", "off"],
["--fit", "on"],
["--fit-ctx", "8192"],
],
)
def test_pass_through_allowed(args):
assert validate_extra_args(args) == args
def test_none_returns_empty_list():
assert validate_extra_args(None) == []
def test_empty_list_returns_empty_list():
assert validate_extra_args([]) == []
def test_value_with_equals_form_passes_through():
assert validate_extra_args(["--top-k=20"]) == ["--top-k=20"]
def test_non_flag_token_passes_through():
# A bare positional value (not preceded by a flag) is preserved
# verbatim. llama-server may reject it, but that's not our job.
assert validate_extra_args(["foo"]) == ["foo"]
# ── Denylist (rejected) ──────────────────────────────────────────────
@pytest.mark.parametrize(
"denied",
[
# Model identity
"-m",
"--model",
"-hf",
"-hfr",
"--hf-repo",
"-hff",
"--hf-file",
"-hft",
"--hf-token",
"-mm",
"--mmproj",
"--mmproj-url",
# Networking (Studio binds + proxies)
"--host",
"--port",
"--path",
"--api-prefix",
"--reuse-port",
# Auth / TLS
"--api-key",
"--api-key-file",
"--ssl-key-file",
"--ssl-cert-file",
# Single-model server
"--webui",
"--no-webui",
"--models-dir",
"--models-max",
],
)
def test_denylist_rejects_all_aliases(denied):
with pytest.raises(ValueError, match = denied):
validate_extra_args([denied, "value"])
def test_denylist_rejects_equals_form():
with pytest.raises(ValueError, match = "--port"):
validate_extra_args(["--port=9000"])
def test_denylist_rejects_short_form_when_long_is_denied():
# -m is the short form of the hard-denied --model; rejecting only
# the long form would leave a trivial bypass.
with pytest.raises(ValueError, match = "-m"):
validate_extra_args(["-m", "/some/other/path.gguf"])
def test_denylist_message_names_offending_flag():
with pytest.raises(ValueError) as excinfo:
validate_extra_args(["--top-k", "20", "--api-key", "secret"])
assert "--api-key" in str(excinfo.value)
def test_first_denied_flag_short_circuits():
# Validation stops at the first denied flag; later denied flags
# in the same call don't matter for behaviour, but the message
# should name the first one we hit.
with pytest.raises(ValueError, match = "--port"):
validate_extra_args(["--port", "1", "--host", "x"])
# ── Numeric values that look flag-ish ─────────────────────────────────
@pytest.mark.parametrize("value", ["-1", "-0.5", "-42", "-.5"])
def test_negative_number_value_is_not_flag(value):
# ``--seed -1`` is a value, not a flag. Validator must not try
# to look up "-1" in the denylist.
assert validate_extra_args(["--seed", value]) == ["--seed", value]
# ── is_managed_flag helper ───────────────────────────────────────────
def test_is_managed_flag_true_for_denied():
assert is_managed_flag("--port") is True
assert is_managed_flag("--api-key") is True
assert is_managed_flag("-m") is True
assert is_managed_flag("--model") is True
def test_is_managed_flag_false_for_pass_through():
assert is_managed_flag("--top-k") is False
assert is_managed_flag("--cache-type-k") is False
assert is_managed_flag("--chat-template-file") is False
# Soft-managed flags pass through (last-wins override)
assert is_managed_flag("-c") is False
assert is_managed_flag("--ctx-size") is False
assert is_managed_flag("--parallel") is False
assert is_managed_flag("--flash-attn") is False
assert is_managed_flag("-ngl") is False
assert is_managed_flag("--threads") is False
# ── strip_shadowing_flags ─────────────────────────────────────────────
def test_strip_shadowing_flags_drops_context_when_requested():
out = strip_shadowing_flags(
["-c", "4096", "--top-k", "20"],
strip_context = True,
strip_cache = False,
strip_spec = False,
strip_template = False,
)
assert out == ["--top-k", "20"]
def test_strip_shadowing_flags_keeps_context_when_not_requested():
out = strip_shadowing_flags(
["-c", "4096", "--top-k", "20"],
strip_context = False,
strip_cache = False,
strip_spec = False,
strip_template = False,
)
assert out == ["-c", "4096", "--top-k", "20"]
def test_strip_shadowing_flags_keeps_chat_template_when_template_disabled():
# Caller did not supply chat_template_override; the inherited
# --chat-template-file must survive the strip.
out = strip_shadowing_flags(
["--chat-template-file", "/tmp/custom.jinja", "--top-k", "20"],
strip_context = True,
strip_cache = True,
strip_spec = True,
strip_template = False,
)
assert out == ["--chat-template-file", "/tmp/custom.jinja", "--top-k", "20"]
def test_strip_shadowing_flags_drops_template_when_requested():
out = strip_shadowing_flags(
["--chat-template-file", "/tmp/custom.jinja", "--top-k", "20"],
strip_template = True,
)
assert out == ["--top-k", "20"]
def test_strip_shadowing_flags_keeps_cache_when_cache_disabled():
out = strip_shadowing_flags(
["--cache-type-k", "q8_0", "--cache-type-v", "q8_0", "--top-k", "20"],
strip_cache = False,
)
assert out == [
"--cache-type-k",
"q8_0",
"--cache-type-v",
"q8_0",
"--top-k",
"20",
]
def test_strip_shadowing_flags_keeps_spec_when_spec_disabled():
out = strip_shadowing_flags(
["--spec-type", "ngram-mod", "--draft-min", "48", "--top-k", "20"],
strip_spec = False,
)
assert out == [
"--spec-type",
"ngram-mod",
"--draft-min",
"48",
"--top-k",
"20",
]
def test_strip_shadowing_flags_boolean_does_not_consume_next_token():
# --spec-default is a boolean shadowing flag; the value-skipping
# heuristic must skip just the flag, not the following positional.
out = strip_shadowing_flags(["--spec-default", "ngram-mod"], strip_spec = True)
assert out == ["ngram-mod"]
def test_strip_shadowing_flags_jinja_boolean_preserves_positional():
out = strip_shadowing_flags(["--jinja", "trailing-positional"], strip_template = True)
assert out == ["trailing-positional"]
def test_strip_shadowing_flags_no_jinja_boolean_preserves_positional():
out = strip_shadowing_flags(
["--no-jinja", "trailing-positional"], strip_template = True
)
assert out == ["trailing-positional"]
def test_strip_shadowing_flags_equals_form_drops_only_the_flag():
out = strip_shadowing_flags(["--ctx-size=4096", "--seed", "-1"], strip_context = True)
assert out == ["--seed", "-1"]
def test_strip_shadowing_flags_handles_none_input():
assert strip_shadowing_flags(None) == []
def test_strip_shadowing_flags_handles_empty_input():
assert strip_shadowing_flags([]) == []
def test_strip_shadowing_flags_defaults_strip_everything():
# The route's already-loaded comparator calls strip_shadowing_flags
# with no kwargs to detect ANY shadowing flag in stored extras.
out = strip_shadowing_flags(
["-c", "4096", "--cache-type-k", "q8_0", "--spec-default", "--jinja"]
)
assert out == []