Conversation
added 6 commits
June 27, 2026 10:18
Talmaj
requested review from
Kosinkadink,
alexisrolland,
comfyanonymous,
guill,
kijai and
rattus128
as code owners
📝 WalkthroughWalkthroughAdds a new DynamicGroup IO type with list-based reconstruction support, introduces two LoRA stack extra nodes that use that grouped input shape, registers the extra node module at startup, and adds unit tests covering DynamicGroup construction and nested output behavior. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
🧹 Nitpick comments (1)
comfy_extras/nodes_lora_stack.py (1)
25-36: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the LoRA file cache.
_LORA_CACHEstores loaded LoRA tensors indefinitely, so long-running sessions that try many LoRAs can retain large CPU tensors forever. Consider an LRU/size cap while preserving reuse.♻️ Possible bounded-cache shape
+from functools import lru_cache from typing_extensions import override @@ -# Module-level cache so repeated executions don't re-read the same file from disk. -_LORA_CACHE: dict[str, tuple] = {} +_LORA_CACHE_MAXSIZE = 16 @@ -def _load_lora_file(lora_name: str): - lora_path = folder_paths.get_full_path_or_raise("loras", lora_name) - cached = _LORA_CACHE.get(lora_path) - if cached is not None: - return cached - lora, metadata = comfy.utils.load_torch_file(lora_path, safe_load=True, return_metadata=True) - _LORA_CACHE[lora_path] = (lora, metadata) - return lora, metadata +@lru_cache(maxsize=_LORA_CACHE_MAXSIZE) +def _load_lora_path(lora_path: str): + return comfy.utils.load_torch_file(lora_path, safe_load=True, return_metadata=True) + + +def _load_lora_file(lora_name: str): + return _load_lora_path(folder_paths.get_full_path_or_raise("loras", lora_name))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_extras/nodes_lora_stack.py` around lines 25 - 36, The module-level LoRA cache in _LORA_CACHE is unbounded, so repeated _load_lora_file calls can retain large tensors indefinitely. Update the caching in _load_lora_file to use a bounded strategy (for example an LRU or fixed-size cap) while keeping the existing reuse behavior for recently accessed entries. Keep the logic centered around _LORA_CACHE and _load_lora_file so the cache still keys by lora_path but evicts older items when the limit is reached.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_extras/nodes_lora_stack.py`:
- Around line 75-80: Normalize the LoRA row strength before calling
load_lora_for_models in the LoRA stack reconstruction path, since DynamicGroup
rows can contain strength=None even when lora_name is present. In the logic
around lora_name/strength in nodes_lora_stack.py, make sure omitted or None
strength values fall back to the default numeric strength (or are skipped
consistently) before _load_lora_file and comfy.sd.load_lora_for_models are
invoked; apply the same change in the duplicate reconstruction path referenced
by the second occurrence.
In `@tests-unit/comfy_api_test/io_dynamic_group_test.py`:
- Around line 6-10: The torch test stub in the test setup is too eager because
it only checks sys.modules, so it can hide a real installed torch that has not
been imported yet. Update the torch-stubbing logic near the top-level test setup
in io_dynamic_group_test.py to first detect whether torch is actually importable
before creating the ModuleType stub, and only inject the stub when the real
module is unavailable. Keep the behavior tied to the existing sys.modules
insertion and the _torch_stub setup so later tests do not get poisoned.
---
Nitpick comments:
In `@comfy_extras/nodes_lora_stack.py`:
- Around line 25-36: The module-level LoRA cache in _LORA_CACHE is unbounded, so
repeated _load_lora_file calls can retain large tensors indefinitely. Update the
caching in _load_lora_file to use a bounded strategy (for example an LRU or
fixed-size cap) while keeping the existing reuse behavior for recently accessed
entries. Keep the logic centered around _LORA_CACHE and _load_lora_file so the
cache still keys by lora_path but evicts older items when the limit is reached.
🪄 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 Plus
Run ID: 5062c37e-6698-439c-8c42-746ddc51ebfd
📒 Files selected for processing (4)
comfy_api/latest/_io.pycomfy_extras/nodes_lora_stack.pynodes.pytests-unit/comfy_api_test/io_dynamic_group_test.py
Comment on lines
+75
to
+80
| lora_name = row.get("lora_name") | ||
| strength = row.get("strength", 1.0) | ||
| if not lora_name or lora_name == "none" or strength == 0: | ||
| continue | ||
| lora, metadata = _load_lora_file(lora_name) | ||
| model, _ = comfy.sd.load_lora_for_models(model, None, lora, strength, 0, lora_metadata=metadata) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Normalize None strength values before applying LoRAs.
DynamicGroup reconstruction can emit a row field with value None; when lora_name is present but strength is omitted, this passes None into load_lora_for_models.
🐛 Proposed fix
- strength = row.get("strength", 1.0)
- if not lora_name or lora_name == "none" or strength == 0:
+ strength = row.get("strength")
+ if not lora_name or lora_name == "none":
+ continue
+ if strength is None:
+ strength = 1.0
+ if strength == 0:
continue
@@
- strength = row.get("strength", 1.0)
- if not lora_name or lora_name == "none" or strength == 0:
+ strength = row.get("strength")
+ if not lora_name or lora_name == "none":
+ continue
+ if strength is None:
+ strength = 1.0
+ if strength == 0:
continueAlso applies to: 111-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@comfy_extras/nodes_lora_stack.py` around lines 75 - 80, Normalize the LoRA
row strength before calling load_lora_for_models in the LoRA stack
reconstruction path, since DynamicGroup rows can contain strength=None even when
lora_name is present. In the logic around lora_name/strength in
nodes_lora_stack.py, make sure omitted or None strength values fall back to the
default numeric strength (or are skipped consistently) before _load_lora_file
and comfy.sd.load_lora_for_models are invoked; apply the same change in the
duplicate reconstruction path referenced by the second occurrence.
Comment on lines
+6
to
+10
| # Stub torch (type-hint only in _io.py; real torch not available in unit-test env) | ||
| if "torch" not in sys.modules: | ||
| _torch_stub = types.ModuleType("torch") | ||
| _torch_stub.Tensor = object # type: ignore[attr-defined] | ||
| sys.modules["torch"] = _torch_stub |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid shadowing a real installed torch module.
Checking only sys.modules means this test stubs torch even when real torch is installed but not imported yet, which can poison later tests in the same process.
🧪 Proposed fix
-# Stub torch (type-hint only in _io.py; real torch not available in unit-test env)
-if "torch" not in sys.modules:
+# Stub torch only when it is genuinely unavailable.
+try:
+ import torch # noqa: F401
+except ImportError:
_torch_stub = types.ModuleType("torch")
_torch_stub.Tensor = object # type: ignore[attr-defined]
sys.modules["torch"] = _torch_stub📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Stub torch (type-hint only in _io.py; real torch not available in unit-test env) | |
| if "torch" not in sys.modules: | |
| _torch_stub = types.ModuleType("torch") | |
| _torch_stub.Tensor = object # type: ignore[attr-defined] | |
| sys.modules["torch"] = _torch_stub | |
| # Stub torch only when it is genuinely unavailable. | |
| try: | |
| import torch # noqa: F401 | |
| except ImportError: | |
| _torch_stub = types.ModuleType("torch") | |
| _torch_stub.Tensor = object # type: ignore[attr-defined] | |
| sys.modules["torch"] = _torch_stub |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests-unit/comfy_api_test/io_dynamic_group_test.py` around lines 6 - 10, The
torch test stub in the test setup is too eager because it only checks
sys.modules, so it can hide a real installed torch that has not been imported
yet. Update the torch-stubbing logic near the top-level test setup in
io_dynamic_group_test.py to first detect whether torch is actually importable
before creating the ModuleType stub, and only inject the stub when the real
module is unavailable. Keep the behavior tied to the existing sys.modules
insertion and the _torch_stub setup so later tests do not get poisoned.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Key Points
LoadLoraModel— takes aMODEL, applies each row's LoRA to the model only(
load_lora_for_models(model, None, lora, strength, 0)), outputsMODEL.LoadLoraTextEncoder— takes aCLIP, applies each row to the text encoder only(
load_lora_for_models(None, clip, lora, 0, strength)), outputsCLIP._lora_template()populates thelora_namecombo fromfolder_paths.get_filename_list("loras")(real files, unlike the test node)._LORA_CACHEavoids re-reading the same file from disk across executions,preserving the perf behavior of the old per-instance
self.loaded_loracache(V3 nodes are stateless classmethods).
"none", orstrength == 0are skipped as a no-op.node_id,display_name,search_aliases,category,description) from the project's node rules.