Feat/merge node by preet-bhadra · Pull Request #67 · PySpur-Dev/pyspur

preet-bhadra · 2024-12-27T11:16:19Z

Now it is renamed to coalesce node, impleted the coalesce node backend and frontend

Important

Introduce CoalesceNode to replace MergeNode, updating backend and frontend components to handle multiple inputs and output the first non-null value.

Behavior:
- Introduce CoalesceNode to replace MergeNode, outputs first non-null value from multiple inputs.
- Backend logic in coalesce.py, handles multiple inputs and outputs first non-null value.
- workflow_executor.py updated to handle CoalesceNode, skips early failure for None inputs.
Models:
- CoalesceNode added to node_types.py.
Frontend:
- Replace MergeNode with CoalesceNode in FlowCanvas.tsx and CoalesceNode.tsx.
- Update NodeSidebar.tsx to handle CoalesceNode configuration.
- Adjust flowSlice.ts to manage CoalesceNode state, including schema rebuilding logic.
Misc:
- Update .env.example to reflect new repository name.
- Minor script update in test-release.sh for Docker compose command.

^{This description was created by}^{for 490c2d1. It will automatically update as commits are pushed.}

…necessary print statements

…ptions

…ssary print statements

… code

…t/merge-node

…anges

preet-bhadra · 2024-12-27T11:17:24Z

@ellipsis review this.

ellipsis-dev

👍 Looks good to me! Reviewed everything up to aa5593e in 1 minute and 34 seconds

More details

Looked at 1430 lines of code in 9 files
Skipped 0 files when reviewing.
Skipped posting 6 drafted comments based on config settings.

1. backend/app/execution/workflow_executor.py:102

Draft comment:
Replace print statements with proper logging to avoid cluttering the console and to provide more control over the logging output.
Reason this comment was not posted:
Confidence changes required: 50%
The code contains multiple print statements for debugging purposes. These should be replaced with proper logging to avoid cluttering the console and to provide more control over the logging output.

2. backend/app/execution/workflow_executor.py:103

Draft comment:
Replace print statements with proper logging to avoid cluttering the console and to provide more control over the logging output.
Reason this comment was not posted:
Confidence changes required: 50%
The code contains multiple print statements for debugging purposes. These should be replaced with proper logging to avoid cluttering the console and to provide more control over the logging output.

3. backend/app/execution/workflow_executor.py:139

Draft comment:
Replace print statements with proper logging to avoid cluttering the console and to provide more control over the logging output.
Reason this comment was not posted:
Confidence changes required: 50%
The code contains multiple print statements for debugging purposes. These should be replaced with proper logging to avoid cluttering the console and to provide more control over the logging output.

4. backend/app/nodes/logic/coalesce.py:47

Draft comment:
Replace print statements with proper logging to avoid cluttering the console and to provide more control over the logging output.
Reason this comment was not posted:
Confidence changes required: 50%
The code contains multiple print statements for debugging purposes. These should be replaced with proper logging to avoid cluttering the console and to provide more control over the logging output.

5. backend/app/nodes/logic/coalesce.py:63

Draft comment:
Replace print statements with proper logging to avoid cluttering the console and to provide more control over the logging output.
Reason this comment was not posted:
Confidence changes required: 50%
The code contains multiple print statements for debugging purposes. These should be replaced with proper logging to avoid cluttering the console and to provide more control over the logging output.

6. backend/app/nodes/logic/coalesce.py:79

Draft comment:
Replace print statements with proper logging to avoid cluttering the console and to provide more control over the logging output.
Reason this comment was not posted:
Confidence changes required: 50%
The code contains multiple print statements for debugging purposes. These should be replaced with proper logging to avoid cluttering the console and to provide more control over the logging output.

Workflow ID: wflow_EqK49RhTpLH3RCDj

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

srijanpatel

Looks good overall. Left some minor comments. Also please drop debug print statements and console logs.

srijanpatel · 2024-12-27T14:02:35Z

+                print(f"Returning first non-null value: {key}, {value}")
+                return self.output_model(**first_non_null_output)  # type: ignore
+
+        # If all values are None, return an empty output


In this case we should return a None output instead of an empty model. None will skip the children nodes.

