From 4d87ae763782731aae884b653172c3b7d5c51389 Mon Sep 17 00:00:00 2001 From: Will Miao Date: Sun, 13 Sep 2026 20:12:45 +0800 Subject: [PATCH] fix(other-models): stop warning about legacy folder keys that alias Enabling Other Models logged two warnings on a stock ComfyUI install: Detected the same folder '.../clip' under multiple other-model categories ('.../clip' is already mapped). Keeping the first category; please fix your path configuration. Nothing was wrong with the configuration. ComfyUI's folder_paths rewrites legacy names before every access (map_legacy: 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"). Both keys are in the enabled allow-list, so the second pass hit the overlap guard for every text-encoder folder and printed advice the user cannot act on. The path list itself was correct (deduped), only the message was wrong. - Config._collapse_legacy_folder_keys() drops a key when the host exposes map_legacy and resolves it to another queried key. That is provably lossless: an empty canonical list implies an empty alias list. The standalone MockFolderPaths has no map_legacy and its keys are independent settings.json entries, so every key is still queried there. - _prepare_other_paths() now tracks the claiming sub_type alongside the business path and downgrades a same-sub_type duplicate to debug, keeping the warning for a genuine cross-category collision (and naming the other category in the message). Regression tests cover the aliased-key layout (no warning, no redundant query, both folders still managed) and the same-sub_type duplicate, and the opt-in test is parametrized over controlnet and clip_vision. --- py/config.py | 83 ++++++++++++++++++--- tests/config/test_other_paths.py | 122 ++++++++++++++++++++++++++++--- 2 files changed, 182 insertions(+), 23 deletions(-) diff --git a/py/config.py b/py/config.py index 4c029f74..56dadef8 100644 --- a/py/config.py +++ b/py/config.py @@ -1169,6 +1169,46 @@ class Config: if sub_type in allowed ] + @staticmethod + def _collapse_legacy_folder_keys(keys: List[str]) -> List[str]: + """Drop folder keys the host already normalizes onto another queried key. + + 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 therefore reports + every text-encoder folder twice and trips the overlap guard with a + conflict the user cannot fix. + + When the host exposes ``map_legacy`` the alias is provably redundant and + is skipped (an empty canonical list implies an empty alias list). + Without it - the standalone mock, whose keys are independent + ``settings.json`` entries - every key is kept, because a ``clip``-only + configuration is then genuinely distinct. + """ + map_legacy = getattr(folder_paths, "map_legacy", None) + if not callable(map_legacy): + return list(keys) + + queried = set(keys) + collapsed: List[str] = [] + for key in keys: + try: + canonical = map_legacy(key) + except Exception: + canonical = key + if canonical != key and canonical in queried: + logger.debug( + "Skipping legacy folder key '%s'; the host resolves it to " + "'%s', which is queried as well.", + key, + canonical, + ) + continue + collapsed.append(key) + return collapsed + def _prepare_other_paths( self, folder_path_map: Mapping[str, Iterable[str]] ) -> Tuple[List[str], Dict[str, str], Dict[str, List[str]]]: @@ -1182,7 +1222,8 @@ class Config: unique_paths: List[str] = [] sub_type_map: Dict[str, str] = {} per_key_roots: Dict[str, List[str]] = {} - seen_real_paths: Dict[str, str] = {} # real path -> business path + # real path -> (business path, sub_type) of the category that claimed it + seen_real_paths: Dict[str, Tuple[str, str]] = {} # Cross-scanner overlap detection: warn when an "other" root is # already covered by the checkpoints/unet or embeddings scanners. @@ -1206,16 +1247,31 @@ class Config: for real_path, business_path in sorted( path_map.items(), key=lambda item: item[1].lower() ): - if real_path in seen_real_paths: - logger.warning( - "Detected the same folder '%s' under multiple other-model " - "categories ('%s' is already mapped). Keeping the first " - "category; please fix your path configuration.", - business_path, - seen_real_paths[real_path], - ) + seen = seen_real_paths.get(real_path) + if seen is not None: + seen_business_path, seen_sub_type = seen + if seen_sub_type == sub_type: + # Same category reached through a second folder_paths + # key (legacy alias, or a sub_type spanning two keys). + # Expected, so never a "fix your configuration" warning. + logger.debug( + "Ignoring duplicate folder '%s' for category '%s' " + "(already covered by '%s').", + business_path, + sub_type, + seen_business_path, + ) + else: + logger.warning( + "Detected the same folder '%s' under multiple other-model " + "categories ('%s' is already mapped as '%s'). Keeping the " + "first category; please fix your path configuration.", + business_path, + seen_business_path, + seen_sub_type, + ) continue - seen_real_paths[real_path] = business_path + seen_real_paths[real_path] = (business_path, sub_type) unique_paths.append(business_path) key_roots.append(business_path) sub_type_map[business_path] = sub_type @@ -1394,10 +1450,15 @@ class Config: Iterates the enabled OTHER_MODEL_FOLDER_SUBTYPES keys and pulls each from ``folder_paths.get_folder_paths(key)`` (in standalone mode the mock serves arbitrary keys from ``settings.json.folder_paths``). + Legacy aliases the host normalizes onto a canonical key (``clip`` -> + ``text_encoders``) are collapsed first so the same folders are not + reported twice. """ try: folder_path_map: Dict[str, List[str]] = {} - for key in self._get_enabled_other_folder_keys(): + for key in self._collapse_legacy_folder_keys( + self._get_enabled_other_folder_keys() + ): try: folder_path_map[key] = folder_paths.get_folder_paths(key) except Exception as exc: diff --git a/tests/config/test_other_paths.py b/tests/config/test_other_paths.py index e2f20baf..c993f42f 100644 --- a/tests/config/test_other_paths.py +++ b/tests/config/test_other_paths.py @@ -116,6 +116,50 @@ class TestPrepareOtherPaths: ] assert len(warnings) == 1 + def test_same_sub_type_duplicate_is_debug_not_warning(self, tmp_path, caplog): + """A sub_type spanning two folder keys legitimately sees a folder twice. + + ``clip`` and ``text_encoders`` both map to ``text_encoder``, so a folder + reachable through both is expected and must not tell the user to fix a + configuration they cannot fix. + """ + text_encoders_dir = tmp_path / "text_encoders" + legacy_clip_dir = tmp_path / "clip" + text_encoders_dir.mkdir() + legacy_clip_dir.mkdir() + + config = _make_config() + with caplog.at_level(logging.DEBUG, logger=config_module.logger.name): + unique, sub_type_map, per_key = config._prepare_other_paths( + { + "text_encoders": [str(text_encoders_dir)], + "clip": [str(legacy_clip_dir), str(text_encoders_dir)], + } + ) + + assert set(unique) == { + _normalize(str(text_encoders_dir)), + _normalize(str(legacy_clip_dir)), + } + assert sub_type_map[_normalize(str(legacy_clip_dir))] == "text_encoder" + assert per_key["clip"] == [_normalize(str(legacy_clip_dir))] + + warnings = [ + record.message + for record in caplog.records + if record.levelname == "WARNING" + and "multiple other-model categories" in record.message + ] + assert warnings == [] + + debug_messages = [ + record.message + for record in caplog.records + if record.levelname == "DEBUG" + and "Ignoring duplicate folder" in record.message + ] + assert len(debug_messages) == 1 + def test_cross_scanner_overlap_warns_but_keeps_path(self, tmp_path, caplog): """An other root overlapping a checkpoint root warns but stays managed.""" shared = tmp_path / "shared_models" @@ -175,7 +219,7 @@ class TestInitOtherPaths: config_module.folder_paths, "get_folder_paths", get_folder_paths ) - def test_default_enabled_keys_exclude_controlnet(self, monkeypatch, tmp_path): + def test_default_enabled_keys_exclude_opt_in_types(self, monkeypatch, tmp_path): dirs = {} for key in ( "vae", @@ -194,25 +238,79 @@ class TestInitOtherPaths: config = _make_config() roots = config._init_other_paths() - assert _normalize(dirs["controlnet"]) not in roots - assert _normalize(dirs["controlnet"]) not in config.other_root_subtypes - for key in ("vae", "upscale_models", "text_encoders", "clip", "clip_vision"): + # clip_vision and controlnet are workflow-driven categories and stay + # opt-in; only VAE / upscaler / text encoder are managed by default. + for key in ("clip_vision", "controlnet"): + assert _normalize(dirs[key]) not in roots + assert _normalize(dirs[key]) not in config.other_root_subtypes + # This stub has no map_legacy (standalone-shaped), so the legacy clip + # key is queried on its own and its folder lands under text_encoder. + for key in ("vae", "upscale_models", "text_encoders", "clip"): assert _normalize(dirs[key]) in roots - def test_controlnet_opt_in_via_setting(self, monkeypatch, tmp_path): - controlnet_dir = tmp_path / "controlnet" - controlnet_dir.mkdir() + def test_legacy_key_is_not_queried_when_host_aliases_it( + self, monkeypatch, tmp_path, caplog + ): + """ComfyUI resolves clip -> text_encoders, so only the canonical key is + queried: its folder list already contains the legacy directory.""" + canonical_dir = tmp_path / "text_encoders" + legacy_dir = tmp_path / "clip" + canonical_dir.mkdir() + legacy_dir.mkdir() - self._stub_folder_paths(monkeypatch, {"controlnet": str(controlnet_dir)}) - get_settings_manager().set("enabled_other_sub_types", ["controlnet"]) + queried = [] + + def get_folder_paths(key): + queried.append(key) + if key == "text_encoders": + # Mirrors ComfyUI folder_paths: both directories are registered + # under the canonical key. + return [str(canonical_dir), str(legacy_dir)] + return [] + + monkeypatch.setattr( + config_module.folder_paths, "get_folder_paths", get_folder_paths + ) + monkeypatch.setattr( + config_module.folder_paths, + "map_legacy", + lambda key: {"clip": "text_encoders"}.get(key, key), + raising=False, + ) + + config = _make_config() + with caplog.at_level(logging.DEBUG, logger=config_module.logger.name): + roots = config._init_other_paths() + + assert "clip" not in queried + assert _normalize(str(canonical_dir)) in roots + assert _normalize(str(legacy_dir)) in roots + assert ( + config.other_root_subtypes[_normalize(str(legacy_dir))] + == "text_encoder" + ) + # The reported bug: this layout used to log "please fix your path + # configuration" twice for aliased keys the user cannot separate. + assert [ + record.message + for record in caplog.records + if record.levelname == "WARNING" + ] == [] + + @pytest.mark.parametrize("opt_in_key", ["controlnet", "clip_vision"]) + def test_opt_in_sub_type_via_setting(self, monkeypatch, tmp_path, opt_in_key): + opt_in_dir = tmp_path / opt_in_key + opt_in_dir.mkdir() + + self._stub_folder_paths(monkeypatch, {opt_in_key: str(opt_in_dir)}) + get_settings_manager().set("enabled_other_sub_types", [opt_in_key]) config = _make_config() roots = config._init_other_paths() - assert _normalize(str(controlnet_dir)) in roots + assert _normalize(str(opt_in_dir)) in roots assert ( - config.other_root_subtypes[_normalize(str(controlnet_dir))] - == "controlnet" + config.other_root_subtypes[_normalize(str(opt_in_dir))] == opt_in_key ) def test_disabled_sub_type_is_not_scanned(self, monkeypatch, tmp_path):