Required wait update stage, update polling improvements, and other update changes #521

cretz · 2024-05-08T20:38:16Z

What was changed

Always send id to server on update (SDK side defaults to uuid4())
Added temporalio.client.WorkflowUpdateStage enumerate that maps to the gRPC equivalent
Added required wait_for_stage parameter to Client.start_update. Disallow ADMITTED at runtime.
Added WorkflowHandle.get_update_handle and WorkflowHandle.get_update_handle_for. These accept a run ID, but default to one on the handle
Updated start update polling to poll until stage reached or ACCEPTED. If more polling needed, switches to result polling.

Don't wait on feature repo CI to pass before reviewing. I will create a feature repo branch to fix that.

Checklist

Fixes temporalio#484

…ther update changes Fixes temporalio#424 Fixes temporalio#485 Fixes temporalio#514

drewhoskins-temporal · 2024-05-09T00:36:50Z

temporalio/client.py

@@ -1950,8 +1956,10 @@ async def start_update(
        Args:
            update: Update function or name on the workflow.
            arg: Single argument to the update.
+            wait_for_stage: Required stage to wait until returning. ADMITTED is


The first sentence of this comment is self-referential and adds no value beyond the name of the field.
Generally, I'd like there to be more clues in the python SDK about what these wait stages mean, in particular flagging that the worker must be present. I'd suggest adding comments to the enum and/or linking out to docs.

The headline

Send an update request to the workflow and return a handle to it
also just echoes what we already know from the name of the method and the return type.

I think there's an opportunity to headline its semantics (like a glance at what you'd find in the docs) and link out to overview docs.

Context: A natural thing I've done when not finding any good description in the code is to find docs online. But I might find python-specific docs which also don't provide a good overview of semantics. As a n00b I might not even be aware that there are overview docs, as I don't have the ontology of temporal or its docs site in my head.
Giving me the basics here and then linking out to the right docs page would save a lot of time and help me learn.

The first sentence of this comment is self-referential and adds no value beyond the name of the field.

This is often the case with required docs. When you are forced to document self-documenting things, the sentences become a bit redundant. But it's preferred in Python libraries over the opposite which is optional documentation.

Generally, I'd like there to be more clues in the python SDK about what these wait stages mean, in particular flagging that the worker must be present. I'd suggest adding comments to the enum and/or linking out to docs.

We try to avoid linking out to docs since those links are rarely stable (we do sometimes though). But we do encourage using the general docs which are meant to expound on the purpose of these options more than we can reasonably do in a non-stale way in every SDK. Want to take a stab at providing suggested documentation here?

I don't think nearly as many things are self-documenting as engineers think are self-documenting. Pretend like you're newish to temporal workflows and reading the comments about update. What questions would you have? What gotchas would you like to be made aware of?

I think you go too fast for me to keep up with you and write comments, so maybe better for you to suggest and I'll take a look.

My suggestion is what's already in this PR, heh, because it's consistent with the rest of the SDK. This has been sitting for many days already so I'm guessing there's no rush. The request is a bit too subjective, so If you can provide a suggestion then we'll know it's correct instead of guess-and-check.

Per off-PR discussion, added link to https://docs.temporal.io/workflows#update

drewhoskins-temporal · 2024-05-09T00:47:11Z

temporalio/client.py

