studio/tests: cover the GGUF load ordering behaviourally and make the structlog stub order-independent (#7442)
* studio: fix Backend CI red on main from an ambiguous ordering anchor test_load_marker_precedes_hub_guard_and_unload fails on main, so every open PR against the repo inherits the failure. Root cause. #7239 (a7761e174) reworked the GGUF GPU-pool validation in _load_model_impl from "if config.is_gguf and effective_gpu_ids is not None:" to a bare "if config.is_gguf:", placed earlier in the function than the GGUF load branch. The test anchors on source.index("if config.is_gguf:"), a first-match search, so it silently re-anchored onto the GPU-pool statement. #7251 (95f42bcce) then restored the assertion "= _resolve_inherited_extra_args(" before "if config.is_gguf:" against a tree where that anchor already pointed at the wrong statement, and main went red. Checking out95f42bcceand running the suite reproduces the same single failure. The code is correct. _resolve_inherited_extra_args still runs before the GGUF load branch and before the hub-download guard that consumes extra_llama_args for require_mmproj, so the guarantee #7251 protects is intact; only the assertion is wrong. Fix. Assert that guarantee behaviourally instead of by source offsets. The new test drives _load_model_impl over a vision GGUF with a stored --no-mmproj from a previous same-model load and captures the require_mmproj the hub guard is called with: inherited --no-mmproj gives False, nothing to inherit gives True, and an explicit request list wins over the stored one both ways. Moving the resolution call after the guard makes the inherited case report True and the test fails, so it detects the reorder the old assertion was meant to catch, without depending on how many "if config.is_gguf:" statements the endpoint has. The surviving marker-before-guard-before-unload assertion had the same ambiguous anchor for its slice start, silently widening the slice past the GPU-pool block. It now slices from the "if config.is_gguf:" nearest above the in-flight marker, which pins the load branch. The structlog test stub gains a get_logger factory so routes/inference.py is importable when structlog is absent. 34 pass in tests/test_gguf_load_cache_reuse.py (was 32 pass, 1 fail); 350 pass across it plus test_llama_cpp_mmproj_fallback.py and test_llama_cpp_mtp_detection.py. A full backend run before and after is identical apart from this test going from fail to pass. * studio/tests: repair a pre-existing bare structlog stub before importing routes * studio/tests: tighten the comments on the new load-ordering coverage * Tighten comments on the load-ordering coverage for PR #7442
This commit is contained in:
parent
6ae037f97c
commit
d7cdc96051
1 changed files with 138 additions and 0 deletions
|
|
@ -9,10 +9,14 @@ No GPU, network, or subprocesses are required.
|
|||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import importlib.util
|
||||
import logging
|
||||
import sys
|
||||
import threading
|
||||
import types as _types
|
||||
from contextlib import nullcontext
|
||||
from pathlib import Path
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
|
@ -28,7 +32,12 @@ _loggers_stub.get_logger = lambda name: __import__("logging").getLogger(name)
|
|||
sys.modules.setdefault("loggers", _loggers_stub)
|
||||
|
||||
_structlog_stub = _types.ModuleType("structlog")
|
||||
# routes/inference.py binds structlog.get_logger at import time, and setdefault
|
||||
# keeps a bare stub an earlier test left behind: repair it rather than rely on order.
|
||||
_structlog_stub.get_logger = lambda *_args, **_kwargs: logging.getLogger("structlog_stub")
|
||||
sys.modules.setdefault("structlog", _structlog_stub)
|
||||
if not hasattr(sys.modules["structlog"], "get_logger"):
|
||||
sys.modules["structlog"].get_logger = _structlog_stub.get_logger
|
||||
|
||||
try:
|
||||
import httpx # noqa: F401
|
||||
|
|
@ -120,6 +129,22 @@ def _fail_get_paths_info(*_args, **_kwargs):
|
|||
raise AssertionError("cached reuse must return before the sizing preflight")
|
||||
|
||||
|
||||
def _load_route_module(name: str, relative_path: str):
|
||||
"""Import a route module under a private name so patches can't leak."""
|
||||
spec = importlib.util.spec_from_file_location(name, Path(_BACKEND_DIR) / relative_path)
|
||||
module = importlib.util.module_from_spec(spec)
|
||||
spec.loader.exec_module(module)
|
||||
return module
|
||||
|
||||
|
||||
async def _inline_to_thread(func, /, *args, **kwargs):
|
||||
return func(*args, **kwargs)
|
||||
|
||||
|
||||
async def _no_gguf_gpu_ids(*_args, **_kwargs):
|
||||
return None
|
||||
|
||||
|
||||
class TestLoadReusesCachedCopy:
|
||||
def test_download_uses_selected_cache_for_lookup_preflight_and_write(
|
||||
self, tmp_path, monkeypatch
|
||||
|
|
@ -809,3 +834,116 @@ class TestLoadHubDownloadExclusion:
|
|||
Path(__file__).resolve().parent.parent / "core" / "inference" / "llama_cpp.py"
|
||||
).read_text()
|
||||
assert "@_with_gguf_load_marker\n def load_model(" in llama_source
|
||||
|
||||
def _capture_hub_guard_require_mmproj(
|
||||
self,
|
||||
stored_extra_args,
|
||||
request_extra_args = None,
|
||||
):
|
||||
"""Drive /load's GGUF path and return the hub guard's require_mmproj.
|
||||
|
||||
The guard reports a conflicting download, so the 409 is the observation
|
||||
point and no llama-server ever starts.
|
||||
"""
|
||||
import core.inference.llama_cpp as llama_cpp_module
|
||||
|
||||
from fastapi import HTTPException
|
||||
from models.inference import LoadRequest
|
||||
|
||||
route = _load_route_module(
|
||||
"inference_route_module_for_inherited_extra_args_test",
|
||||
"routes/inference.py",
|
||||
)
|
||||
captured = {}
|
||||
|
||||
def _fake_blocks(
|
||||
repo,
|
||||
variant,
|
||||
*,
|
||||
require_mmproj,
|
||||
hf_token = None,
|
||||
):
|
||||
captured["repo"] = repo
|
||||
captured["variant"] = variant
|
||||
captured["require_mmproj"] = require_mmproj
|
||||
return True
|
||||
|
||||
# A vision GGUF: require_mmproj is True unless the extras say --no-mmproj.
|
||||
config = SimpleNamespace(
|
||||
is_gguf = True,
|
||||
is_lora = False,
|
||||
is_vision = True,
|
||||
is_audio = False,
|
||||
audio_type = None,
|
||||
has_audio_input = False,
|
||||
gguf_hf_repo = REPO,
|
||||
gguf_variant = VARIANT,
|
||||
gguf_file = None,
|
||||
gguf_mmproj_file = None,
|
||||
identifier = REPO,
|
||||
display_name = REPO,
|
||||
)
|
||||
# Pass-through extras the running backend recorded for the last load.
|
||||
llama_backend = SimpleNamespace(
|
||||
is_loaded = False,
|
||||
extra_args = list(stored_extra_args),
|
||||
extra_args_source = (REPO, VARIANT),
|
||||
hf_variant = VARIANT,
|
||||
model_identifier = REPO,
|
||||
)
|
||||
request = LoadRequest(
|
||||
model_path = REPO,
|
||||
gguf_variant = VARIANT,
|
||||
llama_extra_args = request_extra_args,
|
||||
)
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
route,
|
||||
"ModelConfig",
|
||||
SimpleNamespace(from_identifier = lambda **_kwargs: config),
|
||||
),
|
||||
patch.object(route, "get_llama_cpp_backend", lambda: llama_backend),
|
||||
patch.object(
|
||||
route,
|
||||
"get_inference_backend",
|
||||
lambda: SimpleNamespace(active_model_name = None),
|
||||
),
|
||||
patch.object(route, "_resolve_gguf_gpu_ids_for_request", _no_gguf_gpu_ids),
|
||||
patch.object(route, "_guard_chat_load_against_training", return_value = None),
|
||||
patch.object(route, "_effective_load_in_4bit", return_value = False),
|
||||
patch.object(route, "_hf_offline_if_dns_dead", nullcontext),
|
||||
patch.object(route.asyncio, "to_thread", new = _inline_to_thread),
|
||||
patch.object(llama_cpp_module, "_hub_download_blocks_gguf_load", _fake_blocks),
|
||||
):
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
asyncio.run(
|
||||
route._load_model_impl(
|
||||
request,
|
||||
SimpleNamespace(
|
||||
app = SimpleNamespace(
|
||||
state = SimpleNamespace(llama_parallel_slots = 1),
|
||||
),
|
||||
),
|
||||
current_subject = "test-user",
|
||||
)
|
||||
)
|
||||
|
||||
assert exc_info.value.status_code == 409
|
||||
assert captured["repo"] == REPO
|
||||
return captured["require_mmproj"]
|
||||
|
||||
def test_inherited_extra_args_shape_hub_guard_require_mmproj(self):
|
||||
# Inheritance must resolve before the hub-download guard: an inherited
|
||||
# --no-mmproj decides require_mmproj, so resolving later rejects a load
|
||||
# over a download the effective arguments disable (#7251).
|
||||
assert self._capture_hub_guard_require_mmproj(["--no-mmproj"]) is False
|
||||
# Control: nothing to inherit, so a vision GGUF still needs its mmproj.
|
||||
assert self._capture_hub_guard_require_mmproj([]) is True
|
||||
# An explicit request list wins over the stored one, both ways.
|
||||
assert (
|
||||
self._capture_hub_guard_require_mmproj([], request_extra_args = ["--no-mmproj"]) is False
|
||||
)
|
||||
assert (
|
||||
self._capture_hub_guard_require_mmproj(["--no-mmproj"], request_extra_args = []) is True
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue