Enable Eager Workflow Start #430
Conversation
|
|
temporalio/client.py
Outdated
| eager_start: Reduce the latency to start this workflow. | ||
| It requires that the client is shared between the worker and the starter. | ||
| This is currently experimental. |
There was a problem hiding this comment.
| eager_start: Reduce the latency to start this workflow. | |
| It requires that the client is shared between the worker and the starter. | |
| This is currently experimental. | |
| eager_start: Potentially reduce the latency to start this workflow by | |
| encouraging the server to start it on a local worker running with | |
| this same client. | |
| This is currently experimental. |
Pedantic, don't have to change
There was a problem hiding this comment.
Much clear, thanks!
temporalio/client.py
Outdated
| @@ -1074,6 +1089,7 @@ def __init__( | |||
| result_run_id: Optional[str] = None, | |||
| first_execution_run_id: Optional[str] = None, | |||
| result_type: Optional[Type] = None, | |||
| eager_start_active: bool = False, | |||
There was a problem hiding this comment.
Hrmm, I'm not sure this should be on the handle. Usually the only things on a handle are things that have actual usage value for the handle regardless of whether it was created via start or whether the handle is created via get_workflow_handle or whether this constructor is called manually. This has only read value and only in one case of handle creation.
We don't tell a user on their activity handle that their activity was eagerly started either and that has seemed ok to everyone. Go and Java don't show this anywhere I can see and we're ok with that there. I think we can use a metric or something else to tell a user (and a test) that workflows were started eagerly. I think we can punt on reporting this to users via the handle at first and maybe revisit when a user requests it.
If we do decide to expose this, I think it should not be on the constructor (i.e. late bound directly on the private attribute) and changed to "eagerly started". So:
- Remove this constructor param
- Change init code to be
self._eagerly_started = False - Change property name to be
def eagerly_started(self) -> bool:and remove docstring about cannot be mutated (implied for read-only property) - Set the attribute post-construction in the workflow starter code
I could also be convinced of "started eagerly" instead of "eagerly started" though I like the latter because it matches the parameter name from start/execute.
But still, I am not sure that Python deserves to expose this any more than Go or Java do.
There was a problem hiding this comment.
eagerly_started is much better. I'll change it not to be in the constructor. It is hard to add this in other way, we only have server-side metrics for eager, and that's difficult to synchronize... And I think that once customers start enabling eager, the first question is "is it working?" and we want to provide a simple way to check that. I'll talk with @Quinn-With-Two-Ns to expose this in Java and Go too...
There was a problem hiding this comment.
👍 I would like to have the discussion of how we may expose in Go and Java before exposing in Python. If we find our two most used SDKs haven't/don't need to expose that they started eagerly besides metrics, I would rather not in Python.
There was a problem hiding this comment.
Removing "eagerly started" from handle to be consistent with other SDKs
temporalio/client.py
Outdated
| @@ -287,6 +287,7 @@ async def start_workflow( | |||
| start_signal_args: Sequence[Any] = [], | |||
| rpc_metadata: Mapping[str, str] = {}, | |||
| rpc_timeout: Optional[timedelta] = None, | |||
| eager_start: bool = False, | |||
There was a problem hiding this comment.
| eager_start: bool = False, | |
| request_eager_start: bool = False, |
I wonder if this is a bit clearer (or "enable eager start" or "suggest eager start" or something that doesn't imply any guarantees)
There was a problem hiding this comment.
Good point about not implying guarantees, changing it...
|
Looks good! Had to merge main into your branch, will merge into main if/when CI succeeds. |
|
Thanks! |
Enables Eager Workflow Start for Python SDK after being enabled in Core. This is an experimental feature that
reduces the latency to start a workflow by dispatching the first task directly to a worker that shares the client with the initiator.
Checklist
[Feature Request] Eager workflow start #388