Validate that a node is executable before running the prompt#14649
Validate that a node is executable before running the prompt#14649wei-hai wants to merge 2 commits into
Conversation
A node whose FUNCTION points at a method that does not exist (e.g. a typo in a custom node), or a V3 node missing its execute override, was only detected once that node ran -- after every upstream node had already executed. In a multi-node workflow the user waited for the whole graph to run up to the broken node before seeing the error. validate_prompt already walks every node before execution; add an executability check there so the error is reported up front and attributed to the offending node (returned in node_errors), and nothing runs. The check resolves the V1 FUNCTION method on the class (the common case) and falls back to an instance, since the runtime invokes it on an instance and a node may define FUNCTION or its method in __init__. V3 nodes are checked via their existing VALIDATE_CLASS. Add tests for V1 typo, V3 typo, good nodes, and a node whose method is defined in __init__ (must not be falsely rejected).
wei-hai
requested review from
Kosinkadink,
alexisrolland,
comfyanonymous,
guill,
kijai and
rattus128
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
🚥 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://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests-unit/execution_test/validate_node_executable_test.py (1)
88-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore
NODE_CLASS_MAPPINGSentries after each test.These tests permanently mutate the global node registry, so later tests run with extra classes registered. Please wrap registration in a fixture or
try/finallythat restores the previous value.Possible fix
def _register(class_type, class_def): - nodes.NODE_CLASS_MAPPINGS[class_type] = class_def + old = nodes.NODE_CLASS_MAPPINGS.get(class_type) + nodes.NODE_CLASS_MAPPINGS[class_type] = class_def + return old + + +def _restore(class_type, old): + if old is None: + nodes.NODE_CLASS_MAPPINGS.pop(class_type, None) + else: + nodes.NODE_CLASS_MAPPINGS[class_type] = olddef test_good_node_passes(): - _register("GoodV1Node", _GoodV1Node) - assert node_not_executable_reason(_GoodV1Node, "GoodV1Node") is None - valid, _, _, _ = _validate("GoodV1Node") - assert valid is True + old = _register("GoodV1Node", _GoodV1Node) + try: + assert node_not_executable_reason(_GoodV1Node, "GoodV1Node") is None + valid, _, _, _ = _validate("GoodV1Node") + assert valid is True + finally: + _restore("GoodV1Node", old)Also applies to: 97-135
🤖 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/execution_test/validate_node_executable_test.py` around lines 88 - 90, The helper `_register` in the validate_node_executable tests mutates the global `nodes.NODE_CLASS_MAPPINGS` and leaves added entries behind across tests. Update the registration flow used by these test cases to save the original mapping state before adding classes and restore it afterward, ideally via a fixture or a `try/finally` around the test setup that calls `_register` so each test starts with the same registry.Source: Path instructions
🤖 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 `@execution.py`:
- Around line 1138-1145: The validation in execution.py is eagerly constructing
the class via class_def(), which triggers __init__ and can misclassify valid
nodes as invalid. Update the logic around _v1_function_resolves and the FUNCTION
lookup to only inspect the class object itself without instantiating it, and
keep the existing class_def/class_type error reporting path intact.
---
Nitpick comments:
In `@tests-unit/execution_test/validate_node_executable_test.py`:
- Around line 88-90: The helper `_register` in the validate_node_executable
tests mutates the global `nodes.NODE_CLASS_MAPPINGS` and leaves added entries
behind across tests. Update the registration flow used by these test cases to
save the original mapping state before adding classes and restore it afterward,
ideally via a fixture or a `try/finally` around the test setup that calls
`_register` so each test starts with the same registry.
🪄 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: 4037bfa0-7af9-455d-9bd7-2ca5f9b7d48f
📒 Files selected for processing (2)
execution.pytests-unit/execution_test/validate_node_executable_test.py
Addresses review feedback: the V1 executability check fell back to constructing the node (class_def()) when the FUNCTION method wasn't found on the class. That runs __init__ during validation, so a constructor's side effects or failure could be misreported as invalid_node_definition for an otherwise valid node. Inspect only the class. No core/extra node defines its FUNCTION method on the instance, so this loses no real coverage while removing the side-effect risk. Replace the instance-fallback test with one asserting a node with a raising __init__ but a valid class-level method still passes validation (i.e. it is never instantiated).
| # point resolves to a real method) before we touch any schema-derived | ||
| # attributes below or start execution. Catches code typos up front and | ||
| # attributes the error to the offending node. | ||
| not_executable = node_not_executable_reason(class_, class_type) |
There was a problem hiding this comment.
Since this is validating the class anyway, what do you think of just doing this validation when we register a node instead of doing it on every execution?
This isn't a user error -- it's always a custom node author error, so making the error appear to the author during startup seems a lot more valuable than only surfacing it to the UI once the node is actually used.
There was a problem hiding this comment.
I am not sure how we make it appear to the author, is there a place to store the author's contact when registering the custom node?
There was a problem hiding this comment.
I just mean showing it in the console on startup since the custom node author is virtually guaranteed to start up ComfyUI after they make changes (even if they don't execute every one of their own nodes).
This isn't the sort of issue that might occur on users' machines when it worked perfectly fine on the author's -- it's a static error in typing structure.
There was a problem hiding this comment.
the error does show in the console, this PR makes sure the error is also rendered on frontend in case the console logs are noisy.
3 participants
Problem
A node whose
FUNCTIONpoints at a method that doesn't exist (e.g. a typo in a custom node), or a V3 node missing itsexecuteoverride, is only detected when that node runs — after every upstream node has already executed. In a multi-node workflow the user waits for the whole graph to run up to the broken node before seeing the error, even though the problem is statically knowable before anything runs.Fix
validate_promptalready walks every node before execution (checkingclass_typeis present and the node class is installed). This adds an executability check in that same loop: if a node's declared entry point doesn't resolve to a callable, the prompt is rejected up front with the error attributed to the offending node (returned innode_errors), and nothing runs.FUNCTIONmethod on the class (the common case), falling back to an instance — the runtime invokes the method on an instance (getattr(obj, obj.FUNCTION)), and a node may legitimately defineFUNCTIONor its method in__init__. The instance fallback only runs when the class-level check fails, so normal nodes are never instantiated during validation.VALIDATE_CLASS(execute/define_schema overrides), which is pure MRO introspection — cheap and side-effect free.The check is placed before the loop's
OUTPUT_NODEaccess, which for a structurally broken V3 node would otherwise raise uncaught — so this also turns a pre-existing latent failure into a clean, node-attributed error.Tests
Adds
tests-unit/execution_test/validate_node_executable_test.py: V1 typo and V3 typo are rejected with a per-node error; good V1/V3 nodes pass; and a node that defines its method in__init__is not falsely rejected (guards the instance-fallback path).Note
This is the validation-time counterpart to #14648, which makes runtime/scheduling errors surface to the client instead of silently killing the worker thread. The two are independent (different files) and complementary: this rejects statically-detectable breakage up front; #14648 is the catch-all for errors only knowable at runtime.