Update-with-start #702

dandavison · 2024-12-16T02:09:39Z

Add an update-with-start API, using the MultiOperation gRPC API.

The test suite is not complete yet, but please feel free to review.

In addition to the tests, an example of using the new API is temporalio/samples-python#156:

    cart_id = f"cart-{session_id}"
    start_op = WithStartWorkflowOperation(
        ShoppingCartWorkflow.run,
        id=cart_id,
        id_conflict_policy=common.WorkflowIDConflictPolicy.USE_EXISTING,
        task_queue="uws",
    )
    try:
        price = Decimal(
            await temporal_client.execute_update_with_start(
                ShoppingCartWorkflow.add_item,
                ShoppingCartItem(sku=item_id, quantity=quantity),
                start_workflow_operation=start_op,
            )
        )
    except WorkflowUpdateFailedError:
        price = None

    return price, await start_op.workflow_handle()

From the docstring:

        A WorkflowIDConflictPolicy must be set in the start_workflow_operation. If the
        specified workflow execution is not running, a new workflow execution is started
        and the update is sent in the first workflow task. Alternatively if the specified
        workflow execution is running then, if the WorkflowIDConflictPolicy is
        USE_EXISTING, the update is issued against the specified workflow, and if the
        WorkflowIDConflictPolicy is FAIL, an error is returned. This call will block until
        the update has completed, and return the update result. Note that this means that
        the call will not return successfully until the update has been delivered to a
        worker.

cretz

Mostly minor things, overall LGTM

cretz · 2024-12-16T20:49:58Z

temporalio/client.py

@@ -788,6 +780,308 @@ def get_workflow_handle_for(
            result_type=defn.ret_type,
        )

