Skip to content

fix(model): read MODELS_DIR dynamically so downloads follow a moved models folder - #339

Open
kevin9327 wants to merge 1 commit into
lightningpixel:devfrom
kevin9327:fix/model-download-dynamic-models-dir
Open

kevin9327 wants to merge 1 commit into
lightningpixel:devfrom
kevin9327:fix/model-download-dynamic-models-dir

Conversation

@kevin9327

Copy link
Copy Markdown
Contributor

What

After changing the models folder in Settings → Storage, downloading a model from the Models page writes its weights into the previous folder until the app is restarted. The download reports success, but the model still shows as not downloaded, because the registry and Electron both check the new folder. If the old folder was moved or deleted, the download silently recreates it.

Why it triggers

api/routers/model.py imported the path by name:

from services.generator_registry import generator_registry, MODELS_DIR

POST /settings/pathsGeneratorRegistry.update_paths() rebinds the module global and repoints every generator's model_dir, but the router's own copy keeps the import-time value:

model_root = resolve_model_root(MODELS_DIR, model_id)   # /model/hf-download-sources
dest_dir  = str(MODELS_DIR / model_id)                   # /model/hf-download

This is the models-folder counterpart of #287, which fixed the same stale binding for WORKSPACE_DIR in generation.py.

Fix

Import the module and read registry.MODELS_DIR at call time, as generation.py and the settings router already do. In hf_download_sources it is read once into a local, so the model root and every source destination come from the same folder.

Tests

The existing fixture redirected model_router.MODELS_DIR, a name that no longer exists after the fix, so it now redirects registry.MODELS_DIR. Following test_generation_router.py, it also points any stale module copy at the same temp tree. That way a stale read fails an assertion instead of writing into the real models folder.

New in api/tests/test_model_router.py:

  • test_multi_source_download_follows_a_moved_models_folder: fails before, passes after.
  • test_single_repo_download_follows_a_moved_models_folder: fails before, passes after.
  • test_moved_models_folder_still_refuses_a_non_node_model_id: passes before and after. A model id that doesn't name one extension node is still rejected with 400 after the folder moves. That proves reading the live path doesn't loosen the model-root validation, so the change is not a widening.

The multi-source test's events[-1] == {"percent": 100, "status": "done"} assertion also holds before and after. The stale path always reported success, which is why it went unnoticed.

Fail-before output, with api/routers/model.py reverted and the tests kept. It is verbatim except that the local checkout/temp path prefix is replaced with <repo> / <tmp>, and the registry's import-time path banner that follows it is omitted:

test_composite_model_unload_route_uses_path_converter (tests.test_model_router.MultiSourceRouterTests.test_composite_model_unload_route_uses_path_converter) ... ok
test_lists_every_source_before_sequential_download_with_monotonic_progress (tests.test_model_router.MultiSourceRouterTests.test_lists_every_source_before_sequential_download_with_monotonic_progress) ... ok
test_moved_models_folder_still_refuses_a_non_node_model_id (tests.test_model_router.MultiSourceRouterTests.test_moved_models_folder_still_refuses_a_non_node_model_id) ... ok
test_multi_source_download_follows_a_moved_models_folder (tests.test_model_router.MultiSourceRouterTests.test_multi_source_download_follows_a_moved_models_folder) ... FAIL
test_pause_cancel_and_resume_reuse_one_model_control (tests.test_model_router.MultiSourceRouterTests.test_pause_cancel_and_resume_reuse_one_model_control) ... ok
test_rejects_a_check_filtered_out_of_the_source_plan (tests.test_model_router.MultiSourceRouterTests.test_rejects_a_check_filtered_out_of_the_source_plan) ... ok
test_single_repo_download_follows_a_moved_models_folder (tests.test_model_router.MultiSourceRouterTests.test_single_repo_download_follows_a_moved_models_folder) ... FAIL

======================================================================
FAIL: test_multi_source_download_follows_a_moved_models_folder (tests.test_model_router.MultiSourceRouterTests.test_multi_source_download_follows_a_moved_models_folder)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "<repo>\api\tests\test_model_router.py", line 236, in test_multi_source_download_follows_a_moved_models_folder
    self.assertTrue((moved / "pixal3d/generate/main.bin").is_file())
AssertionError: False is not true

======================================================================
FAIL: test_single_repo_download_follows_a_moved_models_folder (tests.test_model_router.MultiSourceRouterTests.test_single_repo_download_follows_a_moved_models_folder)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "<repo>\api\tests\test_model_router.py", line 261, in test_single_repo_download_follows_a_moved_models_folder
    self.assertEqual(destinations, [str(moved / "sf3d")])
AssertionError: Lists differ: ['C:\[23 chars]\Local\\Temp\\modly-model-router-k9vq458d\\models\\sf3d'] != ['C:\[23 chars]\Local\\Temp\\modly-model-router-k9vq458d\\moved-models\\sf3d']

First differing element 0:
'C:\\[21 chars]\\Local\\Temp\\modly-model-router-k9vq458d\\models\\sf3d'
'C:\\[21 chars]\\Local\\Temp\\modly-model-router-k9vq458d\\moved-models\\sf3d'

- ['<tmp>\\modly-model-router-k9vq458d\\models\\sf3d']
+ ['<tmp>\\modly-model-router-k9vq458d\\moved-models\\sf3d']


----------------------------------------------------------------------
Ran 7 tests in 14.134s

FAILED (failures=2)

After the fix, python -m unittest tests.test_model_router passes all 7 tests (Ran 7 tests in 14.102s / OK).

Whole suite, python -m unittest discover -s tests in api/ (venv with fastapi + python-multipart + httpx):

dev this branch
Whole suite Ran 93 tests / OK (skipped=3) Ran 96 tests / OK (skipped=3)
test_model_router.py 4 pass 7 pass

The 3 skips were already there on dev.

Lint: the repo configures no Python linter, and ESLint ignores api/**. As a spot check, ruff check --isolated on the two touched files reports 32 findings on dev and the same 32 on this branch, so nothing new. No TypeScript touched.

🤖 Generated with Claude Code

…odels 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 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant