Workflow Update #400

Sushisource · 2023-10-13T22:00:08Z

DRAFT - Still need to finish async updates & add more tests

What was changed

Implement Workflow Update

Why?

Duh

Checklist

Closes [Python] Workflow update support #336
How was this tested:
Added tests
Any docs updates needed?
Yup

Sushisource · 2023-10-13T22:01:47Z

temporalio/workflow.py

+                "Dynamic updates do not support a vararg third param, use Sequence[RawValue]",
+            )
+        setattr(fn, "__temporal_update_definition", defn)
+        setattr(fn, "validator", partial(_update_validator, defn))


Probably this should return a class rather than doing this, so IDE docstring/hints work on validator

I think this is quite nice now - we might consider changing the others to the extent it's compatible. Fewer magic attributes.

This is still nicer I think, but avoids using a class decorator because of unlimited type hell.

.dockerignore

cretz · 2023-10-23T13:48:57Z

README.md

@@ -515,6 +515,12 @@ class GreetingWorkflow:
    @workflow.query
    def current_greeting(self) -> str:
        return self._current_greeting
+
+    @workflow.update
+    def set_and_get_greeting(self, greeting: str) -> str:


Hrmm, I wonder if just altering update_salutation to be an update that return the previous salutation part would be enough. Also, I wonder if calling this update_greeting is descriptive enough. I don't think it matters much. But I will take the approach of return-old-thing in the .NET PR.

cretz · 2023-10-23T13:49:39Z

README.md

+* `@workflow.update` - Defines a method as an update
+  * May both accept as input and return a value
+  * May be `async` or non-`async`
+  * May mutate workflow state, and make calls to other workflow APIs like starting activities, etc.


Suggested change

* May mutate workflow state, and make calls to other workflow APIs like starting activities, etc.

* May mutate workflow state, make calls to other workflow APIs like starting activities, etc.

(pedantic, can ignore)

scripts/Dockerfile

temporalio/workflow.py

temporalio/worker/_workflow_instance.py

temporalio/workflow.py

dandavison · 2023-10-21T16:05:00Z

temporalio/worker/_workflow_instance.py

+        async def run_update() -> None:
+            command = self._add_command()
+            command.update_response.protocol_instance_id = job.protocol_instance_id
+            try:


It might be helpful to place this method next to _run_top_level_workflow_function, or have them mention each other in a comment, for people seeking to understand our error handling behavior in the different handlers and in the main workflow code. (I'm assuming that for now we don't see any attractive way for them to share implementation.)

temporalio/worker/_interceptor.py

README.md

temporalio/client.py

temporalio/worker/_workflow_instance.py

cretz · 2023-10-24T13:20:16Z

Saw some things resolved/addressed but maybe not pushed yet pending core update or something. Will wait for push before re-reviewing.

cretz

LGTM, mostly just naming of tracing spans and interceptor classes and similar small things. Oh and we probably want the update handle to have its return type generic/typed.

temporalio/client.py

temporalio/contrib/opentelemetry.py

temporalio/worker/_interceptor.py

temporalio/worker/_workflow_instance.py

cretz · 2023-10-24T18:11:55Z

temporalio/worker/_workflow_instance.py

+        # the specific named update - we shouldn't fall back to the dynamic validator
+        # for some defined, named update which doesn't have a defined validator.
+        handler = self._instance.workflow_get_update_validator(input.update)
+        # Validator may not be defined


In theory this should not longer happen anymore since you check before interceptor, but fine to check again in here (just in case they did something wonky like unset it in an outer interceptor)

.dockerignore

temporalio/client.py

tests/contrib/test_opentelemetry.py

dandavison · 2023-10-25T07:23:40Z

temporalio/client.py

