-
Notifications
You must be signed in to change notification settings - Fork 104
Nexus: worker, workflow-backed operations, and workflow caller #813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
edfa0e5
768d93a
143b510
cce402a
eb0a93e
e47edfe
585353e
d723c05
6da5ad5
c6c24ed
d162d61
1b79dc4
eb86854
7c1905b
5c8b4d4
91a046a
9cdf219
a78b7f2
2b8b8de
eba4273
9d72a97
f3fd617
ef6971d
a59c44e
93c112b
b1f05dc
e73271c
5207abe
03c9cff
51c3786
74640bc
25c8c19
b333023
7c1ad67
d45a7a9
5016561
9021665
b49a60a
d1fb476
9c41058
f7cd556
877248f
857684d
380bf1c
300fc2e
1d0425b
ae82f0c
6431f39
9dfd799
1c07502
3298358
dcdeb1a
688e6c5
ffc2369
daf9ba8
96fe21e
fe988be
e746462
081645c
0000f4d
3914524
cf86778
886f5ed
4403972
3e49329
d69a7cb
29101e7
45a841f
1af01d3
1319732
320e1f1
f8077c5
f74784a
8e0af0a
d9bef4e
6836d4b
2a4b7cf
d45317e
740a822
64e4b01
f0dba64
8c76a6f
97272ed
5ff174c
8f4b524
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ keywords = [ | |
"workflow", | ||
] | ||
dependencies = [ | ||
"nexus-rpc", | ||
"protobuf>=3.20,<6", | ||
"python-dateutil>=2.8.2,<3 ; python_version < '3.11'", | ||
"types-protobuf>=3.20", | ||
|
@@ -41,7 +42,7 @@ dev = [ | |
"psutil>=5.9.3,<6", | ||
"pydocstyle>=6.3.0,<7", | ||
"pydoctor>=24.11.1,<25", | ||
"pyright==1.1.377", | ||
"pyright==1.1.400", | ||
"pytest~=7.4", | ||
"pytest-asyncio>=0.21,<0.22", | ||
"pytest-timeout~=2.2", | ||
|
@@ -50,6 +51,8 @@ dev = [ | |
"twine>=4.0.1,<5", | ||
"ruff>=0.5.0,<0.6", | ||
"maturin>=1.8.2", | ||
"pytest-cov>=6.1.1", | ||
"httpx>=0.28.1", | ||
"pytest-pretty>=1.3.0", | ||
] | ||
|
||
|
@@ -159,6 +162,7 @@ exclude = [ | |
"tests/worker/workflow_sandbox/testmodules/proto", | ||
"temporalio/bridge/worker.py", | ||
"temporalio/contrib/opentelemetry.py", | ||
"temporalio/contrib/pydantic.py", | ||
"temporalio/converter.py", | ||
"temporalio/testing/_workflow.py", | ||
"temporalio/worker/_activity.py", | ||
|
@@ -170,6 +174,10 @@ exclude = [ | |
"tests/api/test_grpc_stub.py", | ||
"tests/conftest.py", | ||
"tests/contrib/test_opentelemetry.py", | ||
"tests/contrib/pydantic/models.py", | ||
"tests/contrib/pydantic/models_2.py", | ||
"tests/contrib/pydantic/test_pydantic.py", | ||
"tests/contrib/pydantic/workflows.py", | ||
"tests/test_converter.py", | ||
"tests/test_service.py", | ||
"tests/test_workflow.py", | ||
|
@@ -204,3 +212,6 @@ exclude = [ | |
[tool.uv] | ||
# Prevent uv commands from building the package by default | ||
package = false | ||
|
||
[tool.uv.sources] | ||
nexus-rpc = { path = "../nexus-sdk-python", editable = true } | ||
Comment on lines
+216
to
+217
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's make sure not to merge until this is proper dependency |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -464,9 +464,15 @@ async def start_workflow( | |||||
rpc_metadata: Mapping[str, str] = {}, | ||||||
rpc_timeout: Optional[timedelta] = None, | ||||||
request_eager_start: bool = False, | ||||||
stack_level: int = 2, | ||||||
priority: temporalio.common.Priority = temporalio.common.Priority.default, | ||||||
versioning_override: Optional[temporalio.common.VersioningOverride] = None, | ||||||
# The following options are deliberately not exposed in overloads | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
That way we can change these up as we need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Taken There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unresolved (but pedantic, don't have to take if not wanted, I am just afraid of users using them) |
||||||
nexus_completion_callbacks: Sequence[NexusCompletionCallback] = [], | ||||||
workflow_event_links: Sequence[ | ||||||
temporalio.api.common.v1.Link.WorkflowEvent | ||||||
] = [], | ||||||
request_id: Optional[str] = None, | ||||||
stack_level: int = 2, | ||||||
) -> WorkflowHandle[Any, Any]: | ||||||
"""Start a workflow and return its handle. | ||||||
|
||||||
|
@@ -529,7 +535,6 @@ async def start_workflow( | |||||
name, result_type_from_type_hint = ( | ||||||
temporalio.workflow._Definition.get_name_and_result_type(workflow) | ||||||
) | ||||||
|
||||||
return await self._impl.start_workflow( | ||||||
StartWorkflowInput( | ||||||
workflow=name, | ||||||
|
@@ -557,6 +562,9 @@ async def start_workflow( | |||||
rpc_timeout=rpc_timeout, | ||||||
request_eager_start=request_eager_start, | ||||||
priority=priority, | ||||||
nexus_completion_callbacks=nexus_completion_callbacks, | ||||||
workflow_event_links=workflow_event_links, | ||||||
request_id=request_id, | ||||||
) | ||||||
) | ||||||
|
||||||
|
@@ -5193,6 +5201,9 @@ class StartWorkflowInput: | |||||
rpc_timeout: Optional[timedelta] | ||||||
request_eager_start: bool | ||||||
priority: temporalio.common.Priority | ||||||
nexus_completion_callbacks: Sequence[NexusCompletionCallback] | ||||||
workflow_event_links: Sequence[temporalio.api.common.v1.Link.WorkflowEvent] | ||||||
request_id: Optional[str] | ||||||
Comment on lines
+5204
to
+5206
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May want to mention here these are unstable/experimental |
||||||
versioning_override: Optional[temporalio.common.VersioningOverride] = None | ||||||
|
||||||
|
||||||
|
@@ -5807,8 +5818,26 @@ async def _build_start_workflow_execution_request( | |||||
self, input: StartWorkflowInput | ||||||
) -> temporalio.api.workflowservice.v1.StartWorkflowExecutionRequest: | ||||||
req = temporalio.api.workflowservice.v1.StartWorkflowExecutionRequest() | ||||||
req.request_eager_execution = input.request_eager_start | ||||||
await self._populate_start_workflow_execution_request(req, input) | ||||||
# _populate_start_workflow_execution_request is used for both StartWorkflowInput | ||||||
# and UpdateWithStartStartWorkflowInput. UpdateWithStartStartWorkflowInput does | ||||||
# not have the following two fields so they are handled here. | ||||||
req.request_eager_execution = input.request_eager_start | ||||||
if input.request_id: | ||||||
req.request_id = input.request_id | ||||||
|
||||||
req.completion_callbacks.extend( | ||||||
temporalio.api.common.v1.Callback( | ||||||
nexus=temporalio.api.common.v1.Callback.Nexus( | ||||||
url=callback.url, header=callback.header | ||||||
) | ||||||
) | ||||||
for callback in input.nexus_completion_callbacks | ||||||
) | ||||||
req.links.extend( | ||||||
temporalio.api.common.v1.Link(workflow_event=link) | ||||||
for link in input.workflow_event_links | ||||||
) | ||||||
return req | ||||||
|
||||||
async def _build_signal_with_start_workflow_execution_request( | ||||||
|
@@ -7231,6 +7260,17 @@ def api_key(self, value: Optional[str]) -> None: | |||||
self.service_client.update_api_key(value) | ||||||
|
||||||
|
||||||
@dataclass(frozen=True) | ||||||
class NexusCompletionCallback: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May want to mention this is unstable/experimental and also not really for user use (I understand exposing because it's exposed in the interceptor) |
||||||
"""Nexus callback to attach to events such as workflow completion.""" | ||||||
|
||||||
url: str | ||||||
"""Callback URL.""" | ||||||
|
||||||
header: Mapping[str, str] | ||||||
"""Header to attach to callback request.""" | ||||||
|
||||||
|
||||||
async def _encode_user_metadata( | ||||||
converter: temporalio.converter.DataConverter, | ||||||
summary: Optional[Union[str, temporalio.api.common.v1.Payload]], | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -911,6 +911,12 @@ def _error_to_failure( | |
failure.child_workflow_execution_failure_info.retry_state = ( | ||
temporalio.api.enums.v1.RetryState.ValueType(error.retry_state or 0) | ||
) | ||
# TODO(nexus-prerelease): test coverage for this | ||
elif isinstance(error, temporalio.exceptions.NexusOperationError): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For symmetry reasons, I suspect we also need to convert There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need to add test coverage per the comment, and for what you're saying. Maybe this is something to resolve when we merge the workflow caller. |
||
failure.nexus_operation_execution_failure_info.SetInParent() | ||
failure.nexus_operation_execution_failure_info.operation_token = ( | ||
error.operation_token | ||
) | ||
|
||
def from_failure( | ||
self, | ||
|
@@ -1006,6 +1012,26 @@ def from_failure( | |
if child_info.retry_state | ||
else None, | ||
) | ||
elif failure.HasField("nexus_handler_failure_info"): | ||
nexus_handler_failure_info = failure.nexus_handler_failure_info | ||
err = temporalio.exceptions.NexusHandlerError( | ||
failure.message or "Nexus handler error", | ||
type=nexus_handler_failure_info.type, | ||
retryable={ | ||
temporalio.api.enums.v1.NexusHandlerErrorRetryBehavior.NEXUS_HANDLER_ERROR_RETRY_BEHAVIOR_RETRYABLE: True, | ||
temporalio.api.enums.v1.NexusHandlerErrorRetryBehavior.NEXUS_HANDLER_ERROR_RETRY_BEHAVIOR_NON_RETRYABLE: False, | ||
}.get(nexus_handler_failure_info.retry_behavior), | ||
) | ||
elif failure.HasField("nexus_operation_execution_failure_info"): | ||
nexus_op_failure_info = failure.nexus_operation_execution_failure_info | ||
err = temporalio.exceptions.NexusOperationError( | ||
failure.message or "Nexus operation error", | ||
scheduled_event_id=nexus_op_failure_info.scheduled_event_id, | ||
endpoint=nexus_op_failure_info.endpoint, | ||
service=nexus_op_failure_info.service, | ||
operation=nexus_op_failure_info.operation, | ||
operation_token=nexus_op_failure_info.operation_token, | ||
) | ||
else: | ||
err = temporalio.exceptions.FailureError(failure.message or "Failure error") | ||
err._failure = failure | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -362,6 +362,69 @@ def retry_state(self) -> Optional[RetryState]: | |
return self._retry_state | ||
|
||
|
||
class NexusHandlerError(FailureError): | ||
"""Error raised on Nexus handler failure.""" | ||
|
||
def __init__( | ||
self, | ||
message: str, | ||
*, | ||
type: str, | ||
retryable: Optional[bool] = None, | ||
): | ||
"""Initialize a Nexus handler error.""" | ||
super().__init__(message) | ||
self._type = type | ||
self._retryable = retryable | ||
Comment on lines
+377
to
+378
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Users of this exception should be allowed to see these properties/attributes IMO, even if we don't think they ever will There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's resolve this when we review the workflow caller. [In fact, let's consider using the same PR for that. Just for now, let's review handler/worker-relevant stuff only] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the type be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO no it should not be an enum if it was chosen to not be in the API |
||
|
||
|
||
class NexusOperationError(FailureError): | ||
"""Error raised on Nexus operation failure.""" | ||
|
||
def __init__( | ||
self, | ||
message: str, | ||
*, | ||
scheduled_event_id: int, | ||
endpoint: str, | ||
service: str, | ||
operation: str, | ||
operation_token: str, | ||
): | ||
"""Initialize a Nexus operation error.""" | ||
super().__init__(message) | ||
self._scheduled_event_id = scheduled_event_id | ||
self._endpoint = endpoint | ||
self._service = service | ||
self._operation = operation | ||
self._operation_token = operation_token | ||
|
||
@property | ||
def scheduled_event_id(self) -> int: | ||
"""The NexusOperationScheduled event ID for the failed operation.""" | ||
return self._scheduled_event_id | ||
|
||
@property | ||
def endpoint(self) -> str: | ||
"""The endpoint name for the failed operation.""" | ||
return self._endpoint | ||
|
||
@property | ||
def service(self) -> str: | ||
"""The service name for the failed operation.""" | ||
return self._service | ||
|
||
@property | ||
def operation(self) -> str: | ||
"""The name of the failed operation.""" | ||
return self._operation | ||
|
||
@property | ||
def operation_token(self) -> str: | ||
"""The operation token returned by the failed operation.""" | ||
return self._operation_token | ||
|
||
|
||
def is_cancelled_exception(exception: BaseException) -> bool: | ||
"""Check whether the given exception is considered a cancellation exception | ||
according to Temporal. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see this section