+
+
+class WorkflowUpdateWaitStage(IntEnum):
+    """Stage to wait for workflow update to reach before returning from


Another comment that mostly restates the name of the enum.

This is commonly the case with all forced-documentation docs, which includes most of the Python docs. We can't rely on self-documenting names. Similar thing exists in Go. If there is a general different posture we want to take with the Python API docs, I wonder if we can do that generally. I am totally supportive of such a general posture change, just not necessarily for this one enum in this one PR.

dandavison · 2024-05-09T14:17:02Z

temporalio/client.py

@@ -1858,8 +1859,8 @@ async def execute_update(
            update,
            arg,
            args=args,
+            wait_for_stage=WorkflowUpdateWaitStage.COMPLETED,


Just want to double-check we're sure WorkflowUpdateWaitStage is the right name for the enum here. The underlying API entity is called "Lifecycle Stage". Calling it "Wait Stage" bakes the concept of waiting into it -- is there a chance that that will be annoying in the future because we find ourselves wanting to refer to a lifecycle stage without any notion of "waiting" being involved?

👍 I have no strong opinion here and I now that I think about it, I like WorkflowUpdateLifecycleStage because, when we someday get a describe() call on WorkflowUpdateHandle, we'll want this stage to be there too.

I'll change this enum name to WorkflowUpdateLifecycleStage (but keep the parameter name as wait_for_stage) assuming there's no objection. Anyone else have an opinion on what to name this?

Yeah we should consider the future describe() call, which we can implement today, so calling the enum WorkflowUpdateLifecycleStage makes a lot more sense.

Updated to WorkflowUpdateLifecycleStage

Seems legit to me. Or just leave out Lifecycle, not sure it's carrying much weight here. Up to y'all.

+1 Lifecycle isn't needed -- WorkflowUpdateStage is sufficient.

Works for me, @Quinn-With-Two-Ns thoughts?

Enum name updated

Sushisource · 2024-05-20T18:27:09Z

temporalio/client.py

+        if wait_for_stage == WorkflowUpdateStage.ADMITTED:
+            raise ValueError("ADMITTED wait stage not supported")


Imo probably just let the server deal with this so we don't need a release to remove it later

I think we want to revisit because I am hoping this required parameter becomes optional and durability + admitted becomes the default (like it is with signals). And we need to update the docs.

cretz added 2 commits May 7, 2024 16:23

Make default ID if not provided to update

e6efd5c

Fixes temporalio#484

Required wait update stage, properly poll update until reached, and o…

b19370e

…ther update changes Fixes temporalio#424 Fixes temporalio#485 Fixes temporalio#514

cretz requested a review from a team as a code owner May 8, 2024 20:38

drewhoskins-temporal reviewed May 9, 2024

View reviewed changes

dandavison reviewed May 9, 2024

View reviewed changes

cretz added 2 commits May 10, 2024 14:46

Rename wait stage and update features branch target

1a88641

Change enum name to WorkflowUpdateStage and minor doc update

1a2acd5

Quinn-With-Two-Ns approved these changes May 15, 2024

View reviewed changes

Sushisource approved these changes May 20, 2024

View reviewed changes

dandavison approved these changes May 20, 2024

View reviewed changes

Merge branch 'main' into update-updates

1b2d411

cretz merged commit 11a97d1 into temporalio:main May 20, 2024
12 checks passed

cretz deleted the update-updates branch May 20, 2024 20:23

cretz mentioned this pull request May 21, 2024

Required wait update stage and polling improvements temporalio/sdk-dotnet#251

Merged

cretz mentioned this pull request May 28, 2024

adds test example for hello_update.py temporalio/samples-python#118

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Required wait update stage, update polling improvements, and other update changes #521

Required wait update stage, update polling improvements, and other update changes #521

cretz commented May 8, 2024 •

edited

Loading

drewhoskins-temporal May 9, 2024

drewhoskins-temporal May 9, 2024

cretz May 9, 2024 •

edited

Loading

drewhoskins-temporal May 13, 2024 •

edited

Loading

cretz May 13, 2024

cretz May 15, 2024

drewhoskins-temporal May 9, 2024

cretz May 9, 2024

dandavison May 9, 2024

cretz May 9, 2024 •

edited

Loading

Quinn-With-Two-Ns May 10, 2024

cretz May 10, 2024

drewhoskins-temporal May 13, 2024

dandavison May 13, 2024

cretz May 13, 2024

cretz May 15, 2024

Sushisource May 20, 2024

cretz May 20, 2024 •

edited

Loading



		class WorkflowUpdateWaitStage(IntEnum):
		"""Stage to wait for workflow update to reach before returning from

		if wait_for_stage == WorkflowUpdateStage.ADMITTED:
		raise ValueError("ADMITTED wait stage not supported")

Required wait update stage, update polling improvements, and other update changes #521

Required wait update stage, update polling improvements, and other update changes #521

Conversation

cretz commented May 8, 2024 • edited Loading

What was changed

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz May 9, 2024 • edited Loading

Choose a reason for hiding this comment

drewhoskins-temporal May 13, 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

cretz May 9, 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

cretz May 20, 2024 • edited Loading

Choose a reason for hiding this comment

cretz commented May 8, 2024 •

edited

Loading

cretz May 9, 2024 •

edited

Loading

drewhoskins-temporal May 13, 2024 •

edited

Loading

cretz May 9, 2024 •

edited

Loading

cretz May 20, 2024 •

edited

Loading