Add input and output handles to components #49
Conversation
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
ohltyler
requested review from
dbwiddis,
owaiskazi19,
joshpalis,
amitgalitz and
jackiehanyang
as code owners
| { | ||
| id: 'text-embedding-processor', | ||
| label: 'Text embedding processor', | ||
| baseClass: COMPONENT_CLASS.TEXT_EMBEDDING_PROCESSOR, |
There was a problem hiding this comment.
Again, aware a lot of the questions I am asking might be not relevant as lots hasn't been decided. Here do we have the processor as an input for the index? I thought we wont be doing that as the processor is an input to the ingest pipeline creation.
There was a problem hiding this comment.
Yes it's undecided, refer to above comments.
| // Validates that connections can only be made when the source and target classes align, and | ||
| // that multiple connections to the same target handle are not allowed unless the input configuration | ||
| // for that particular component allows for it. | ||
| export function isValidConnection( |
There was a problem hiding this comment.
quick question if we do here the validation if the source class aligns with target class, what is left for the onConnect method where we do validation as well?
There was a problem hiding this comment.
onConnect will handle any other updates we need to do after a connection is made, such as updating properties of the edge itself, or update properties of the node being connected potentially. isValidConnection is just for gatekeeping that a connection is allowed to be made or not.
| const targetNodeId = connection.target; | ||
| const inputClass = sourceHandle || ''; | ||
| // We store the output classes in a pipe-delimited string. Converting back to a list. | ||
| const outputClasses = targetHandle?.split('|') || []; |
There was a problem hiding this comment.
Did you test here if we have a list of 3 output classes, and a list of 3 target classes, just one of them matching is enough?
There was a problem hiding this comment.
Yes. Also, I was splitting the target handle instead of the source handle. Updated in latest commit.
There was a problem hiding this comment.
I got rid of input/output language since that added some confusion.
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
* Add input & output handlers and some validation Signed-off-by: Tyler Ohlsen <[email protected]> * Clean up IDs & base classes Signed-off-by: Tyler Ohlsen <[email protected]> * Clean up isValidConnection Signed-off-by: Tyler Ohlsen <[email protected]> * Enforce baseclass in input Signed-off-by: Tyler Ohlsen <[email protected]> --------- Signed-off-by: Tyler Ohlsen <[email protected]> (cherry picked from commit a9113e8)
Signed-off-by: Tyler Ohlsen <[email protected]> (cherry picked from commit a9113e8) Co-authored-by: Tyler Ohlsen <[email protected]>
Description
This PR adds the input and output handles to the general
WorkspaceComponent. Specifically, we add anInputHandleandOutputHandleclass which adds theHandlereactflow component dynamically based on the component spec. These classes configure the data within the input and output handles so they can be processed correctly.Also adds a custom
isValidConnectionhelper fn to determine if a connection from a source to target is allowed. Currently, we do basic validation that 1/ the base classes are the same and 2/ prevent multiple connections if the input handle does not allow it. Short video below demonstrates this.Current logic assumes the output handle may be multiple classes (string[]). These are serialized/deserialized by turning them into a pipe-delimited string. The input handle is under the assumption to just have one class for now.
Additional clean up
idfield from the component classes. The uniqueidfor a node will just use the existing reactflow node that already has anid. When a user adds a component to the workspace (onDrop()), a random ID will be generated using the newgenerateIdhelper fn.stringto aCOMPONENT_CLASSenumTesting
isValidConnectionfn works as expected for the different use casesscreen-capture.12.webm
Issues Resolved
Makes progress on #8, #10, and #16
Check List
--signoffBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.