From 4836c510e20c18c0f4cc4cce9521f428504596d3 Mon Sep 17 00:00:00 2001 From: kevin9327 <5299031+kevin9327@users.noreply.github.com> Date: Sun, 13 Sep 2026 06:42:23 +0900 Subject: [PATCH] fix(model): read MODELS_DIR dynamically so downloads follow a moved models folder model.py bound the models path with `from services.generator_registry import MODELS_DIR`, capturing it at import time. POST /settings/paths rebinds that module global when the models folder is moved in Settings, but /model/hf-download and /model/hf-download-sources kept resolving against the old folder, so a model downloaded afterwards landed in the previous location and still showed as not downloaded. Read registry.MODELS_DIR at call time instead, the same way generation.py reads WORKSPACE_DIR. Co-Authored-By: Claude Opus 5 --- api/routers/model.py | 13 ++++-- api/tests/test_model_router.py | 81 ++++++++++++++++++++++++++++++++-- 2 files changed, 87 insertions(+), 7 deletions(-) diff --git a/api/routers/model.py b/api/routers/model.py index 0b40d155..0a124369 100644 --- a/api/routers/model.py +++ b/api/routers/model.py @@ -10,7 +10,11 @@ from urllib.request import Request, urlopen from fastapi import APIRouter, HTTPException, Request as FastAPIRequest from fastapi.responses import StreamingResponse -from services.generator_registry import generator_registry, MODELS_DIR +# Import the module (not the name) so MODELS_DIR is read at call time: the +# settings endpoint rebinds it when the user moves the models folder, and a +# binding captured at import would keep downloading into the old one. +import services.generator_registry as registry +from services.generator_registry import generator_registry from services.model_sources import ( normalize_model_sources, resolve_download_path, @@ -140,10 +144,11 @@ async def hf_download_sources(request: FastAPIRequest, model_id: str): if raw_sources is None: raise ValueError("sources are required") sources = normalize_model_sources({"model_sources": raw_sources}) - model_root = resolve_model_root(MODELS_DIR, model_id) + models_dir = registry.MODELS_DIR + model_root = resolve_model_root(models_dir, model_id) destinations = { source["id"]: resolve_source_destination( - MODELS_DIR, model_id, source["destination"] + models_dir, model_id, source["destination"] ) for source in sources } @@ -305,7 +310,7 @@ async def hf_download( """ import json as _json import os - dest_dir = str(MODELS_DIR / model_id) + dest_dir = str(registry.MODELS_DIR / model_id) # Prefer skip_prefixes passed directly from the client (authoritative, no registry dep) if skip_prefixes: try: diff --git a/api/tests/test_model_router.py b/api/tests/test_model_router.py index 3abaef0b..62ea9a1e 100644 --- a/api/tests/test_model_router.py +++ b/api/tests/test_model_router.py @@ -7,9 +7,11 @@ from pathlib import Path from unittest.mock import patch +from fastapi import HTTPException from starlette.requests import Request import routers.model as model_router +import services.generator_registry as registry SOURCES = [ @@ -69,12 +71,22 @@ def setUp(self) -> None: self.tempdir = tempfile.TemporaryDirectory(prefix="modly-model-router-") self.models_dir = Path(self.tempdir.name) / "models" self.models_dir.mkdir() - self.old_models_dir = model_router.MODELS_DIR - model_router.MODELS_DIR = self.models_dir + self.old_models_dir = registry.MODELS_DIR + registry.MODELS_DIR = self.models_dir + # Keep the test hermetic against an import-time copy of the path: if the + # router still holds its own MODELS_DIR name, point that copy at the same + # temp tree so a stale read fails the assertions below instead of + # writing into the real models folder. + self.had_stale_models_dir = hasattr(model_router, "MODELS_DIR") + if self.had_stale_models_dir: + self.old_stale_models_dir = model_router.MODELS_DIR + model_router.MODELS_DIR = self.models_dir self.old_hf_module = sys.modules.get("huggingface_hub") def tearDown(self) -> None: - model_router.MODELS_DIR = self.old_models_dir + registry.MODELS_DIR = self.old_models_dir + if self.had_stale_models_dir: + model_router.MODELS_DIR = self.old_stale_models_dir model_router._download_controls.clear() if self.old_hf_module is None: sys.modules.pop("huggingface_hub", None) @@ -191,6 +203,69 @@ async def run(): self.assertIn("excluded from its download plan", events[-1]["error"]) self.assertFalse((self.models_dir / "pixal3d/generate/other.bin").exists()) + def move_models_folder(self) -> Path: + # What POST /settings/paths does through GeneratorRegistry.update_paths(): + # rebind the registry's MODELS_DIR while the API keeps running. + moved = Path(self.tempdir.name) / "moved-models" + moved.mkdir() + registry.MODELS_DIR = moved + return moved + + def test_multi_source_download_follows_a_moved_models_folder(self) -> None: + calls: list[str] = [] + self.install_hf_stub({"org/main": ["main.bin"]}, calls) + moved = self.move_models_folder() + + def fake_download(**kwargs): + target = Path(kwargs["dest_dir"]) / kwargs["filename"] + target.parent.mkdir(parents=True, exist_ok=True) + target.write_bytes(b"data") + return 4 + + async def run(): + with patch.object(model_router, "_download_file_streamed", fake_download): + response = await model_router.hf_download_sources( + request_for([SOURCES[0]]), "pixal3d/generate" + ) + return await collect_events(response) + + events = asyncio.run(run()) + # Reported as a success whichever folder it lands in -- which is why the + # stale path went unnoticed. + self.assertEqual(events[-1], {"percent": 100, "status": "done"}) + self.assertTrue((moved / "pixal3d/generate/main.bin").is_file()) + self.assertFalse((self.models_dir / "pixal3d/generate/main.bin").exists()) + + def test_single_repo_download_follows_a_moved_models_folder(self) -> None: + calls: list[str] = [] + self.install_hf_stub({"org/main": ["main.bin"]}, calls) + moved = self.move_models_folder() + destinations: list[str] = [] + + def fake_download(**kwargs): + destinations.append(kwargs["dest_dir"]) + return 4 + + async def run(): + with patch.object(model_router, "_download_file_streamed", fake_download): + response = await model_router.hf_download( + repo_id="org/main", + model_id="sf3d", + skip_prefixes="[]", + include_prefixes="[]", + ) + return await collect_events(response) + + events = asyncio.run(run()) + self.assertEqual(events[-1], {"percent": 100, "status": "done"}) + self.assertEqual(destinations, [str(moved / "sf3d")]) + + def test_moved_models_folder_still_refuses_a_non_node_model_id(self) -> None: + self.move_models_folder() + with self.assertRaises(HTTPException) as raised: + asyncio.run(model_router.hf_download_sources(request_for([SOURCES[0]]), "pixal3d")) + self.assertEqual(raised.exception.status_code, 400) + def test_composite_model_unload_route_uses_path_converter(self) -> None: paths = {route.path for route in model_router.router.routes} self.assertIn("/unload/{model_id:path}", paths)