+    # Overload for no-param update
+    @overload
+    async def execute_update_with_start(


I think this needs to be execute_update_with_start_workflow to differentiate from all of the non-workflow stuff on the client (same for start_update_with_start_workflow)

Thanks, I hadn't appreciated the distinction from Java/TS's "workflow clients". Done.

cretz · 2024-12-16T20:54:37Z

temporalio/client.py

+            update: Update function or name on the workflow. arg: Single argument to the
+            update. args: Multiple arguments to the update. Cannot be set if arg is.
+            start_workflow_operation: a WithStartWorkflowOperation definining the
+                WorkflowIDConflictPolicy and how to start the workflow in the event that a
+                workflow is started.
+            id: ID of the update. If not set, the default is a new UUID. result_type: For


Some newlines aren't showing here to separate the args

Thanks, fixed.

cretz · 2024-12-16T20:56:23Z

temporalio/client.py

+
+    # Overload for no-param workflow, with_start
+    @overload
+    def __init__(


This may need to new user metadata stuff that was added in #701

Yup, rebased.

cretz · 2024-12-16T20:57:25Z

temporalio/client.py

        )
+        self._workflow_handle: Future[WorkflowHandle[SelfType, ReturnType]] = Future()
+
+    async def workflow_handle(self) -> WorkflowHandle[SelfType, ReturnType]:


No strong opinion here, but would be ok if this was just called handle since it's in the start-workflow class

I think I'll leave it as workflow_handle. It's an "operation" which is not really a standard SDK concept, so the clarity probably helps IMO, plus handle could be mistaken for a verb (a lot of non-English-first-language-speakers find the handle-verb, handle-noun, handler-noun terms confusing in software.)

Ok. May be worth noting Go, Java, and .NET WithStartWorkflowOperation classes to not qualify their methods with workflow either which makes sense.

As decided in group discussion, we're going with await start_op.workflow_handle()

cretz · 2024-12-16T20:59:02Z

temporalio/client.py

@@ -4919,6 +5460,12 @@ async def start_workflow_update(
        """Called for every :py:meth:`WorkflowHandle.update` and :py:meth:`WorkflowHandle.start_update` call."""
        return await self.next.start_workflow_update(input)

+    async def start_workflow_update_with_start(


I think this would be more clearly named start_update_with_start_workflow (and changing input class name too). IMO it makes sense to have the method name match the client call (which should also be that IMO).

I agree. But signal/query/update/terminate do not do this; they have query_workflow, signal_workflow, start_workflow_update, terminate_workflow, etc.

What do you think? It seems unfortunately that the most consistent name is start_workflow_update_with_start_workflow, in order to match start_workflow_update

OK discussed offline; it is now named start_update_with_start_workflow to match the client call. (The idea behind start_workflow_update is that the workflow handle can name it start_update but in other contexts we need to be more explicit about what "update" is).

cretz · 2024-12-16T21:04:40Z

temporalio/client.py

+        start_req = (
+            await self._build_update_with_start_start_workflow_execution_request(
+                start_input
+            )
+        )
+        update_req = await self._build_update_workflow_execution_request(
+            update_input, workflow_id=start_input.id
+        )


I am concerned that exceptions that happen here (e.g. serializing workflow/update args) will leave someone waiting on the workflow handle hanging. Same for things like cancel.

Is it possible to make sure that no matter how this method exits, the start operation handle awaiter is updated? I didn't check if other langs did this, but I think it makes sense.

Yes absolutely, good call. Done. Both the exception handling and the inner logic are involved so the inner logic is in a separate function, with a finally case in the outer function ensuring that the promise is rejected in all cases.

cretz · 2024-12-16T21:07:48Z

temporalio/client.py

+                            ),
+                            None,
+                        )
+                        if status and status.code in RPCStatusCode:


Should add special handling for WorkflowAlreadyStartedError here I think

Thank you, that's done now, and in general the error handling and poll loop has been rewritten.

cretz · 2024-12-16T21:09:55Z

temporalio/client.py

+                temporalio.api.workflowservice.v1.StartWorkflowExecutionResponse
+            ] = [
+                r.start_workflow
+                for r in multiop_response.responses


Doesn't really matter, but I think we should be able to assume the indexes of responses match 1:1 with the request objects. So may be able to just do multiop_response.responses[0].start_workflow and not have to loop.

Yeah I decided that you're right and I didn't need to program so defensively here. So more minimal/cleaner now.

cretz · 2024-12-16T21:11:54Z

temporalio/client.py

-        # Build the handle. If the user's wait stage is COMPLETED, make sure we
-        # poll for result.
-        handle: WorkflowUpdateHandle[Any] = WorkflowUpdateHandle(
+        return WorkflowUpdateHandle(


When we do execute_update (or the user set wait stage to completed), we don't return the handle until we have polled for outcome

You're right, there's a test that I'd intended to be confirming that the result is fetched in that context, but the test was misconceived. It's not the easiest thing to test for, but I've put the poll call in. Maybe we can add a test later based on manipulating the history long poll timeout.

break handle = WorkflowUpdateHandle( client=self._client, id=update_req.request.meta.update_id, workflow_id=start_input.id, workflow_run_id=start_response.run_id, known_outcome=known_outcome, ) if update_input.wait_for_stage == WorkflowUpdateStage.COMPLETED: await handle._poll_until_outcome() return handle

cretz

Nothing blocking

cretz · 2024-12-18T20:45:49Z

temporalio/client.py

+
+    # TODO (dan):
+    # temporalio/client.py:926: error: Overloaded function implementation does not accept all possible arguments of signature 1  [misc]
+    async def start_update_with_start_workflow(  # type: ignore


I think here and other user facing entry points (e.g. execute equivalent and the class doc for WithStartWorkflowOperation) should have the "experimental" warning that looks similar to what we're removing on #707.

Added experimental warnings

cretz · 2024-12-18T20:46:32Z

temporalio/client.py

@@ -5418,7 +5984,8 @@ async def start_workflow_update(
        handle: WorkflowUpdateHandle[Any] = WorkflowUpdateHandle(
            client=self._client,
            id=req.request.meta.update_id,
-            workflow_id=input.id,
+            workflow_id=workflow_id,
+            # TODO: Why don't we use the run ID from the update response here?


This is a bug we believe (and it exists in .NET too)

Deleted comment from this PR

cretz · 2024-12-18T20:49:10Z

temporalio/client.py

+                            if (
+                                st.details
+                                and not st.details[0].Is(
+                                    temporalio.api.failure.v1.MultiOperationExecutionAborted.DESCRIPTOR
+                                )
+                            )


What did we end up deciding here about what a successful start but failed update looks like? I think server side today st.details is never None correct? Should we ignore OK statuses? This is super rare and so non-blocking for this PR, but whatever the decision we may need to apply to other SDKs too.

I'm skipping OK statuses in Python and TS.

cretz · 2024-12-19T13:18:58Z

temporalio/client.py

+                            st
+                            for st in multiop_failure.statuses
+                            if (
+                                st.details


I think this logic is a bit off. I don't think we should require details to be considered an error

Thanks, that sounds right. 5a06a6a

dandavison force-pushed the uws branch 4 times, most recently from 5a868c0 to 9f1d783 Compare December 16, 2024 19:46

dandavison marked this pull request as ready for review December 16, 2024 20:23

dandavison requested a review from a team as a code owner December 16, 2024 20:23

cretz reviewed Dec 16, 2024

View reviewed changes

dandavison force-pushed the uws branch 3 times, most recently from 0b37fd2 to 3c17209 Compare December 18, 2024 18:15

cretz approved these changes Dec 18, 2024

View reviewed changes

cretz approved these changes Dec 19, 2024

View reviewed changes

dandavison force-pushed the uws branch 2 times, most recently from faf4140 to 10f3026 Compare December 19, 2024 17:05

Update-with-start

5b631a0

dandavison force-pushed the uws branch from 8c5834b to 5b631a0 Compare December 19, 2024 18:22

dandavison merged commit 540faeb into main Dec 19, 2024
12 checks passed

dandavison deleted the uws branch December 19, 2024 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update-with-start #702

Update-with-start #702

dandavison commented Dec 16, 2024 •

edited

Loading

cretz left a comment

cretz Dec 16, 2024

dandavison Dec 17, 2024

cretz Dec 16, 2024

dandavison Dec 18, 2024

cretz Dec 16, 2024

dandavison Dec 18, 2024

cretz Dec 16, 2024

dandavison Dec 17, 2024

cretz Dec 17, 2024 •

edited

Loading

dandavison Dec 19, 2024

cretz Dec 16, 2024

dandavison Dec 18, 2024

dandavison Dec 18, 2024 •

edited

Loading

dandavison Dec 18, 2024

cretz Dec 16, 2024

dandavison Dec 18, 2024

cretz Dec 16, 2024

dandavison Dec 18, 2024

cretz Dec 16, 2024

dandavison Dec 18, 2024

cretz Dec 16, 2024

dandavison Dec 18, 2024

cretz left a comment

cretz Dec 18, 2024

dandavison Dec 19, 2024

cretz Dec 18, 2024

dandavison Dec 18, 2024

cretz Dec 18, 2024

dandavison Dec 18, 2024

cretz Dec 19, 2024

dandavison Dec 19, 2024

Update-with-start #702

Update-with-start #702

Conversation

dandavison commented Dec 16, 2024 • edited Loading

cretz left a 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

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 Dec 17, 2024 • 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 Dec 18, 2024 • 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

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 left a 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

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

dandavison commented Dec 16, 2024 •

edited

Loading

cretz Dec 17, 2024 •

edited

Loading

dandavison Dec 18, 2024 •

edited

Loading