mirror of
https://github.com/willmiao/ComfyUI-Lora-Manager.git
synced 2026-09-20 18:51:26 -03:00
fix(other-models): default downloads to a flat path, not {base_model}/{first_tag}
get_download_path_template() fell back to "{base_model}/{first_tag}" for any
unconfigured model type, so other-model downloads were silently nested under an
arbitrary CivitAI tag even though the settings UI exposes no template row for
"other" and priority_tags has no "other" entry (making {first_tag} resolve to
tags[0]).
Add DEFAULT_DOWNLOAD_PATH_TEMPLATES with other -> "" so unconfigured and
unknown types resolve to a flat layout under the already sub_type-scoped
default_other_roots; explicit settings.json values still win. Mirror the flat
default in the frontend DEFAULT_PATH_TEMPLATES and stop the download/move
default-path previews from rendering "/undefined" or a dangling slash.
This commit is contained in:
@@ -170,7 +170,7 @@ Flow: `POST /api/lm/download-model` (`py/routes/model_route_registrar.py:104`; G
|
|||||||
|
|
||||||
Hooks: `_record_downloaded_version_history` (model_type is free text — zero change); `_sync_downloaded_version` (`:1984` → scanner dispatch `:2130-2135`) add `other`; `py/utils/example_images_download_manager.py` scanner dispatch at `:411-421`, `:591-601`, `:1089+` — add `other` at all three (silent no-scanner otherwise).
|
Hooks: `_record_downloaded_version_history` (model_type is free text — zero change); `_sync_downloaded_version` (`:1984` → scanner dispatch `:2130-2135`) add `other`; `py/utils/example_images_download_manager.py` scanner dispatch at `:411-421`, `:591-601`, `:1089+` — add `other` at all three (silent no-scanner otherwise).
|
||||||
|
|
||||||
Path templates: `get_download_path_template("other")` already falls back to `"{base_model}/{first_tag}"` — works with zero change; optional settings-UI row (§9.4).
|
Path templates: `get_download_path_template("other")` is unset, so `other` resolves to a **flat** layout (empty template) — downloads land directly under the resolved sub_type root. This is deliberate: other-model roots are already split per sub_type (`default_other_roots`), and `priority_tags` has no `other` entry, so `{first_tag}` would fall back to an arbitrary CivitAI tag and scatter files into unstable folders. Users who want nesting can still set `download_path_templates["other"]` in `settings.json`. See `DEFAULT_DOWNLOAD_PATH_TEMPLATES` (`py/utils/constants.py`) and `DEFAULT_PATH_TEMPLATES` (`static/js/utils/constants.js`).
|
||||||
|
|
||||||
### 9.2 File-level routing (model.type / file.type → sub_type) **[locked]**
|
### 9.2 File-level routing (model.type / file.type → sub_type) **[locked]**
|
||||||
|
|
||||||
|
|||||||
@@ -25,6 +25,7 @@ from typing import (
|
|||||||
from platformdirs import user_config_dir
|
from platformdirs import user_config_dir
|
||||||
|
|
||||||
from ..utils.constants import (
|
from ..utils.constants import (
|
||||||
|
DEFAULT_DOWNLOAD_PATH_TEMPLATES,
|
||||||
DEFAULT_ENABLED_OTHER_SUB_TYPES,
|
DEFAULT_ENABLED_OTHER_SUB_TYPES,
|
||||||
DEFAULT_HASH_CHUNK_SIZE_MB,
|
DEFAULT_HASH_CHUNK_SIZE_MB,
|
||||||
DEFAULT_PRIORITY_TAG_CONFIG,
|
DEFAULT_PRIORITY_TAG_CONFIG,
|
||||||
@@ -2335,10 +2336,14 @@ class SettingsManager:
|
|||||||
"""Get download path template for specific model type
|
"""Get download path template for specific model type
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
model_type: The type of model ('lora', 'checkpoint', 'embedding')
|
model_type: The type of model ('lora', 'checkpoint', 'embedding',
|
||||||
|
'other')
|
||||||
|
|
||||||
Returns:
|
Returns:
|
||||||
Template string for the model type, defaults to '{base_model}/{first_tag}'
|
Template string for the model type. Falls back to the per-type
|
||||||
|
default in ``DEFAULT_DOWNLOAD_PATH_TEMPLATES``; unknown model types
|
||||||
|
resolve to an empty string (flat layout) rather than silently
|
||||||
|
nesting downloads under an unconfigured subfolder.
|
||||||
"""
|
"""
|
||||||
templates = self.settings.get("download_path_templates", {})
|
templates = self.settings.get("download_path_templates", {})
|
||||||
|
|
||||||
@@ -2362,27 +2367,19 @@ class SettingsManager:
|
|||||||
logger.warning(
|
logger.warning(
|
||||||
f"Failed to parse download_path_templates JSON string: {e}. Setting default values."
|
f"Failed to parse download_path_templates JSON string: {e}. Setting default values."
|
||||||
)
|
)
|
||||||
default_template = "{base_model}/{first_tag}"
|
templates = dict(DEFAULT_DOWNLOAD_PATH_TEMPLATES)
|
||||||
templates = {
|
|
||||||
"lora": default_template,
|
|
||||||
"checkpoint": default_template,
|
|
||||||
"embedding": default_template,
|
|
||||||
}
|
|
||||||
self.settings["download_path_templates"] = templates
|
self.settings["download_path_templates"] = templates
|
||||||
self._save_settings()
|
self._save_settings()
|
||||||
|
|
||||||
# Ensure templates is a dictionary
|
# Ensure templates is a dictionary
|
||||||
if not isinstance(templates, dict):
|
if not isinstance(templates, dict):
|
||||||
default_template = "{base_model}/{first_tag}"
|
templates = dict(DEFAULT_DOWNLOAD_PATH_TEMPLATES)
|
||||||
templates = {
|
|
||||||
"lora": default_template,
|
|
||||||
"checkpoint": default_template,
|
|
||||||
"embedding": default_template,
|
|
||||||
}
|
|
||||||
self.settings["download_path_templates"] = templates
|
self.settings["download_path_templates"] = templates
|
||||||
self._save_settings()
|
self._save_settings()
|
||||||
|
|
||||||
return templates.get(model_type, "{base_model}/{first_tag}")
|
return templates.get(
|
||||||
|
model_type, DEFAULT_DOWNLOAD_PATH_TEMPLATES.get(model_type, "")
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
_SETTINGS_MANAGER: Optional["SettingsManager"] = None
|
_SETTINGS_MANAGER: Optional["SettingsManager"] = None
|
||||||
|
|||||||
@@ -251,6 +251,19 @@ DEFAULT_PRIORITY_TAG_CONFIG = {
|
|||||||
"embedding": ", ".join(CIVITAI_MODEL_TAGS),
|
"embedding": ", ".join(CIVITAI_MODEL_TAGS),
|
||||||
}
|
}
|
||||||
|
|
||||||
|
# Default download path template for each model type. "other" defaults to a
|
||||||
|
# flat layout (empty template) on purpose: other-model downloads are already
|
||||||
|
# separated by sub_type roots (default_other_roots), and priority_tags has no
|
||||||
|
# "other" entry, so {first_tag} would resolve to an arbitrary CivitAI tag and
|
||||||
|
# scatter files into unstable folders. Users can still opt in to a template by
|
||||||
|
# writing "other" into download_path_templates in settings.json.
|
||||||
|
DEFAULT_DOWNLOAD_PATH_TEMPLATES: Dict[str, str] = {
|
||||||
|
"lora": "{base_model}/{first_tag}",
|
||||||
|
"checkpoint": "{base_model}/{first_tag}",
|
||||||
|
"embedding": "{base_model}/{first_tag}",
|
||||||
|
"other": "",
|
||||||
|
}
|
||||||
|
|
||||||
# baseModel values from CivitAI that should be treated as diffusion models (unet)
|
# baseModel values from CivitAI that should be treated as diffusion models (unet)
|
||||||
# These model types are incorrectly labeled as "checkpoint" by CivitAI but are actually diffusion models
|
# These model types are incorrectly labeled as "checkpoint" by CivitAI but are actually diffusion models
|
||||||
DIFFUSION_MODEL_BASE_MODELS = frozenset(
|
DIFFUSION_MODEL_BASE_MODELS = frozenset(
|
||||||
|
|||||||
@@ -2520,9 +2520,14 @@ export class DownloadManager {
|
|||||||
const singularType = this._isDiffusionModel
|
const singularType = this._isDiffusionModel
|
||||||
? 'unet'
|
? 'unet'
|
||||||
: this.apiClient.modelType.replace(/s$/, '');
|
: this.apiClient.modelType.replace(/s$/, '');
|
||||||
const templates = state.global.settings.download_path_templates;
|
const templates = state.global?.settings?.download_path_templates;
|
||||||
const template = templates[singularType];
|
const template = templates?.[singularType];
|
||||||
fullPath += `/${template}`;
|
// An empty or absent template means a flat layout: keep the
|
||||||
|
// root as-is instead of appending "/undefined" or a
|
||||||
|
// dangling slash.
|
||||||
|
if (template) {
|
||||||
|
fullPath += `/${template}`;
|
||||||
|
}
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
console.error('Failed to fetch template:', error);
|
console.error('Failed to fetch template:', error);
|
||||||
fullPath += '/' + translate('modals.download.autoOrganizedPath');
|
fullPath += '/' + translate('modals.download.autoOrganizedPath');
|
||||||
|
|||||||
@@ -226,14 +226,13 @@ class MoveManager {
|
|||||||
|
|
||||||
if (modelRoot) {
|
if (modelRoot) {
|
||||||
if (this.useDefaultPath) {
|
if (this.useDefaultPath) {
|
||||||
// Show actual template path
|
// Show actual template path; an empty/absent template means a
|
||||||
|
// flat layout, so keep the root as-is.
|
||||||
const singularType = config.singularName || apiClient.modelType.replace(/s$/, '');
|
const singularType = config.singularName || apiClient.modelType.replace(/s$/, '');
|
||||||
const templates = state.global.settings.download_path_templates;
|
const templates = state.global?.settings?.download_path_templates;
|
||||||
const template = templates[singularType];
|
const template = templates?.[singularType];
|
||||||
if (template) {
|
if (template) {
|
||||||
fullPath += `/${template}`;
|
fullPath += `/${template}`;
|
||||||
} else {
|
|
||||||
fullPath += '/' + translate('modals.download.autoOrganizedPath');
|
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
// Show manual path selection
|
// Show manual path selection
|
||||||
|
|||||||
@@ -353,7 +353,11 @@ export const DEFAULT_PATH_TEMPLATES = {
|
|||||||
lora: '{base_model}/{first_tag}',
|
lora: '{base_model}/{first_tag}',
|
||||||
checkpoint: '{base_model}',
|
checkpoint: '{base_model}',
|
||||||
unet: '{base_model}',
|
unet: '{base_model}',
|
||||||
embedding: '{first_tag}'
|
embedding: '{first_tag}',
|
||||||
|
// Other models (VAE/upscaler/...) default to a flat layout: their root is
|
||||||
|
// already split per sub_type, and priority_tags has no "other" entry, so
|
||||||
|
// {first_tag} would resolve to an arbitrary CivitAI tag.
|
||||||
|
other: ''
|
||||||
};
|
};
|
||||||
|
|
||||||
// Model type labels for UI
|
// Model type labels for UI
|
||||||
|
|||||||
@@ -0,0 +1,183 @@
|
|||||||
|
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||||
|
|
||||||
|
const {
|
||||||
|
DOWNLOAD_MANAGER_MODULE,
|
||||||
|
MODAL_MANAGER_MODULE,
|
||||||
|
UI_HELPERS_MODULE,
|
||||||
|
STATE_MODULE,
|
||||||
|
LOADING_MANAGER_MODULE,
|
||||||
|
API_FACTORY_MODULE,
|
||||||
|
STORAGE_HELPERS_MODULE,
|
||||||
|
FOLDER_TREE_MANAGER_MODULE,
|
||||||
|
I18N_HELPERS_MODULE,
|
||||||
|
SUMMARY_MODULE,
|
||||||
|
mockApiClient,
|
||||||
|
mockLoadingManager,
|
||||||
|
mockFolderTreeManager,
|
||||||
|
mockState,
|
||||||
|
} = vi.hoisted(() => {
|
||||||
|
const mockApiClient = {
|
||||||
|
modelType: 'loras',
|
||||||
|
apiConfig: {
|
||||||
|
config: {
|
||||||
|
displayName: 'LoRA',
|
||||||
|
singularName: 'lora',
|
||||||
|
},
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
|
const mockLoadingManager = {
|
||||||
|
showSimpleLoading: vi.fn(),
|
||||||
|
setStatus: vi.fn(),
|
||||||
|
hide: vi.fn(),
|
||||||
|
};
|
||||||
|
|
||||||
|
const mockFolderTreeManager = {
|
||||||
|
getSelectedPath: vi.fn(() => ''),
|
||||||
|
};
|
||||||
|
|
||||||
|
const mockState = {
|
||||||
|
global: {
|
||||||
|
settings: {
|
||||||
|
download_path_templates: {},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
loadingManager: mockLoadingManager,
|
||||||
|
};
|
||||||
|
|
||||||
|
return {
|
||||||
|
DOWNLOAD_MANAGER_MODULE: new URL('../../../static/js/managers/DownloadManager.js', import.meta.url).pathname,
|
||||||
|
MODAL_MANAGER_MODULE: new URL('../../../static/js/managers/ModalManager.js', import.meta.url).pathname,
|
||||||
|
UI_HELPERS_MODULE: new URL('../../../static/js/utils/uiHelpers.js', import.meta.url).pathname,
|
||||||
|
STATE_MODULE: new URL('../../../static/js/state/index.js', import.meta.url).pathname,
|
||||||
|
LOADING_MANAGER_MODULE: new URL('../../../static/js/managers/LoadingManager.js', import.meta.url).pathname,
|
||||||
|
API_FACTORY_MODULE: new URL('../../../static/js/api/modelApiFactory.js', import.meta.url).pathname,
|
||||||
|
STORAGE_HELPERS_MODULE: new URL('../../../static/js/utils/storageHelpers.js', import.meta.url).pathname,
|
||||||
|
FOLDER_TREE_MANAGER_MODULE: new URL('../../../static/js/components/FolderTreeManager.js', import.meta.url).pathname,
|
||||||
|
I18N_HELPERS_MODULE: new URL('../../../static/js/utils/i18nHelpers.js', import.meta.url).pathname,
|
||||||
|
SUMMARY_MODULE: new URL('../../../static/js/components/DownloadBatchSummaryModal.js', import.meta.url).pathname,
|
||||||
|
mockApiClient,
|
||||||
|
mockLoadingManager,
|
||||||
|
mockFolderTreeManager,
|
||||||
|
mockState,
|
||||||
|
};
|
||||||
|
});
|
||||||
|
|
||||||
|
vi.mock(MODAL_MANAGER_MODULE, () => ({
|
||||||
|
modalManager: {
|
||||||
|
showModal: vi.fn(),
|
||||||
|
closeModal: vi.fn(),
|
||||||
|
},
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock(UI_HELPERS_MODULE, () => ({
|
||||||
|
showToast: vi.fn(),
|
||||||
|
showActionToast: vi.fn(),
|
||||||
|
setupAutoNewlineOnPaste: vi.fn(),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock(STATE_MODULE, () => ({
|
||||||
|
state: mockState,
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock(LOADING_MANAGER_MODULE, () => ({
|
||||||
|
LoadingManager: vi.fn(() => mockLoadingManager),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock(API_FACTORY_MODULE, () => ({
|
||||||
|
getModelApiClient: vi.fn(() => mockApiClient),
|
||||||
|
resetAndReload: vi.fn(),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock(STORAGE_HELPERS_MODULE, () => ({
|
||||||
|
getStorageItem: vi.fn((_key, defaultValue) => defaultValue),
|
||||||
|
setStorageItem: vi.fn(),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock(FOLDER_TREE_MANAGER_MODULE, () => ({
|
||||||
|
FolderTreeManager: vi.fn(() => mockFolderTreeManager),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock(I18N_HELPERS_MODULE, () => ({
|
||||||
|
translate: vi.fn((_, __, fallback) => fallback ?? ''),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock(SUMMARY_MODULE, () => ({
|
||||||
|
showDownloadBatchSummary: vi.fn(),
|
||||||
|
}));
|
||||||
|
|
||||||
|
describe('DownloadManager default-path preview', () => {
|
||||||
|
let DownloadManager;
|
||||||
|
|
||||||
|
beforeEach(async () => {
|
||||||
|
vi.resetModules();
|
||||||
|
vi.clearAllMocks();
|
||||||
|
({ DownloadManager } = await import(DOWNLOAD_MANAGER_MODULE));
|
||||||
|
|
||||||
|
document.body.innerHTML = `
|
||||||
|
<select id="modelRoot"><option value="/models/vae">/models/vae</option></select>
|
||||||
|
<div id="targetPathDisplay"></div>
|
||||||
|
`;
|
||||||
|
document.getElementById('modelRoot').value = '/models/vae';
|
||||||
|
|
||||||
|
mockState.global.settings.download_path_templates = {};
|
||||||
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
document.body.innerHTML = '';
|
||||||
|
});
|
||||||
|
|
||||||
|
function useOtherClient(manager) {
|
||||||
|
mockApiClient.modelType = 'other';
|
||||||
|
mockApiClient.apiConfig.config = { displayName: 'Other Model', singularName: 'other' };
|
||||||
|
manager.apiClient = mockApiClient;
|
||||||
|
}
|
||||||
|
|
||||||
|
it('renders the bare root for a flat (empty) other template', () => {
|
||||||
|
mockState.global.settings.download_path_templates = { other: '' };
|
||||||
|
const manager = new DownloadManager();
|
||||||
|
useOtherClient(manager);
|
||||||
|
manager.useDefaultPath = true;
|
||||||
|
|
||||||
|
manager.updateTargetPath();
|
||||||
|
|
||||||
|
const text = document.getElementById('targetPathDisplay').textContent;
|
||||||
|
expect(text).toBe('/models/vae');
|
||||||
|
expect(text).not.toContain('undefined');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('renders the bare root when the other template key is absent', () => {
|
||||||
|
mockState.global.settings.download_path_templates = {};
|
||||||
|
const manager = new DownloadManager();
|
||||||
|
useOtherClient(manager);
|
||||||
|
manager.useDefaultPath = true;
|
||||||
|
|
||||||
|
manager.updateTargetPath();
|
||||||
|
|
||||||
|
const text = document.getElementById('targetPathDisplay').textContent;
|
||||||
|
expect(text).toBe('/models/vae');
|
||||||
|
expect(text).not.toContain('undefined');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('appends a configured template to the root', () => {
|
||||||
|
mockState.global.settings.download_path_templates = { other: '{base_model}' };
|
||||||
|
const manager = new DownloadManager();
|
||||||
|
useOtherClient(manager);
|
||||||
|
manager.useDefaultPath = true;
|
||||||
|
|
||||||
|
manager.updateTargetPath();
|
||||||
|
|
||||||
|
expect(document.getElementById('targetPathDisplay').textContent).toBe('/models/vae/{base_model}');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('renders the manual selection when default paths are off', () => {
|
||||||
|
mockFolderTreeManager.getSelectedPath.mockReturnValue('nested/folder');
|
||||||
|
const manager = new DownloadManager();
|
||||||
|
useOtherClient(manager);
|
||||||
|
manager.useDefaultPath = false;
|
||||||
|
|
||||||
|
manager.updateTargetPath();
|
||||||
|
|
||||||
|
expect(document.getElementById('targetPathDisplay').textContent).toBe('/models/vae/nested/folder');
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -428,6 +428,29 @@ async def test_default_paths_use_per_sub_type_root(
|
|||||||
|
|
||||||
assert result["success"] is True
|
assert result["success"] is True
|
||||||
assert str(tmp_path / "vae") in str(captured["save_dir"])
|
assert str(tmp_path / "vae") in str(captured["save_dir"])
|
||||||
|
assert captured["relative_path"] == ""
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_default_paths_other_is_flat_without_configured_template(
|
||||||
|
monkeypatch, scanners, metadata_provider, tmp_path
|
||||||
|
):
|
||||||
|
"""Regression: an unconfigured 'other' template must resolve to a flat
|
||||||
|
layout at the sub_type root instead of the {base_model}/{first_tag}
|
||||||
|
fallback (which scattered files into arbitrary CivitAI-tag folders)."""
|
||||||
|
get_settings_manager().settings["download_path_templates"].pop("other", None)
|
||||||
|
|
||||||
|
captured = {}
|
||||||
|
_capture_execute(monkeypatch, captured)
|
||||||
|
|
||||||
|
manager = DownloadManager()
|
||||||
|
result = await manager.download_from_civitai(
|
||||||
|
model_version_id=99, use_default_paths=True
|
||||||
|
)
|
||||||
|
|
||||||
|
assert result["success"] is True
|
||||||
|
assert captured["relative_path"] == ""
|
||||||
|
assert str(tmp_path / "vae") in str(captured["save_dir"])
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
|
|||||||
@@ -345,6 +345,26 @@ def test_download_path_template_invalid_json(manager):
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_download_path_template_defaults_flat_for_other(manager):
|
||||||
|
"""'other' has no settings-UI row and no priority_tags entry, so an
|
||||||
|
unconfigured template must resolve to a flat layout rather than the
|
||||||
|
{base_model}/{first_tag} fallback (which scatters files into arbitrary
|
||||||
|
CivitAI-tag folders)."""
|
||||||
|
manager.settings["download_path_templates"] = {}
|
||||||
|
|
||||||
|
assert manager.get_download_path_template("other") == ""
|
||||||
|
|
||||||
|
# An explicit user configuration still wins.
|
||||||
|
manager.settings["download_path_templates"] = {"other": "{base_model}"}
|
||||||
|
assert manager.get_download_path_template("other") == "{base_model}"
|
||||||
|
|
||||||
|
|
||||||
|
def test_download_path_template_unknown_type_is_flat(manager):
|
||||||
|
manager.settings["download_path_templates"] = {}
|
||||||
|
|
||||||
|
assert manager.get_download_path_template("not-a-model-type") == ""
|
||||||
|
|
||||||
|
|
||||||
def test_auto_set_default_roots(manager):
|
def test_auto_set_default_roots(manager):
|
||||||
# Clear any previously auto-set values to test fresh behavior
|
# Clear any previously auto-set values to test fresh behavior
|
||||||
manager.settings["default_lora_root"] = ""
|
manager.settings["default_lora_root"] = ""
|
||||||
|
|||||||
Reference in New Issue
Block a user