Conversation
Talmaj
requested review from
Kosinkadink,
alexisrolland,
comfyanonymous,
guill and
rattus128
as code owners
📝 WalkthroughWalkthroughA unit test was added to the model detection test suite to validate model uniqueness. The test introduces a helper function that converts nested configuration structures into hashable forms, enabling comparison of models based on their 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests-unit/comfy_test/model_detection_test.py`:
- 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68c5791b-fa9a-4675-ab15-68a39c994e6a
📒 Files selected for processing (1)
tests-unit/comfy_test/model_detection_test.py
| models = comfy.supported_models.models | ||
| groups = defaultdict(list) | ||
| for model in models: | ||
| key = (_freeze(model.unet_config), _freeze(model.required_keys)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
alexisrolland
changed the title
2 participants
This is a safeguard that we don't by mistake define the same identifiers for different models.
To test that it works, I've manually duplicated unet_config and required_keys for two models, to which we get a nice output which models have problematic identifiers.