-
Notifications
You must be signed in to change notification settings - Fork 18
Add validation guarding against parent node output references #3630
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.
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
Conversation
src/vellum/workflows/nodes/displayable/bases/inline_prompt_node/node.py
Outdated
Show resolved
Hide resolved
| class_outputs = vars(node_cls.Outputs) | ||
|
|
||
| for name, value in class_outputs.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] we support iterating over the node class: for reference, value in node_cls.Outputs:
| continue | ||
|
|
||
| parent_node_class = node_value.outputs_class.__parent_class__ | ||
| if parent_node_class in node_cls.__mro__: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it valid to reference a sibling value in the same Outputs class? e.g.
class CustomNode(BaseNode):
class Outputs:
a: str = SomeOtherNode.Outputs.str_output
b: str = a
...From some quick testing the node_value for b points to SomeOtherNode.Outputs.str_output and it is correctly marked as valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that's just standard output referencing of upstream nodes and how final output nodes work
| Validates that the node does not reference parent class outputs. | ||
| """ | ||
| errors = [] | ||
| for node_output in node_cls.Outputs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #3630 (comment)
we're iterating on a subclass of BaseOutputs and not an instance, so we need to fetch the values separately
| continue | ||
|
|
||
| parent_node_class = node_value.outputs_class.__parent_class__ | ||
| if parent_node_class in node_cls.__mro__: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that's just standard output referencing of upstream nodes and how final output nodes work
Prevent users from defining custom
Outputsclasses where an output references something from the parent classHere
custom_outputdoesn't get resolved during execution and remainsundefined. Separately, I think custom outputs for anInlinePromptNodewill just get dropped during serialization and possibly should be forbidden