Skip to content
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

101 featch available variables from previous steps #8062

Merged
merged 20 commits into from
Oct 28, 2024

Conversation

martmull
Copy link
Contributor

@martmull martmull commented Oct 25, 2024

  • add outputSchema in workflow step settings
  • use outputSchemas to compute step available variables
demo.mov

@martmull martmull force-pushed the 101-featch-available-variables-from-previous-steps branch 5 times, most recently from 37c2513 to c483f4c Compare October 25, 2024 13:50
@martmull martmull force-pushed the 101-featch-available-variables-from-previous-steps branch from c483f4c to f89eee1 Compare October 25, 2024 14:21
@martmull martmull marked this pull request as ready for review October 25, 2024 14:34
@martmull martmull force-pushed the 101-featch-available-variables-from-previous-steps branch from 8a8e8ef to 450a397 Compare October 25, 2024 15:42
Copy link
Contributor

@thomtrp thomtrp left a comment

Choose a reason for hiding this comment

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

Great work 💪

Overall I wonder if we will not trigger too many calculation. The user may be playing with different nodes and we should not always generate the output schema. I start to wonder if we should not use a button to generate variables as ActivePieces and Windmill do...

@martmull
Copy link
Contributor Author

@thomtrp I agree with your concern, think we can try like this and update the behavior in a future PR if necessary

@martmull martmull force-pushed the 101-featch-available-variables-from-previous-steps branch from 642b6b6 to f156c1a Compare October 28, 2024 09:14
Copy link
Contributor

@thomtrp thomtrp left a comment

Choose a reason for hiding this comment

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

Ok so comments that will remain open:

  • build available variables schema should be done on backend side
  • computing step output schema should not happen on any update

Let's fix remaining comments and move on. We will test the feature like this and we will discuss with product how we can get a more efficient behavior in UI.

Copy link
Contributor

@thomtrp thomtrp left a comment

Choose a reason for hiding this comment

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

Great work 🔥

@Devessier
Copy link
Contributor

Great job, @martmull! Your work is fantastic.

@martmull martmull enabled auto-merge (squash) October 28, 2024 11:21
@martmull martmull merged commit 1aa961d into main Oct 28, 2024
18 of 19 checks passed
@martmull martmull deleted the 101-featch-available-variables-from-previous-steps branch October 28, 2024 11:25
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.

3 participants