srijanpatel · 2024-12-27T14:03:42Z

Remove this from the final commit.

srijanpatel · 2024-12-27T14:05:23Z

-        } else if (node.name === 'MergeNode') {
-          acc[node.name] = (props: any) => <MergeNode {...props} />;
+        } else if (node.name === 'CoalesceNode') {
+          acc[node.name] = CoalesceNode;


@preet-bhadra can you make sure there are no linter errors here? If there are then let's use the functional style as it was done for merge node.

srijanpatel · 2024-12-27T14:08:26Z

+
+interface CoalesceNodeProps {
+  id: string;
+  data: CoalesceNodeData;


This can be FlowWorkflowNode['data']
We should use already declared types as much as possible.
No need to declare CoalesceNodeData type separately.

srijanpatel · 2024-12-27T14:12:31Z

Nice work moving the router and coalesce node logic to respective functions

- In WorkflowExecutor, handle input nodes differently based on their type. - In CoalesceNode, return None if all input values are None.

…euse the flowworkflownode schema

…t/merge-node

ellipsis-dev

👍 Looks good to me! Reviewed everything up to 490c2d1 in 1 minute and 27 seconds

More details

Looked at 1280 lines of code in 13 files
Skipped 0 files when reviewing.
Skipped posting 5 drafted comments based on config settings.

1. backend/app/execution/workflow_executor.py:99

Draft comment:
Consider replacing print statements with a proper logging mechanism for better control over log levels and outputs.
Reason this comment was not posted:
Confidence changes required: 50%
The PR introduces a new CoalesceNode to replace the MergeNode. The backend logic and frontend components have been updated accordingly. The changes seem to be well-integrated, but there are a few areas that need attention. Specifically, the use of print statements for logging in workflow_executor.py is not ideal for production code. Additionally, the CoalesceNode logic in coalesce.py could be improved for clarity and efficiency. The frontend components seem to be updated correctly, but there are some redundant console logs that should be removed. Overall, the PR is a good step forward, but these minor issues should be addressed before merging.

2. backend/app/nodes/logic/coalesce.py:50

Draft comment:
Consider optimizing the iteration over data by combining preference checks with the general iteration to avoid redundant loops. Also, simplify the use of create_model if possible.
Reason this comment was not posted:
Confidence changes required: 50%
The CoalesceNode logic in coalesce.py is mostly correct, but the handling of preferences and the iteration over data could be optimized. The current implementation checks preferences first and then iterates over all data, which is fine, but could be more efficient if combined. Additionally, the use of create_model could be simplified.

3. frontend/src/components/canvas/FlowCanvas.tsx:192

Draft comment:
Remove redundant console.log statements to clean up the code.
Reason this comment was not posted:
Confidence changes required: 50%
The PR includes changes to the frontend components to replace MergeNode with CoalesceNode. While the changes are mostly correct, there are some redundant console logs that should be removed to clean up the code.

4. frontend/src/components/nodes/DynamicNode.tsx:174

Draft comment:
Remove redundant console.log statements to clean up the code.
Reason this comment was not posted:
Confidence changes required: 50%
The PR includes changes to the frontend components to replace MergeNode with CoalesceNode. While the changes are mostly correct, there are some redundant console logs that should be removed to clean up the code.

5. frontend/src/components/nodes/logic/RouterNode.tsx:282

Draft comment:
Remove redundant console.log statements to clean up the code.
Reason this comment was not posted:
Confidence changes required: 50%
The PR includes changes to the frontend components to replace MergeNode with CoalesceNode. While the changes are mostly correct, there are some redundant console logs that should be removed to clean up the code.

Workflow ID: wflow_QnBXqUAa31uWrXaq

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

srijanpatel · 2024-12-27T16:11:10Z

LGTM!

preet-bhadra added 21 commits December 27, 2024 02:59

Add CoalesceNode to logic node types

ec5f88c

Refactor WorkflowExecutor to handle CoalesceNode inputs and remove un…

1bf83a7

…necessary print statements

Refactor workflow_management.py to remove unused import statement

b827f3c

Remove unused JSON node types

b2e9dec

Delete sample_llm.py to remove unused code and update configuration o…

b58831f

…ptions

Add example_merge.json with new workflow configuration

60233eb

Refactor CoalesceNode to handle CoalesceNode inputs and remove unnece…

8bfc21d

…ssary print statements

Refactor FlowCanvas to replace MergeNode with CoalesceNode

afcceea

Refactor MergeNode component to replace it with CoalesceNode

703fb50

Refactor CoalesceNode to handle CoalesceNode inputs and remove unnece…

3749437

…ssary print statements

Refactor CoalesceNode to improve handle layout and remove unnecessary…

d6a3a6a

… code

Merge branch 'main' of https://github.com/sevn-ai/pyspur_dev into fea…

02f65f4

…t/merge-node

Refactor CoalesceNode.tsx: Add type guard for FlowWorkflowNode

c7aebbf

Refactor NodeSidebar.tsx: Remove unused code for conditional routes

44d53ca

Refactor store.ts: Add metadata field to nodeTypes

e27f53b

Merge branch 'main' of https://github.com/sevn-ai/pyspur_dev into fea…

ac48361

…t/merge-node

Refactor flowSlice.ts: Update input node output schema on variable ch…

5eb91a2

…anges

Refactor flowSlice.ts: Update input node output schema on variable ch…

5758b88

…anges

Refactor flowSlice.ts: Update input node output schema on variable ch…

5e41601

…anges

Refactor flowSlice.ts: updateNodeData used helper functions now

70f1537

Refactor flowSlice.ts: Remove unnecessary comments

aa5593e

ellipsis-dev Bot reviewed Dec 27, 2024

View reviewed changes

srijanpatel requested changes Dec 27, 2024

View reviewed changes

preet-bhadra added 6 commits December 27, 2024 20:20

Refactor: Remove debug print and console.log statements.

db62c1b

Refactor: Update input handling in WorkflowExecutor and CoalesceNode

68f4abb

- In WorkflowExecutor, handle input nodes differently based on their type. - In CoalesceNode, return None if all input values are None.

Refactor: Remove example_merge.json

93888f1

Refactor: Update CoalesceNodeProps interface in CoalesceNode.tsx to r…

a1d44d9

…euse the flowworkflownode schema

Refactor: Update import statement for CoalesceNode in FlowCanvas.tsx

0b73f28

Merge branch 'main' of https://github.com/sevn-ai/pyspur_dev into fea…

9a51ea1

…t/merge-node

preet-bhadra added 2 commits December 27, 2024 21:34

Refactor: Update GitHub repository name in .env.example

7eaad85

Refactor: Update docker-compose command in test-release.sh

490c2d1

preet-bhadra marked this pull request as ready for review December 27, 2024 16:08

ellipsis-dev Bot reviewed Dec 27, 2024

View reviewed changes

srijanpatel merged commit 63a8f51 into main Dec 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/merge node#67

Feat/merge node#67
srijanpatel merged 29 commits intomainfrom
feat/merge-node

preet-bhadra commented Dec 27, 2024 •

edited by ellipsis-dev Bot

Loading

Uh oh!

preet-bhadra commented Dec 27, 2024

Uh oh!

ellipsis-dev Bot left a comment

Uh oh!

srijanpatel left a comment

Uh oh!

srijanpatel Dec 27, 2024

Uh oh!

srijanpatel Dec 27, 2024

Uh oh!

srijanpatel Dec 27, 2024

Uh oh!

srijanpatel Dec 27, 2024

Uh oh!

srijanpatel Dec 27, 2024

Uh oh!

ellipsis-dev Bot left a comment

Uh oh!

srijanpatel commented Dec 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

preet-bhadra commented Dec 27, 2024 • edited by ellipsis-dev Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

preet-bhadra commented Dec 27, 2024

Uh oh!

ellipsis-dev Bot left a comment

Choose a reason for hiding this comment

Uh oh!

srijanpatel left a comment

Choose a reason for hiding this comment

Uh oh!

srijanpatel Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

srijanpatel Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

srijanpatel Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

srijanpatel Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

srijanpatel Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev Bot left a comment

Choose a reason for hiding this comment

Uh oh!

srijanpatel commented Dec 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

preet-bhadra commented Dec 27, 2024 •

edited by ellipsis-dev Bot

Loading