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

Workflow Invocation Methods #432

Merged
merged 16 commits into from
May 13, 2024
Merged

Workflow Invocation Methods #432

merged 16 commits into from
May 13, 2024

Conversation

kraftp
Copy link
Contributor

@kraftp kraftp commented May 9, 2024

This PR introduces two new methods for invoking workflows from handlers. For workflow bar in class Foo with argument arg:

  • ctxt.startWorkflow(Foo).bar(arg) starts the workflow and returns its handle. This is equivalent to the current ctxt.invoke(Foo).bar(arg).
  • ctxt.invokeWorkflow(Foo).bar(arg) executes the workflow and returns its result. This is equivalent to the current ctxt.invoke(foo).bar(arg).then((x)=>x.getResult()). The latter is very unintuitive and is tripping users up.

To avoid confusion, we also deprecate calling invoke on workflows--use one of the new methods instead.

My hope is that this makes it much clearer to users what is actually happening when a handler invokes a workflow.

src/httpServer/handler.ts Outdated Show resolved Hide resolved
@kraftp kraftp marked this pull request as ready for review May 10, 2024 23:56
@chuck-dbos
Copy link
Collaborator

Do we have an opportunity here to make invocation of child workflows have the same syntax as invoking workflows from the handler (and transactions and communicators also)?

@qianl15
Copy link
Member

qianl15 commented May 11, 2024

Do we have an opportunity here to make invocation of child workflows have the same syntax as invoking workflows from the handler (and transactions and communicators also)?

This traced back to when we implemented child workflows. I tried to use the same invoke syntax but the issue was recursive return type inference loop. If developers forgot to specify a return type for a workflow, then Typescript compiler will complain "method doesn't exist on the return value of invoke()" for all workflows. The root cause was the WF invoke() would return itself so that the compiler will fall into a recursive loop to infer the return type: "implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions".

Back then we discussed a solution: we can enforce people to always explicitly specify the return type for a workflow.

@chuck-dbos
Copy link
Collaborator

This traced back to when we implemented child workflows. I tried to use the same invoke syntax but the issue was recursive return type inference loop.

So, if we could avoid this problem cleanly, then that is what we would want?

@kraftp
Copy link
Contributor Author

kraftp commented May 11, 2024

This traced back to when we implemented child workflows. I tried to use the same invoke syntax but the issue was recursive return type inference loop.

So, if we could avoid this problem cleanly, then that is what we would want?

Yes, it would be much cleaner! Do you have an idea of how to do it? Harry tried back when we first did invoke and couldn't get past this problem, and I tried again as part of this PR and couldn't find a better solution.

@chuck-dbos
Copy link
Collaborator

Yes, it would be much cleaner! Do you have an idea of how to do it? Harry tried back when we first did invoke and couldn't get past this problem, and I tried again as part of this PR and couldn't find a better solution.

I would like time to take a shot at it tomorrow.

Comment on lines +158 to +160
invokeWorkflow<T extends object>(object: T, workflowUUID?: string): SyncHandlerWfFuncs<T> {
return this.mainInvoke(object, workflowUUID, false) as unknown as SyncHandlerWfFuncs<T>;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

tbh invokeWorkflow doesn't really convey the synchronous aspect of the call. What about something super explicit like runWorkflowToCompletion or startWorkflowSync, invokeWorkflowAndWait ?

Copy link
Contributor Author

@kraftp kraftp May 13, 2024

Choose a reason for hiding this comment

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

People intuitively expect invocation to be synchronous (invoke itself is synchronous) so calling it invokeWorkflow is clear, anything more complex may be confusing.

@chuck-dbos chuck-dbos self-requested a review May 13, 2024 16:56
@qianl15 qianl15 merged commit 480507b into main May 13, 2024
2 checks passed
@qianl15 qianl15 deleted the kraftp/runWorkflow branch May 13, 2024 17:08
qianl15 pushed a commit to dbos-inc/dbos-docs that referenced this pull request May 13, 2024
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.

4 participants