-
Notifications
You must be signed in to change notification settings - Fork 16
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
pkg/workflows/sdk: add WorkflowSpecFactory.BeginSerial/BeginAsync #821
base: workflowchart
Are you sure you want to change the base?
Conversation
1d141c0
to
1033cb9
Compare
afa5ef4
to
2a277c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, if you can add the invisible dependency to in this function then the workflow engine will respect it. I don't think this can be merged without it.
w.serialMode = true | ||
} | ||
|
||
func (w *WorkflowSpecFactory) BeginAsync() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: serial and parallel pair better, or concurrent. All the steps are actually async under the hood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would EndSerial()
make sense?
Edit: Or just SetSerial(bool)
in place of both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking forward about how to generalize this a bit might help too. Right now we have one global mode, but what if we want to build sub-graphs within one workflow, and control the mode of each subgraph as we construct it? I think it will be important for the API to be consistent throughout, and one test would be whether we can write helper funcs that are able to add steps to a workflow or a sub-workflow, without any special logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree that subgraphs are important generalization. subgraphs and phases/checkpoints that we discussed yesterday are similar concepts. i think you can implement phases with an ordered list of subgraphs such that the previous element in the list must complete before starting the given subgraph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes me think of another error scenario - when a step fails, and so we do not execute the subsequent dependent steps, must we still propagate failure through them? i.e. if we have A --> B -..-> C, where B uses data from A and C runs only if B fails, then what happens if A fails? Do we skip B (and C)? Or do we propagate the failure through B, triggering C? I think users will want to express the latter, in terms of "if any part of the subgraph fails, run this alternate subgraph"
func NewSerialWorkflowSpecFactory(params NewWorkflowParams) *WorkflowSpecFactory { | ||
f := NewWorkflowSpecFactory(params) | ||
f.BeginSerial() | ||
return f | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it just be a param? We can default it either way right now because we don't have any actual WASM workflows yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a param is necessary because it is trivial to call the method instead. I only added this as a convenience to cater to the "serial only" user experience, but would gladly remove it.
b26ed1a
to
7c3e9ff
Compare
74d39f7
to
4a33a18
Compare
@@ -35,3 +153,61 @@ func (w *WorkflowSpec) Steps() []StepDefinition { | |||
s = append(s, w.Targets...) | |||
return s | |||
} | |||
|
|||
func (w *WorkflowSpec) FormatChart() (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jmank88 , I am trying to figure out how we provide the visual to users via CLI. From the dev platform CLI perspective,
- We get the paths of local files from users and get the wasm binary and config as byte[]
- We can get an instance of WorkflowSpec via GetWorkflowSpec
- We can call this FormatChart() to get the visual? Is the visual a SVG or image to the local?
Want to hear your thoughts on the flow, and how to integrate. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this discussion over to #790, which just introduces the chart without any of this builder stuff.
Depends on #790
This PR extends
WorkflowSpecFactory
to includeBeginSerial()
andBeginAsync()
methods for toggling between two builder modes.Here is an example from a test which reads from two APIs and one chain, then writes on chain:
Here is the same workflow with (dotted) edges added in order to serialize the overall DAG:
TODO
Condition
functional