+            outcome = self._known_result
+            return await _update_outcome_to_result(
+                outcome,


I'd like to change the _known_result variable name to _known_outcome. That name matches its type, and it's an important distinction since the outcome can be Failed, in which case there isn't a "result" in the sense that that word is used everywhere else.

(I'm not making this comment during a code review -- I'm making it after getting briefly confused by this issue while reading the Python implementation in the course of Typescript implementation, so a data point exists that this is a real confusion risk.)

.NET did call it KnownOutcome but I don't think it matters much since this is private API. It seems ok to me to name something "known result" if it's a "known" thing for the "result" method later, but I'm ok changing it to known_outcome too.

temporalio/client.py

dandavison · 2023-10-25T09:56:23Z

temporalio/client.py

+            TimeoutError: The specified timeout was reached when waiting for the update result.
+            RPCError: Update result could not be fetched for some other reason.
+        """
+        if self._known_result is not None:


nit: I'd prefer to write this as if self._known_result. The types establish that bool(self._known_result) iff the response has an Outcome.

temporalio/client.py

dandavison · 2023-10-25T19:58:07Z

temporalio/client.py

+        *,
+        args: Sequence[Any] = [],
+        id: Optional[str] = None,
+        wait_for_stage: temporalio.api.enums.v1.UpdateWorkflowExecutionLifecycleStage.ValueType = temporalio.api.enums.v1.UpdateWorkflowExecutionLifecycleStage.UPDATE_WORKFLOW_EXECUTION_LIFECYCLE_STAGE_ADMITTED,


I understand why you defaulted this to ADMITTED (it's what's most natural when asking for a handle) and I know the API is not exposed to users, but ADMITTED is completely unimplemented server-side and there is no agreed plan for how it would be implemented yet, and no timeline for implementing it, so maybe it's a bit odd for it to be in the SDK codebase? Maybe it should be a required positional arg.

dandavison

LGTM!

Sushisource added 5 commits October 11, 2023 11:11

Update Core

e839219

Add docker image for proto building

cf7cc9b

Basic happy path working

2b0d6f0

Validator running

c2281c5

Add UpdateHandle. Polling not yet implemented.

84b909c

Sushisource commented Oct 13, 2023

View reviewed changes

Add polling

d1e0681

Sushisource force-pushed the workflow-update branch 2 times, most recently from 488fa66 to 1da053f Compare October 16, 2023 21:49

Linting / mypy

713c847

Sushisource force-pushed the workflow-update branch from 1da053f to 713c847 Compare October 16, 2023 21:53

Sushisource added 2 commits October 17, 2023 11:14

Update core / additional failure path tests

abe36c3

Dynamic update handlers

533b9f7

Sushisource force-pushed the workflow-update branch from 818f6f2 to 533b9f7 Compare October 17, 2023 21:46

Use class for update decorator

5daec88

Sushisource force-pushed the workflow-update branch from 6f8121c to 5daec88 Compare October 17, 2023 23:28

Sushisource added 2 commits October 17, 2023 16:56

Add update definitions to workflow definition test

668532d

Finally! Type system defeated!

3a9dc2c

Sushisource force-pushed the workflow-update branch from dc23266 to 3a9dc2c Compare October 19, 2023 23:25

Sushisource added 3 commits October 19, 2023 16:47

Rename update to execute_update and hide wait_for_stage param

6a7c52a

Always run validator interceptor

4be97cc

Clean up some last TODOs / workflow definition error tests

953b8e4

Sushisource marked this pull request as ready for review October 20, 2023 18:26

Sushisource requested a review from a team as a code owner October 20, 2023 18:26

Sushisource added 2 commits October 20, 2023 14:49

3.7 throws this error differently

2e54df6

Update README

188bf97

Sushisource force-pushed the workflow-update branch from 9ddd357 to 188bf97 Compare October 20, 2023 23:39

Skip updates in Java test server for now

03e3197

cretz reviewed Oct 23, 2023

View reviewed changes

dandavison reviewed Oct 23, 2023

View reviewed changes

Sushisource added 2 commits October 23, 2023 11:13

Make validator interceptor sync

cc0871d

Address various review comments

de98531

Sushisource and others added 3 commits October 24, 2023 10:03

Update core & add inbound tracing interceptor

afc1807

Make sure only temporal errors fail update during handler

8b41042

Merge branch 'main' into workflow-update

5d2b614

cretz reviewed Oct 24, 2023

View reviewed changes

Sushisource added 2 commits October 24, 2023 15:00

Renames / no poll update tracing

6b0972d

Add start_update overloads / update handle type

020f663

cretz reviewed Oct 25, 2023

View reviewed changes

.dockerignore Outdated Show resolved Hide resolved

cretz reviewed Oct 25, 2023

View reviewed changes

temporalio/client.py Outdated Show resolved Hide resolved

temporalio/client.py Outdated Show resolved Hide resolved

cretz reviewed Oct 25, 2023

View reviewed changes

tests/contrib/test_opentelemetry.py Outdated Show resolved Hide resolved

dandavison reviewed Oct 25, 2023

View reviewed changes

Sushisource force-pushed the workflow-update branch from b1a5ee7 to ba4bda7 Compare October 25, 2023 16:38

Ensure otel test will wait until after task fails to send update

6670ee5

Sushisource force-pushed the workflow-update branch from ba4bda7 to 6670ee5 Compare October 25, 2023 17:05

Sushisource added 2 commits October 25, 2023 11:00

Latest review comments

1731fcf

Fix failure setting on update rejection

1c18fc2

dandavison reviewed Oct 25, 2023

View reviewed changes

cretz approved these changes Oct 25, 2023

View reviewed changes

Merge branch 'main' into workflow-update

fabaecf

dandavison approved these changes Oct 25, 2023

View reviewed changes

Sushisource merged commit 2c15ed3 into main Oct 25, 2023
12 checks passed

Sushisource deleted the workflow-update branch October 25, 2023 21:05

troyanov mentioned this pull request Oct 29, 2023

[Bug] Compilation fails for ARCH s390x and ppc64el #394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workflow Update #400

Workflow Update #400

Sushisource commented Oct 13, 2023 •

edited

Loading

Sushisource Oct 13, 2023

Sushisource Oct 17, 2023 •

edited

Loading

Sushisource Oct 19, 2023

cretz Oct 23, 2023

cretz Oct 23, 2023

dandavison Oct 21, 2023

cretz commented Oct 24, 2023

cretz left a comment •

edited

Loading

cretz Oct 24, 2023

dandavison Oct 25, 2023

cretz Oct 25, 2023 •

edited

Loading

Sushisource Oct 25, 2023

dandavison Oct 25, 2023

dandavison Oct 25, 2023

dandavison left a comment

	* May mutate workflow state, and make calls to other workflow APIs like starting activities, etc.
	* May mutate workflow state, make calls to other workflow APIs like starting activities, etc.

Workflow Update #400

Workflow Update #400

Conversation

Sushisource commented Oct 13, 2023 • edited Loading

What was changed

Why?

Checklist

Choose a reason for hiding this comment

Sushisource Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz commented Oct 24, 2023

cretz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison left a comment

Choose a reason for hiding this comment

Sushisource commented Oct 13, 2023 •

edited

Loading

Sushisource Oct 17, 2023 •

edited

Loading

cretz left a comment •

edited

Loading

cretz Oct 25, 2023 •

edited

Loading