Workflow/Activity Priorities #802

Sushisource · 2025-03-26T23:27:49Z

What was changed

Updated Core
Expose priority setting on workflow client & in child/activities launched from workflows

Why?

Priorities are cool!

Checklist

Closes [Feature Request] Support Priority Annotations #760
How was this tested:
Added integ test
Any docs updates needed?

dandavison · 2025-03-27T00:02:11Z

tests/worker/test_workflow.py

+
+@activity.defn
+async def check_priority_activity(should_have_priorty: int) -> str:
+    assert activity.info().priority.priority_key == should_have_priorty


nit: "priorty"

dandavison · 2025-03-27T00:02:18Z

scripts/_proto/Dockerfile

@@ -9,6 +10,7 @@ COPY ./ ./
 RUN mkdir -p ./temporalio/api
 RUN uv sync --all-extras
 RUN uv add "protobuf<4"
+RUN uv sync --all-extras


We can delete line 11 right?

~~Probably~~ Yes

cretz · 2025-03-27T12:06:26Z

temporalio/client.py

@@ -456,6 +460,7 @@ async def start_workflow(
        rpc_timeout: Optional[timedelta] = None,
        request_eager_start: bool = False,
        stack_level: int = 2,
+        priority: Optional[temporalio.common.Priority] = None,


Need to apply this to all execute_workflow calls too

cretz · 2025-03-27T12:09:39Z

temporalio/client.py

@@ -456,6 +460,7 @@ async def start_workflow(
        rpc_timeout: Optional[timedelta] = None,
        request_eager_start: bool = False,
        stack_level: int = 2,
+        priority: Optional[temporalio.common.Priority] = None,


Also probably need this on ScheduleActionStartWorkflow unless we're intentionally leaving off that for now

cretz · 2025-03-27T12:10:57Z

temporalio/common.py

+    (more will be added here later)
+    """
+
+    priority_key: int = 0


I don't think this field should have a default value at this time, meaning the idea of creating Priority() without any params doesn't make sense at this time (but we can make it default-able later as more fields come about).

Also, even though proto and Go don't have the ability to model optional, we do, so we can treat zero as Optional here which may make sense. Of course we can still accept 0 or None to mean None, but when relaying to a user, I think None makes more sense.

Sure, I can agree with that

cretz · 2025-03-27T12:11:29Z

temporalio/activity.py

@@ -116,6 +116,7 @@ class Info:
    workflow_namespace: str
    workflow_run_id: str
    workflow_type: str
+    priority: temporalio.common.Priority


Is priority always non-null in proto for activity task and workflow start event?

No, it's not, but we decided that just means default. Not personally a huge fan of that, but, the information conveyed is the same temporalio/sdk-java#2453 (comment)

Works for me. When it comes to us passing it in, does server care whether we send a null priority or a non-null one with default values? The reason I ask about that direction is because if we want always output to be non-Optional, we might as well never accept Optional input and just default to a fixed immutable Priority.default. So there's no such thing as Optional[Priority] anywhere (kinda touched on in another comment).

cretz · 2025-03-27T12:13:02Z

temporalio/common.py

+    inherit Priority from the workflow that created them, but may override fields when they are
+    started or modified. For each field of a Priority on an activity/workflow, not present or equal
+    to zero/empty string means to inherit the value from the calling workflow, or if there is no
+    calling workflow, then use the default (documented below).


Suggested change

calling workflow, then use the default (documented below).

calling workflow, then use the default (documented on the field).

cretz · 2025-03-27T12:14:30Z

temporalio/common.py

+    """
+
+    @staticmethod
+    def from_proto(proto: temporalio.api.common.v1.Priority) -> Priority:


Suggested change

def from_proto(proto: temporalio.api.common.v1.Priority) -> Priority:

def _from_proto(proto: temporalio.api.common.v1.Priority) -> Priority:

If _to_proto is private, this should be too IMO (I know some structures like RetryPolicy we chose to expose both, but I like the idea of not exposing until needed)

cretz · 2025-03-27T12:21:57Z

temporalio/client.py

@@ -456,6 +460,7 @@ async def start_workflow(
        rpc_timeout: Optional[timedelta] = None,
        request_eager_start: bool = False,
        stack_level: int = 2,
+        priority: Optional[temporalio.common.Priority] = None,


Suggested change

priority: Optional[temporalio.common.Priority] = None,

priority: temporalio.common.Priority = temporalio.common.Priority.default,

If priority is never null when returned from the server, arguably it could be more idiomatic to do this from a user POV (i.e. make a default class var on the Priority with the default form)

I dunno, I like the explicit nature of either providing it or not. Plus just on a practical level I think it makes the code that is accepting it simpler to just check None and not set it at all.

I like the explicit nature of either providing it or not

Except when it comes to accessing it as a user, where we don't like that. So we say interceptor outbound input has this optional but interceptor inbound does not which loses symmetry a bit I think. Isn't it true that Priority() and None effectively mean the same thing?

But this is almost exactly contradictory with your suggestion to make the key itself optional. Now, either way, the user needs to see if that was set or not. Having it be optional here also means they have the most obvious way to determine if they explicitly set it, or not. The asymmetry is kind of a good thing I think, because on outbound they get to choose, but on inbound they don't, so it expresses that.

But this is almost exactly contradictory with your suggestion to make the key itself optional

Not really. I support bidirectional optional-ality if you will. Basically if it is optional when they provide it it should be optional when they read it. Basically when we can (which is not always with server-side/calculated defaults), we either use the language construct to represent default-as-unset or we don't, but half of the time default being an empty object and the other half of the time it being None was a bit confusing to me while reading. The user only uses this parameter when they are changing the parameter, so the default is in effect more of a documentation thing (though there is the interceptor component). It is confusing that I can't expect the default on output be the default on input.

Having said that, my opinion is not strong here nor is it blocking, but I think it may affect a couple of other languages that can have smart parameter defaults like .NET and Ruby.

cretz · 2025-03-27T12:24:54Z

temporalio/workflow.py

@@ -1878,6 +1883,7 @@ def start_activity(
    cancellation_type: ActivityCancellationType = ActivityCancellationType.TRY_CANCEL,
    activity_id: Optional[str] = None,
    versioning_intent: Optional[VersioningIntent] = None,
+    priority: Optional[temporalio.common.Priority] = None,


Don't forget start_activity_class, execute_activity_class, start_activity_method, and execute_activity_method

Done. There's a lot of those!

cretz

Nothing blocking, though would like default input to be same as default output if possible from a logic POV, but non-blocking.

dandavison · 2025-03-28T15:43:19Z

temporalio/client.py

@@ -711,6 +722,7 @@ async def execute_workflow(
        rpc_metadata: Mapping[str, str] = {},
        rpc_timeout: Optional[timedelta] = None,
        request_eager_start: bool = False,
+        priority: Optional[temporalio.common.Priority] = None,


execute_workflow isn't passing the new parameter on to start_workflow.

dandavison · 2025-03-28T15:46:00Z

temporalio/client.py

@@ -2439,6 +2451,7 @@ def __init__(
        start_delay: Optional[timedelta] = None,
        rpc_metadata: Mapping[str, str] = {},
        rpc_timeout: Optional[timedelta] = None,
+        priority: Optional[temporalio.common.Priority] = None,
    ) -> None: ...



Missed an overload (l. 2484)

dandavison · 2025-03-28T15:53:16Z

temporalio/workflow.py

@@ -3965,6 +4025,7 @@ async def start_child_workflow(
            UI/CLI. This can be in Temporal markdown format and can span multiple lines. This is
            a fixed value on the workflow that cannot be updated. For details that can be
            updated, use :py:meth:`Workflow.get_current_details` within the workflow.
+        priority: Priority to use for this workflow.


It's not passing the parameter on to workflow_start_child_workflow here

dandavison · 2025-03-28T16:07:21Z

temporalio/common.py

+
+    The default priority is (min+max)/2. With the default max of 5 and min of 1, that comes out to
+    3.
+    """


I think it would be useful to users to validate in Python, rather than waiting for an error from the server. Especially since they may think they can use floats, or 0, etc. Since this is a frozen data class it'll be sufficient to validate in __post_init__, e.g.

def __post_init__(self): if self.priority_key is not None: if not isinstance(self.priority_key, int): raise TypeError("Priority key must be an integer") if self.priority_key < 1: raise ValueError("Priority key must be a positive integer")

Yeah that's cheap and easy, I will add

Sushisource added 8 commits March 25, 2025 17:13

Update Core

10082a7

Add download TTL field to ephem server configs

80a9e4f

Update protos

386dad7

Handle proto updates / add new RPCs

6dca0e6

Pass through priorities

ac33dbd

Use specific CLI version

050562d

Cleanup

11f83e5

Update core/protos again to get priorities on tasks

e4bbd54

Sushisource requested a review from a team as a code owner March 26, 2025 23:27

Attach everywhere & add test

43b9aed

Sushisource force-pushed the priority branch from 0119147 to 43b9aed Compare March 26, 2025 23:32

dandavison reviewed Mar 27, 2025

View reviewed changes

Fix Rust lint & remove unneeded docker line

0b29b9c

cretz reviewed Mar 27, 2025

View reviewed changes

Sushisource added 2 commits March 27, 2025 09:24

Pass through in missed methods

78e3089

Use optional priority key

96730dc

cretz approved these changes Mar 28, 2025

View reviewed changes

cretz mentioned this pull request Mar 28, 2025

Expose root workflow execution #805

Merged

dandavison reviewed Mar 28, 2025

View reviewed changes

Sushisource added 3 commits March 28, 2025 09:08

Fix more overloads, thanks Dan

6e6b45d

Change to nonoptional Priority

ced8749

Add some simple validation

9060804

Sushisource enabled auto-merge (squash) March 28, 2025 18:20

Merge branch 'main' into priority

ee03543

dandavison approved these changes Mar 28, 2025

View reviewed changes

Sushisource merged commit bf747f1 into main Mar 28, 2025
14 checks passed

Sushisource deleted the priority branch March 28, 2025 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workflow/Activity Priorities #802

Workflow/Activity Priorities #802

Sushisource commented Mar 26, 2025

dandavison Mar 27, 2025

dandavison Mar 27, 2025

Sushisource Mar 27, 2025 •

edited

Loading

cretz Mar 27, 2025

cretz Mar 27, 2025

cretz Mar 27, 2025 •

edited

Loading

Sushisource Mar 27, 2025

cretz Mar 27, 2025

Sushisource Mar 27, 2025

cretz Mar 27, 2025

cretz Mar 27, 2025

cretz Mar 27, 2025

cretz Mar 27, 2025 •

edited

Loading

Sushisource Mar 27, 2025

cretz Mar 27, 2025

Sushisource Mar 27, 2025

cretz Mar 28, 2025 •

edited

Loading

cretz Mar 27, 2025

Sushisource Mar 27, 2025

cretz left a comment

dandavison Mar 28, 2025

Sushisource Mar 28, 2025

dandavison Mar 28, 2025

dandavison Mar 28, 2025

dandavison Mar 28, 2025

Sushisource Mar 28, 2025

	calling workflow, then use the default (documented below).
	calling workflow, then use the default (documented on the field).

	def from_proto(proto: temporalio.api.common.v1.Priority) -> Priority:
	def _from_proto(proto: temporalio.api.common.v1.Priority) -> Priority:

	priority: Optional[temporalio.common.Priority] = None,
	priority: temporalio.common.Priority = temporalio.common.Priority.default,

Workflow/Activity Priorities #802

Workflow/Activity Priorities #802

Conversation

Sushisource commented Mar 26, 2025

What was changed

Why?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushisource Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Mar 27, 2025 • 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

cretz Mar 27, 2025 • 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

cretz Mar 28, 2025 • edited Loading

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

Sushisource Mar 27, 2025 •

edited

Loading

cretz Mar 27, 2025 •

edited

Loading

cretz Mar 27, 2025 •

edited

Loading

cretz Mar 28, 2025 •

edited

Loading