Phase 2 - Initial workflow implementation

[cretz]

Phase 2 items included:

• Separate payload converters from payload codecs in data converters
• Typed workflow functions on client
• Typed workflow definitions
• Basic signal/query support including dynamic signal/query
• Executing activity, local activity, and child activity
• Timers and conditions
• Cancellation of things inside workflows
• Workflow runtime backed by event loop
• API preparing for future sandboxing of workflows

Phase 2 TODO:

• Payload codec (EDIT: Done)
• Async activity (Async activity support #24)
• Child/external workflow signal (EDIT: Done)
• External workflow cancel (EDIT: Done)
• Workflow logger (EDIT: Done)
• In-workflow signal/query handlers (EDIT: Done)
• Fatal worker errors (Propagate fatal worker errors #25)
• Search attributes (Search attributes #26)
• Continue as new (EDIT: Done)
• Patching (Versioning/Patching #27)
• Stack trace query (EDIT: Done, albeit ugly)
• Lots of little TODOs sprinkled throughout
• More tests (More workflow tests #28)
• "Real" public documentation (and trim down README to mostly just links)

Notes:

• The internals were reworked a few times in order to optimize for maintenance (which is the highest burden of these SDKs in my experience). A lot of effort was spent on clarity and simplicity.
• I apologize for such a large PR, but it was the smallest I could make and capture all things needed to be confident in the API design.
• The documentation in the README is intentionally sparse and bulleted. The expectation is that the documentation team can take it and run with it. Once public Python documentation appears, I expect to trim the README down to mostly just links and build/dev details.

[Sushisource]

Lookin' real nice

[Sushisource]

The number of threads here should be independent of (and probably lower than) max WFTs. There shouldn't be any blocking stuff going on in these threads, so it ought to be fine for them to be sliced up among a number of active workflows

[cretz]

There shouldn't be any blocking stuff going on in these threads

There is in our case because this is how we do deadlock detection. Essentially, every WFT runs in its own thread. Of course WFTs should only take a couple milliseconds so that is harmless, but technically 5 WFTs running means 5 threads running.

[bergundy]

Hmm.. do you need a thread per concurrent running workflow? Is it just easier to manage? Why not use a single thread?

[cretz]

do you need a thread per concurrent running workflow?

No. It's a choice.

Is it just easier to manage? Why not use a single thread?

We could use a single thread, but now you've removed all concurrency from workflow tasks for a worker. The goal of a thread is to detect deadlocks and support concurrent workflow tasks. I could definitely reduce the number here or even use a single thread if we wanted.

Note that the current model supports things like using non-asyncio HTTP calls inside payload codec.

[Sushisource]

My main concern is you'd possibly run into problems with consuming too many threads and the associated overhead as is the case with Java which uses the same approach.

Deadlock detection is important, but exists to solve a problem that should be rather temporary. Personally I think it's probably OK to lock up a few other in-flight workflows for a second because of a deadlock in one. The user is going to need to address that immediately in any case. Whereas the thread-per-workflow thing is possibly a problem for the happy path

[cretz]

I can understand that. Suggestion on what the default max-threads should be? I can make it something like "5". That sound good? I have confirmed Java uses max-concurrent-WFT to determine thread pool size, though I understand we may not want to mimic Java here.

(of course many users might be better off passing in their own thread pools they can share across workers, both activity and workflow)

[cretz]

After further discussion, I'll put this at some low number like "4". The GIL prevents most practical concurrency anyways (though newer Python versions are working on improving this).

[bergundy]

I'm not concerned with thread consumption since we don't actually use a thread per workflow for caching purposes like Java does.
I do tend to agree with @Sushisource though that there's not much benefit to using more than 1 thread here since activations will be processed serially anyways, since there's no I/O in workflow code.
Thread will buy you some concurrency in case the workflow runs CPU intensive work which it technically shouldn't but I don't see any harm in setting this to a very low value like Chad suggests but I wouldn't tie this to the max concurrent WFTs since there's work that can be done concurrently before we call into user code like payload codecs.

[cretz]

I have changed this to the CPU count or 4, whichever is higher.

[cretz]

Suggested change

	RuntimeError("Unknown child start fail cause: {job.failed.cause}")
	RuntimeError(f"Unknown child start fail cause: {job.failed.cause}")

[cretz]

Done

[bergundy]

Is there some lint rule that can identify this?

[cretz]

Hrmm, maybe. Currently I only use a style linter and MyPy and a doc linter. I don't have a general purpose linter for general Python suggestions. I will see if I can find a well-accepted one.

[cretz]

I checked pylint and confirmed it wouldn't catch this. I suppose there would be too many false positives with normal brace use in string literals.

I also just gave pylint a general look and it doesn't give us more than MyPy really, it's actually worse (can't recognize stub files). And black solves the formatting. So I think we're as good as we can reasonably get with linting currently.

[cretz]

I apologize for the force pushes. They are minor attempts to fix whatever I have done recently to make tests flaky on top of the custom-signal/query-handler registration that I committed.

[lorensr]

Great readme! ☺️

[bergundy]

I fixed this already

[cretz]

Thanks! Will remove this on my next core update

[bergundy]

Is there a place where you prevent generating new commands after a terminal workflow command has been emitted?

[bergundy]

Could simplify with special value: name: Union[Literal[CatchAll, str]] and get rid of dynamic..

[cretz]

Not really, because this is the final destination of the overloads. And one overload is name= and the other is dynamic=. So I have to accept both parameters in the final target method because it has to be a superset of overload parameters. Unless you are saying @workflow.signal(name=CatchAll) is more user-friendly @workflow.signal(dynamic=True).

Note, non-@overload functions are not documentation in API doc. This is basically the best way to do mutually exclusive parameters.

[bergundy]

I was thinking @workflow.signal(name=CatchAll) is more friendly, it also implies that you can combine named and catchall signal handlers.

[cretz]

I disagree it's more friendly (and it'd be @workflow.signal(name=workflow.CatchAll)). They are different mutually exclusive parameters. Also, you can't combine them because the dynamic catch all one is given a name and then the args (we actually enforce at runtime that they accept self, a name, then positional varargs).

[cretz]

After discussion it was decided not to reuse the name as a way to mark this signal dynamic/catch-all

[cretz]

Is there a place where you prevent generating new commands after a terminal workflow command has been emitted?

Not directly. Rather I am forcing the loop to stop, which should prevent any other command generation, in the two places that can generate terminal workflow commands - workflow function return or signal handler failure.

See the _force_stop_loop field. Though now I need to revisit it to change the comment I use above its definition (no longer valid) and make sure I set it on workflow success instead of just workflow failure. I will update that shortly.

[cretz]

Is there a place where you prevent generating new commands after a terminal workflow command has been emitted?

See the _force_stop_loop field.

Well, it looks like the _force_stop_loop isn't gonna work because I want to run queries after a terminal workflow command. I have chosen to run the query handler inside the event loop so it has the local event loop as context (technically I allow an async query handler though one could argue I should not).

So I am going to add something that removes non-respond_to_query commands after a complete_workflow_execution or fail_workflow_execution or continue_as_new_workflow_execution command. Due to how the commands are built in a copy-on-write way, I may just prevent them from being added instead of culling them at the end.

EDIT: I have pushed changes to cull them before returning

[bergundy]

nit: use keyword argument so it's clear what is False

[cretz]

We don't support keyword arguments in SDKs. Were this something besides a test case, I'd make params a dataclass.

[bergundy]

There's probably more, I'd review the other SDKs' tests to see if there's anything important we're missing.

[bergundy]

Test for query after workflow completed / failed

[bergundy]

https://github.com/temporalio/sdk-typescript/blob/main/packages/test/src/test-signals-are-always-delivered.ts

[cretz]

Last call on this PR. While I will be adding more tests throughout the day and maybe also updating the core submodule, it is basically ready to merge (with all outstanding phase 2 TODOs to become issues).

[cretz]

Merging. All outstanding tasks have been created as issues.