Skip to content

Add test that each model has unique identifiers (CORE-134) #13654

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Talmaj wants to merge 1 commit into
base: master
Choose a base branch
from
+32 −0
Open

Add test that each model has unique identifiers (CORE-134) #13654

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions tests-unit/comfy_test/model_detection_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,23 @@
from collections import defaultdict

import torch

from comfy.model_detection import detect_unet_config, model_config_from_unet_config
import comfy.supported_models


def _freeze(value):
"""Recursively convert a value to a hashable form so configs can be
compared/used as dict keys or set members."""
if isinstance(value, dict):
return frozenset((k, _freeze(v)) for k, v in value.items())
if isinstance(value, (list, tuple)):
return tuple(_freeze(v) for v in value)
if isinstance(value, set):
return frozenset(_freeze(v) for v in value)
return value


def _make_longcat_comfyui_sd():
"""Minimal ComfyUI-format state dict for pre-converted LongCat-Image weights."""
sd = {}
Expand Down Expand Up @@ -110,3 +124,21 @@ def test_flux_schnell_comfyui_detected_as_flux_schnell(self):
model_config = model_config_from_unet_config(unet_config, sd)
assert model_config is not None
assert type(model_config).__name__ == "FluxSchnell"

def test_unet_config_and_required_keys_combination_is_unique(self):
"""Each model in the registry must have a unique combination of
``unet_config`` and ``required_keys``. If two models share the same
combination, ``BASE.matches`` cannot disambiguate between them and the
first one in the list will always win."""
models = comfy.supported_models.models
groups = defaultdict(list)
for model in models:
key = (_freeze(model.unet_config), _freeze(model.required_keys))
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Align required_keys normalization with matches() semantics (keys-only).

At Line 136, using _freeze(model.required_keys) can miss collisions when required_keys is a dict with same keys but different values. matches() only uses required-key membership, so this should normalize to key names only.

Suggested patch 
         groups = defaultdict(list)
         for model in models:
-            key = (_freeze(model.unet_config), _freeze(model.required_keys))
+            required_keys_key = (
+                frozenset(model.required_keys.keys())
+                if isinstance(model.required_keys, dict)
+                else frozenset(model.required_keys)
+            )
+            key = (_freeze(model.unet_config), required_keys_key)
             groups[key].append(model.__name__)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests-unit/comfy_test/model_detection_test.py` at line 136, The key used for
deduping models currently freezes model.required_keys including values, which
can miss collisions because matches() only considers membership; update the
normalization to freeze only the key names (e.g., use the set or list of
model.required_keys.keys()) so the key becomes (_freeze(model.unet_config),
_freeze(<only the required key names>)) and thereby align with matches()
semantics; modify the use of _freeze around model.required_keys in the code that
builds the key to use just the keys instead of the full dict.

Copy link
Copy Markdown
Contributor Author

@Talmaj Talmaj May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, the current code checks key and values for required_keys, whereas the def matches, really checks only the keys. I think we'll rather change def matches in the future to check both.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Talmaj, that makes sense — thanks for the clarification! The test is intentionally stricter than the current matches() semantics, anticipating a future tightening of matches() to also compare values.

One thing worth noting: until matches() is updated, there's a narrow gap where two models sharing the same required-key names but different values would slip past this test undetected, yet still cause an ambiguous detection at runtime (since matches() only checks membership today). That scenario is admittedly rare in practice.

To make the intent clear to future readers, you might consider adding a short comment like:

# NOTE: _freeze includes dict values intentionally – this test anticipates that
# matches() will be updated to compare values too, not just key membership.
key = (_freeze(model.unet_config), _freeze(model.required_keys))

That way, the "why is this stricter than matches()?" question is answered in-place. Entirely up to you though!

✏️ Learnings added 
Learnt from: Talmaj
Repo: Comfy-Org/ComfyUI PR: 13654
File: tests-unit/comfy_test/model_detection_test.py:136-136
Timestamp: 2026-05-04T18:27:56.271Z
Learning: In `tests-unit/comfy_test/model_detection_test.py` (Comfy-Org/ComfyUI), the test `test_unet_config_and_required_keys_combination_is_unique` intentionally freezes both keys AND values of `model.required_keys`. This is by design: the current `matches()` in `comfy.supported_models` only checks key membership, but the intent is to update `matches()` in the future to also check values. The stricter test is forward-looking.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: pythongosssss
Repo: Comfy-Org/ComfyUI PR: 12555
File: comfy_extras/nodes_glsl.py:719-724
Timestamp: 2026-02-21T14:01:41.482Z
Learning: In PyOpenGL, bare Python scalars can be accepted for 1-element array parameters by NumberHandler. This means you can pass an int/float directly to OpenGL texture deletion (e.g., glDeleteTextures(tex)) without wrapping in a list. Verify function-specific expectations and ensure types match what the OpenGL call expects; use explicit lists only when the API requires an array.

groups[key].append(model.__name__)

duplicates = {k: names for k, names in groups.items() if len(names) > 1}
assert not duplicates, (
"Found models sharing the same (unet_config, required_keys) "
"combination, which makes detection ambiguous: "
+ "; ".join(", ".join(names) for names in duplicates.values())
)
Loading