Skip to content

Add IfElseNode and Merge Node#40

Merged
JeanKaddour merged 19 commits intomainfrom
feat/logicNodes
Dec 8, 2024
Merged

Add IfElseNode and Merge Node#40
JeanKaddour merged 19 commits intomainfrom
feat/logicNodes

Conversation

@JeanKaddour
Copy link
Copy Markdown
Contributor

@JeanKaddour JeanKaddour commented Dec 8, 2024

This pull request includes significant updates to the backend and frontend components, focusing on the implementation of IfElse and merge nodes in the workflow execution system. These changes enhance the system's ability to handle complex workflows with conditional branching and merging.

Backend Changes:

  • Conditional Nodes Implementation:

    • Added ConditionalNode class to handle routing based on evaluated conditions (backend/app/nodes/logic/conditional.py).
    • Introduced Condition, BranchCondition, and ConditionalNodeConfig models to define and manage conditions and branches.
  • Merge Nodes Implementation:

    • Added MergeNode class to merge outputs from conditional branches (backend/app/nodes/logic/merge.py).
    • Introduced MergeNodeConfig, MergeNodeInput, and MergeNodeOutput models to configure and handle merge operations.
  • Node Executor Enhancements:

    • Updated NodeExecutor to track active branches and handle conditional nodes (backend/app/execution/node_executor.py). [1] [2] [3]
    • Added methods to filter active branch links and determine the active branch during node execution.
  • Workflow Executor Enhancements:

    • Modified WorkflowExecutor to support conditional branching and merging (backend/app/execution/workflow_executor.py). [1] [2] [3] [4]
    • Implemented methods to get active links and dependent nodes based on conditional branches.
  • API Changes:

    • Updated API methods to use EvalRunStatusEnum for status fields (backend/app/api/evals_management.py). [1] [2] [3]

Frontend Changes:

  • Node Types Integration:

    • Added imports for ConditionalNode and MergeNode in FlowCanvas.tsx (frontend/src/components/canvas/FlowCanvas.tsx).
    • Updated useNodeTypes to include ConditionalNode and MergeNode in the node types configuration (frontend/src/components/canvas/FlowCanvas.tsx).
  • Dependencies:

    • Updated package-lock.json to include new dependencies for different platforms (frontend/package-lock.json).

Important

Add IfElse and Merge nodes to enhance workflow execution with conditional branching and merging capabilities, updating both backend and frontend components.

  • Backend Changes:
    • Add ConditionalNode and MergeNode classes in if_else.py and merge.py for handling conditional branching and merging.
    • Update NodeExecutor and WorkflowExecutor in node_executor.py and workflow_executor.py to support new node types and manage active branches.
    • Modify API in evals_management.py to use EvalRunStatusEnum for status fields.
  • Frontend Changes:
    • Integrate IfElseNode and MergeNode in FlowCanvas.tsx and RunViewFlowCanvas.jsx.
    • Update DynamicNode.tsx, OutputDisplayNode.jsx, and IfElseNode.tsx to support new node types.
    • Add IfElseEditor.tsx and MergeEditor.tsx for node configuration in the sidebar.
    • Update styles in DynamicNode.module.css for new nodes.
    • Modify EvalCard.tsx and evals.tsx to handle eval runs with new nodes.

This description was created by Ellipsis for 79508f5. It will automatically update as commits are pushed.

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 2f8afae in 1 minute and 42 seconds

More details
  • Looked at 1977 lines of code in 22 files
  • Skipped 1 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/components/nodes/logic/IfElseNode.tsx:89
  • Draft comment:
    Consider adding handleUpdateBranches to the dependency array of the useEffect on line 72 to avoid potential stale closures.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The 'useEffect' on line 72 is intended to run once on component mount, as indicated by the empty dependency array. Adding 'handleUpdateBranches' to the dependencies would cause the effect to re-run whenever 'handleUpdateBranches' changes, which might not be the intended behavior. Since 'handleUpdateBranches' is defined within the component and not passed as a prop, it is unlikely to change unless the component re-renders, which would already trigger the effect.
    I might be overlooking the possibility that 'handleUpdateBranches' could change due to some other reason, but given the current context, it seems unlikely.
    Given that 'handleUpdateBranches' is defined within the component and not expected to change, the current setup with an empty dependency array seems appropriate.
    The comment is not necessary because adding 'handleUpdateBranches' to the dependency array is not required for the intended behavior of the 'useEffect'.
2. frontend/src/components/nodes/logic/MergeNode.tsx:75
  • Draft comment:
    Ensure that nodeRef and data are included in the dependency array of the useEffect on line 61 to correctly recalculate the node width when these dependencies change.
  • Reason this comment was not posted:
    Comment did not seem useful.
3. frontend/src/components/nodes/nodeSidebar/NodeSidebar.tsx:473
  • Draft comment:
    Consider integrating the special handling for MergeNode within the keys.map loop in renderConfigFields to maintain consistency with other node types.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In frontend/src/components/nodes/nodeSidebar/NodeSidebar.tsx, the renderConfigFields function has a special handling for MergeNode that directly returns a MergeEditor component. However, this bypasses the keys.map logic, which might be necessary for other configurations. It might be better to integrate this logic within the keys.map loop to ensure consistency.

Workflow ID: wflow_2NgLGGetkZpXzfZX


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Comment thread frontend/src/store/nodeTypesSlice.ts Outdated
'nodeTypes/fetchNodeTypes',
async () => {
const response = await getNodeTypes();
console.log("here is the response", response);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the console log statement in fetchNodeTypes to avoid unnecessary output in production.

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 79508f5 in 27 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/store/nodeTypesSlice.ts:88
  • Draft comment:
    Remove the leftover debug statement to clean up the code.
    const response = await getNodeTypes();
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_3fHmuF39rg5W79vl


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@JeanKaddour JeanKaddour merged commit f0d82a9 into main Dec 8, 2024
@JeanKaddour JeanKaddour deleted the feat/logicNodes branch December 8, 2024 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants