mirror of
https://github.com/willmiao/ComfyUI-Lora-Manager.git
synced 2026-09-21 19:21:27 -03:00
fix(other-models): make clip_vision opt-in like controlnet
DEFAULT_ENABLED_OTHER_SUB_TYPES managed vae, upscaler, text_encoder and clip_vision while controlnet was the sole opt-in type. That split was not defensible on demand breadth: ControlNet is the broader category by install base, and clip_vision is the narrower one (IPAdapter/SVD image conditioning, usually one to three files) whose CivitAI type is retired upstream. Keep the default set to the dependency-style assets every pipeline needs and where "which one am I actually using" is the real problem - VAE, upscalers and text encoders - and treat clip_vision and controlnet symmetrically as opt-in. The feature is still unreleased, so the change needs no migration. - Sync all five surfaces holding a default: DEFAULT_ENABLED_OTHER_SUB_TYPES, DEFAULT_SETTINGS, both DEFAULT_SETTINGS_BASE/createDefaultSettings lists, updateOtherModelsControls()'s fallback and the Jinja fallback. - The selection is persisted per user, so only the untouched default moves; existing default_other_roots entries for a disabled sub_type are preserved. - Fix the Jinja fallback using `or`, which treated an all-unchecked empty allow-list as "unset" and re-checked every box on render; `is none` keeps the empty list empty. - Document the revised defaults and rationale in the plan. Tests assert the new default trio, the normalize fallback, that both opt-in types stay out of the default scan, and the auto-set iteration test now enables clip_vision explicitly since it exercises the loop, not the default.
This commit is contained in:
@@ -1,6 +1,6 @@
|
|||||||
# Plan: "Other Models" Page — Unified Management for VAE / Upscaler / Text Encoder / etc.
|
# Plan: "Other Models" Page — Unified Management for VAE / Upscaler / Text Encoder / etc.
|
||||||
|
|
||||||
**Status:** v2 — **Phase 1 implemented** (2026-09-12, commits `27da7b3c` backend + `fa7ce725` frontend; verified live against a running ComfyUI instance: scan/hash/sub_type-derivation/fetch/previews all green). **Phase 2 implemented** (2026-09-12, per §9 design; full pytest + vitest green). **Phase 3 implemented** (§11: opt-in management toggles; default off). **i18n done** (2026-09-13): all 36 new keys translated in the 9 non-English locales — the `[TODO: Translate]` placeholders left by the sync script during development are gone (see `docs/i18n-translation-guidelines.md` §2, "Other Models feature").
|
**Status:** v2 — **Phase 1 implemented** (2026-09-12, commits `27da7b3c` backend + `fa7ce725` frontend; verified live against a running ComfyUI instance: scan/hash/sub_type-derivation/fetch/previews all green). **Phase 2 implemented** (2026-09-12, per §9 design; full pytest + vitest green). **Phase 3 implemented** (§11: opt-in management toggles; default off). **i18n done** (2026-09-13): all 36 new keys translated in the 9 non-English locales — the `[TODO: Translate]` placeholders left by the sync script during development are gone (see `docs/i18n-translation-guidelines.md` §2, "Other Models feature"). **Default set revised (pre-release):** only `vae` / `upscaler` / `text_encoder` are managed by default — `clip_vision` and `controlnet` are both opt-in (§2, §11.1.1).
|
||||||
**Scope (Phase 1):** scan + manage (list, search, filter, tags, folders, preview, rename, move, delete/exclude, CivitAI metadata fetch) for a new model type `other`, exposed as a new web page. **Phase 2 (§9):** one-click download from CivitAI for these types.
|
**Scope (Phase 1):** scan + manage (list, search, filter, tags, folders, preview, rename, move, delete/exclude, CivitAI metadata fetch) for a new model type `other`, exposed as a new web page. **Phase 2 (§9):** one-click download from CivitAI for these types.
|
||||||
|
|
||||||
## 1. Goal
|
## 1. Goal
|
||||||
@@ -32,11 +32,20 @@ Add a fourth page that manages "everything else" — VAE, upscalers, text encode
|
|||||||
| `vae` | `vae` | `VAE` | yes |
|
| `vae` | `vae` | `VAE` | yes |
|
||||||
| `upscaler` | `upscale_models` | `Upscaler` | yes |
|
| `upscaler` | `upscale_models` | `Upscaler` | yes |
|
||||||
| `text_encoder` | `text_encoders`, `clip` (legacy) | `TextEncoder` (CLIP is retired upstream) | yes |
|
| `text_encoder` | `text_encoders`, `clip` (legacy) | `TextEncoder` (CLIP is retired upstream) | yes |
|
||||||
| `clip_vision` | `clip_vision` | `CLIPVision` | yes |
|
| `clip_vision` | `clip_vision` | `CLIPVision` | no (mapping present, opt-in) |
|
||||||
| `controlnet` | `controlnet` | `Controlnet` | no (mapping present, opt-in) |
|
| `controlnet` | `controlnet` | `Controlnet` | no (mapping present, opt-in) |
|
||||||
|
|
||||||
New folder categories = one line in the mapping table (see §4.1).
|
New folder categories = one line in the mapping table (see §4.1).
|
||||||
|
|
||||||
|
**Why only three are on by default** (revised in Phase 3, before release):
|
||||||
|
VAE, upscalers and text encoders are dependency-style assets every pipeline
|
||||||
|
needs, and "which one am I actually using" is the recurring problem they
|
||||||
|
solve. `clip_vision` and `controlnet` are workflow-driven instead
|
||||||
|
(IPAdapter/SVD image conditioning; per-workflow ControlNet variants), and
|
||||||
|
ControlNet libraries routinely run to dozens of files, so both are treated
|
||||||
|
symmetrically as opt-in. Enumerating all five as "the default set" was not
|
||||||
|
defensible on demand breadth alone.
|
||||||
|
|
||||||
4. **Phase 1 = scan/manage only.** Downloads from CivitAI (`download_manager.py` type mapping, default-root settings keys, download routing) are Phase 2 (§9). CivitAI **metadata fetch** for existing files IS in Phase 1 (hash-based lookup is type-agnostic; only the type-validation hook needs new values).
|
4. **Phase 1 = scan/manage only.** Downloads from CivitAI (`download_manager.py` type mapping, default-root settings keys, download routing) are Phase 2 (§9). CivitAI **metadata fetch** for existing files IS in Phase 1 (hash-based lookup is type-agnostic; only the type-validation hook needs new values).
|
||||||
|
|
||||||
5. **Out of scope (default off, revisit later):** usage statistics buckets, recipe matching (`recipe_scanner.py` only merges lora+checkpoint scanners), statistics page, embeddings re-classification (stays its own page — merging would be a breaking change).
|
5. **Out of scope (default off, revisit later):** usage statistics buckets, recipe matching (`recipe_scanner.py` only merges lora+checkpoint scanners), statistics page, embeddings re-classification (stays its own page — merging would be a breaking change).
|
||||||
@@ -262,13 +271,35 @@ page shows an "enable" empty state until the user turns it on.
|
|||||||
| key | type | default | meaning |
|
| key | type | default | meaning |
|
||||||
|---|---|---|---|
|
|---|---|---|---|
|
||||||
| `enable_other_models` | bool | `false` | master switch |
|
| `enable_other_models` | bool | `false` | master switch |
|
||||||
| `enabled_other_sub_types` | list[str] | `["vae","upscaler","text_encoder","clip_vision"]` | allow-list; controlnet still opt-in |
|
| `enabled_other_sub_types` | list[str] | `["vae","upscaler","text_encoder"]` | allow-list; `clip_vision` and `controlnet` are opt-in (see §2) |
|
||||||
|
|
||||||
`enabled_other_folders` (the unreleased, additive, no-UI backend key) was removed
|
`enabled_other_folders` (the unreleased, additive, no-UI backend key) was removed
|
||||||
and replaced by the sub_type-level allow-list; there is no migration because the
|
and replaced by the sub_type-level allow-list; there is no migration because the
|
||||||
feature never shipped. `text_encoder` expands to `text_encoders` + legacy `clip`
|
feature never shipped. `text_encoder` expands to `text_encoders` + legacy `clip`
|
||||||
via `OTHER_SUB_TYPE_FOLDER_KEYS`.
|
via `OTHER_SUB_TYPE_FOLDER_KEYS`.
|
||||||
|
|
||||||
|
The default allow-list lives on five surfaces that must stay in sync:
|
||||||
|
`DEFAULT_ENABLED_OTHER_SUB_TYPES` (`py/utils/constants.py`), `DEFAULT_SETTINGS`
|
||||||
|
(`py/services/settings_manager.py`), the two `DEFAULT_SETTINGS_BASE` /
|
||||||
|
`createDefaultSettings` lists (`static/js/state/index.js`), the
|
||||||
|
`updateOtherModelsControls()` fallback (`static/js/managers/SettingsManager.js`)
|
||||||
|
and the server-rendered Jinja fallback
|
||||||
|
(`templates/components/modals/settings/library.html`).
|
||||||
|
|
||||||
|
### 11.1.1 Legacy key handling in `Config._init_other_paths`
|
||||||
|
|
||||||
|
ComfyUI's `folder_paths` rewrites legacy names before every access (`clip` →
|
||||||
|
`text_encoders`, `unet` → `diffusion_models`) and registers both legacy
|
||||||
|
directories under the canonical key, so `get_folder_paths("clip")` returns
|
||||||
|
exactly the same list as `get_folder_paths("text_encoders")`. Querying both keys
|
||||||
|
made the overlap guard fire twice with `please fix your path configuration` for a
|
||||||
|
configuration the user cannot fix. `Config._collapse_legacy_folder_keys()` now
|
||||||
|
drops a key when the host exposes `map_legacy` and resolves it to another queried
|
||||||
|
key, and `_prepare_other_paths()` downgrades a same-`sub_type` duplicate to
|
||||||
|
`debug` (a cross-`sub_type` collision still warns). In standalone mode
|
||||||
|
`MockFolderPaths` has no `map_legacy` and its keys are independent
|
||||||
|
`settings.json` entries, so every key is still queried there.
|
||||||
|
|
||||||
`settings.json.example` intentionally stays minimal (only `use_portable_settings`,
|
`settings.json.example` intentionally stays minimal (only `use_portable_settings`,
|
||||||
`civitai_api_key`, and the four core `folder_paths` keys: `loras`, `checkpoints`,
|
`civitai_api_key`, and the four core `folder_paths` keys: `loras`, `checkpoints`,
|
||||||
`unet`, `embeddings`). Optional keys — including the other-model folder paths and
|
`unet`, `embeddings`). Optional keys — including the other-model folder paths and
|
||||||
|
|||||||
@@ -97,11 +97,17 @@ VALID_OTHER_SUB_TYPES = ["vae", "upscaler", "text_encoder", "clip_vision", "cont
|
|||||||
# Sub-types managed when the (opt-in) Other Models feature is switched on.
|
# Sub-types managed when the (opt-in) Other Models feature is switched on.
|
||||||
# The feature itself defaults to off (``enable_other_models`` = False), so
|
# The feature itself defaults to off (``enable_other_models`` = False), so
|
||||||
# nothing here is scanned until the user enables it.
|
# nothing here is scanned until the user enables it.
|
||||||
|
#
|
||||||
|
# The default set is deliberately limited to the dependency-style assets every
|
||||||
|
# pipeline needs and where "which one am I actually using" is the real problem:
|
||||||
|
# VAE, upscalers and text encoders. ``clip_vision`` and ``controlnet`` are
|
||||||
|
# workflow-driven instead (IPAdapter/SVD, per-workflow ControlNet variants) and
|
||||||
|
# ControlNet libraries routinely run to dozens of files, so both stay opt-in
|
||||||
|
# and are treated symmetrically.
|
||||||
DEFAULT_ENABLED_OTHER_SUB_TYPES: List[str] = [
|
DEFAULT_ENABLED_OTHER_SUB_TYPES: List[str] = [
|
||||||
"vae",
|
"vae",
|
||||||
"upscaler",
|
"upscaler",
|
||||||
"text_encoder",
|
"text_encoder",
|
||||||
"clip_vision",
|
|
||||||
]
|
]
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -2429,7 +2429,7 @@ export class SettingsManager {
|
|||||||
const enableOtherModels = !!state.global.settings.enable_other_models;
|
const enableOtherModels = !!state.global.settings.enable_other_models;
|
||||||
const enabledSubTypes = new Set(
|
const enabledSubTypes = new Set(
|
||||||
state.global.settings.enabled_other_sub_types
|
state.global.settings.enabled_other_sub_types
|
||||||
|| ['vae', 'upscaler', 'text_encoder', 'clip_vision']
|
|| ['vae', 'upscaler', 'text_encoder']
|
||||||
);
|
);
|
||||||
|
|
||||||
document.querySelectorAll('[data-other-subtype-toggle]').forEach((input) => {
|
document.querySelectorAll('[data-other-subtype-toggle]').forEach((input) => {
|
||||||
|
|||||||
@@ -26,7 +26,7 @@ const DEFAULT_SETTINGS_BASE = Object.freeze({
|
|||||||
default_embedding_root: '',
|
default_embedding_root: '',
|
||||||
default_other_roots: {},
|
default_other_roots: {},
|
||||||
enable_other_models: false,
|
enable_other_models: false,
|
||||||
enabled_other_sub_types: ['vae', 'upscaler', 'text_encoder', 'clip_vision'],
|
enabled_other_sub_types: ['vae', 'upscaler', 'text_encoder'],
|
||||||
recipes_path: '',
|
recipes_path: '',
|
||||||
base_model_path_mappings: {},
|
base_model_path_mappings: {},
|
||||||
download_path_templates: {},
|
download_path_templates: {},
|
||||||
@@ -76,7 +76,7 @@ export function createDefaultSettings() {
|
|||||||
download_path_templates: { ...DEFAULT_PATH_TEMPLATES },
|
download_path_templates: { ...DEFAULT_PATH_TEMPLATES },
|
||||||
priority_tags: { ...DEFAULT_PRIORITY_TAG_CONFIG },
|
priority_tags: { ...DEFAULT_PRIORITY_TAG_CONFIG },
|
||||||
default_other_roots: {},
|
default_other_roots: {},
|
||||||
enabled_other_sub_types: ['vae', 'upscaler', 'text_encoder', 'clip_vision'],
|
enabled_other_sub_types: ['vae', 'upscaler', 'text_encoder'],
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -41,7 +41,10 @@
|
|||||||
|
|
||||||
{{ sm.setting_toggle('enableOtherModels', 'enable_other_models', 'settings.folderSettings.enableOtherModels', 'settings.folderSettings.enableOtherModelsHelp') }}
|
{{ sm.setting_toggle('enableOtherModels', 'enable_other_models', 'settings.folderSettings.enableOtherModels', 'settings.folderSettings.enableOtherModelsHelp') }}
|
||||||
|
|
||||||
{% set enabled_other_sub_types = settings.get('enabled_other_sub_types') or ['vae', 'upscaler', 'text_encoder', 'clip_vision'] %}
|
{# 'is none' (not 'or') so an empty allow-list stays empty instead of
|
||||||
|
falling back to the defaults and re-checking every box. #}
|
||||||
|
{% set enabled_other_sub_types = settings.get('enabled_other_sub_types') %}
|
||||||
|
{% if enabled_other_sub_types is none %}{% set enabled_other_sub_types = ['vae', 'upscaler', 'text_encoder'] %}{% endif %}
|
||||||
<div class="setting-item other-subtype-toggles" id="otherSubTypeToggles">
|
<div class="setting-item other-subtype-toggles" id="otherSubTypeToggles">
|
||||||
<div class="setting-row">
|
<div class="setting-row">
|
||||||
<div class="setting-info">
|
<div class="setting-info">
|
||||||
|
|||||||
@@ -1248,6 +1248,30 @@ def test_other_models_disabled_by_default(manager):
|
|||||||
assert manager.is_other_sub_type_enabled("vae") is False
|
assert manager.is_other_sub_type_enabled("vae") is False
|
||||||
|
|
||||||
|
|
||||||
|
def test_default_enabled_other_sub_types_are_the_dependency_trio(manager):
|
||||||
|
"""clip_vision and controlnet are workflow-driven, so both stay opt-in."""
|
||||||
|
manager.settings["enable_other_models"] = True
|
||||||
|
|
||||||
|
assert manager.get_enabled_other_sub_types() == [
|
||||||
|
"vae",
|
||||||
|
"upscaler",
|
||||||
|
"text_encoder",
|
||||||
|
]
|
||||||
|
assert manager.is_other_sub_type_enabled("clip_vision") is False
|
||||||
|
assert manager.is_other_sub_type_enabled("controlnet") is False
|
||||||
|
|
||||||
|
|
||||||
|
def test_enabled_other_sub_types_falls_back_to_defaults(manager):
|
||||||
|
manager.settings["enable_other_models"] = True
|
||||||
|
manager.settings["enabled_other_sub_types"] = None
|
||||||
|
|
||||||
|
assert manager.get_enabled_other_sub_types() == [
|
||||||
|
"vae",
|
||||||
|
"upscaler",
|
||||||
|
"text_encoder",
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
def test_auto_set_default_other_roots_skipped_when_feature_off(manager):
|
def test_auto_set_default_other_roots_skipped_when_feature_off(manager):
|
||||||
manager.settings["enable_other_models"] = False
|
manager.settings["enable_other_models"] = False
|
||||||
manager.settings["default_other_roots"] = {}
|
manager.settings["default_other_roots"] = {}
|
||||||
@@ -1270,6 +1294,8 @@ def test_set_enabled_other_sub_types_normalizes(manager):
|
|||||||
def test_auto_set_default_other_roots(manager):
|
def test_auto_set_default_other_roots(manager):
|
||||||
manager.settings["enable_other_models"] = True
|
manager.settings["enable_other_models"] = True
|
||||||
manager.settings["default_other_roots"] = {}
|
manager.settings["default_other_roots"] = {}
|
||||||
|
# clip_vision is opt-in, so enable it explicitly for this iteration test.
|
||||||
|
manager.settings["enabled_other_sub_types"] = ["vae", "upscaler", "clip_vision"]
|
||||||
manager.settings["folder_paths"] = {
|
manager.settings["folder_paths"] = {
|
||||||
"vae": ["/vae"],
|
"vae": ["/vae"],
|
||||||
"upscale_models": ["/upscalers"],
|
"upscale_models": ["/upscalers"],
|
||||||
|
|||||||
Reference in New Issue
Block a user