Combine node inputs and outputs generation into a single node_signature function - #498
Combine node inputs and outputs generation into a single node_signature function#498loichuder wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| task_identifier: str | ||
| import_error: Optional[Exception] | ||
| inputs: List[NodeInput] | ||
| outputs: List[str] |
There was a problem hiding this comment.
Do we need something else than the output name?
There was a problem hiding this comment.
It can have everything and input has, even a value in the case of a pydantic model (default value).
Whether we need it, not sure, but when talking about a signature API then I would say yes.
There was a problem hiding this comment.
In which case the name NodeInput doesn't cut it anymore.
Since we already renamed the original to GraphInput we can give it any name.
There was a problem hiding this comment.
I would have to add a parsing simliar to inputs but for outputs in this case. Want me to do it in this PR or another one?
|
|
||
|
|
||
| def node_inputs(node_id: NodeIdType, node_attrs: Dict[str, Any]) -> NodeInputs: | ||
| def node_signature(node_id: NodeIdType, node_attrs: Dict[str, Any]) -> NodeSignature: |
There was a problem hiding this comment.
Can stay for this PR but in a follow up I would move this to another module. inputs.py does not cover it anymore.
b18360e to
d99d544
Compare
2 participants
PR summary
Addresses #493 (comment)
While it allows to merge the inputs/outputs generation, it does not seem to help much for checking connections.
node_signatureworks on a node-per-node basis while checking for connections need full graph knowledge.Perhaps I missed something in your comment.
AI Disclosure