Add Temporal Operation Handler#1503
Conversation
26c52b8 to
b0dd232
Compare
| for index, (param_type, expected_param_type) in enumerate( | ||
| zip(param_types, expected_param_types), start=1 | ||
| ): | ||
| if not _is_subclass(expected_param_type, param_type): |
There was a problem hiding this comment.
I've swapped expected_param_type and param_type in this check here. Parameters should be contravariant (e.g. you could provide a function that accepts nexusrpc.StartOperationContext) but the previous direction was covariance (e.g. you could provide a function that accepts a child of WorkflowRunOperationContext).
In the covariant case, the type provided at runtime will never fulfill the decorated function's requirement of the more specific type. Type checkers already prevent this, but bug was that the warning below would not trigger at runtime so users that don't type check would not receive the warning.
In the contravariant case, the type provided at runtime will fulfill the decorated function's requirement of the less specific type.
The new tests in test_handler_operation_definitions.py demonstrate both paths and assert that the warning is or is not delivered as appropriate.
Rename the Temporal operation start context to TemporalNexusStartOperationContext and add TemporalNexusCancelOperationContext with access to the worker client. Make TemporalNexusOperationHandler a public abstract base with overrideable start_operation and cancel_workflow_run hooks, while keeping the decorator-backed implementation private. Update type annotations, exports, and tests, including coverage for custom Temporal operation cancellation.
57af53b to
eb3c560
Compare
| class CustomCancelNexusOpHandler( | ||
| nexus.TemporalNexusOperationHandler[str, None] | ||
| ): | ||
| @override | ||
| async def start_operation( | ||
| self, | ||
| ctx: nexus.TemporalNexusStartOperationContext, | ||
| client: nexus.TemporalNexusClient, | ||
| input: str, | ||
| ) -> nexus.TemporalOperationResult[None]: | ||
| result = await client.start_workflow(BlockingWorkflow.run, id=input) | ||
| event.set() | ||
| return result | ||
|
|
||
| @override | ||
| async def cancel_workflow_run( | ||
| self, ctx: nexus.TemporalNexusCancelOperationContext, workflow_id: str | ||
| ): | ||
| # get a handle to the workflow | ||
| handle = ctx.client.get_workflow_handle(workflow_id) | ||
|
|
||
| # cancel the workflow | ||
| await handle.cancel() | ||
|
|
||
| return CustomCancelNexusOpHandler() |
There was a problem hiding this comment.
Added this test to demonstrate how users can customize cancellation.
| await client_workflow_handle.cancel(**kwargs) | ||
|
|
||
|
|
||
| class TemporalNexusOperationHandler(OperationHandler[InputT, OutputT], ABC): |
There was a problem hiding this comment.
Base class is ABC to allow customization of cancellation and require implementors to override start_operation. The decorator uses the concrete internal implementation _TemporalNexusOperationHandler to invoke the decorated method.
… to prevent users from instantiating.
| namespace = token_details.get("ns") | ||
| if not isinstance(namespace, str) or not namespace: | ||
| raise TypeError( | ||
| f"invalid token: expected namespace to be a non-empty string, got {type(namespace)}" |
There was a problem hiding this comment.
If namespace is an empty string, this is going to be a weird error message - it'll say "expected namespace to be non-empty string, got string".
There is also a comment above in token.py saying "Allow empty string for ns" - is it allowed then? Seems like that comment or this check should change? (I noticed that this time after a prior comment from the last code review!)
There was a problem hiding this comment.
Good call on the error message, I didn't quite realize that! Also good call on the conflicting behavior. I got overzealous with validation here. I've changed the namespace logic to reflect what was previously supported so that should clear up both issues!
| """ | ||
|
|
||
| value: _ResultT | object = temporalio.common._arg_unset | ||
| token: str | None = None |
There was a problem hiding this comment.
Just out of pure paranoia ... should you add a check to make sure no one calls TemporalOperationResult(token=None)? (or async_token with a None value?) It looks like that will work, then will fail with a runtime error below in _to_nexus_result if I read this right. Either way, seems worth validating the inputs in the constructor.
Also you have a constructor and then it looks like the desire is that the two class methods call the constructor? In Python, can you hide the constructor from the user to avoid confusion? If we don't want them to call it, then it's nice if it's not there to be called. But that is very much just a nitpick.
There was a problem hiding this comment.
You can't truly hide a constructor in Python. You can do some kinda gross things to do it, but IMO it's not worth it.
I have added a __post_init__ implementation to validate that it's been constructed in an expected state to help out with that. Now regardless of how it is constructed, it will assert that it has exactly one of value & token, and if it has a token that it's a non-empty string.
| return self._temporal_context.client | ||
|
|
||
|
|
||
| class TemporalNexusCancelOperationContext(CancelOperationContext): |
There was a problem hiding this comment.
I think these can just be aliases.
There was a problem hiding this comment.
I swapped back to classes for those contexts after doing some experimentation. The reasons were largely around runtime type checking and printing the type of them that leaked the nexusrpc type rather than using the named alias.
The extra functionality that was just exposing metric meter and client have been removed from them though to avoid duplicating access patterns.
…xt to be type aliases. Remove overly cautious _is_subclass helper. Add typedef for TemporalNexusOperationStartHandlerFunc to improve readability
…against new options
What was changed
Added support for Temporal-backed Nexus operation handlers.
This introduces
@temporalio.nexus.temporal_operation,TemporalNexusClient, andTemporalOperationResult, allowing Nexus operation handlers to either return a synchronous result or start a Temporal workflow as the async backing operation. The branch also adds token parsing support for generic operation tokens, cancellation support for Temporal-backed workflow operations, Nexus workflow client overloads for the new operation shape, and focused runtime/type coverage.Why?
This gives Nexus operation handlers a first-class way to interact with Temporal while preserving Nexus async operation semantics, workflow linking/callback behavior, cancellation handling, and typed workflow-start ergonomics.
Checklist
How was this tested:
Any docs updates needed?
Yes. The Nexus docs should cover
@temporalio.nexus.temporal_operation,TemporalNexusClient.start_workflow,TemporalOperationResult.sync, and async workflow-backed operation behavior.