Conversation
Evanthx
force-pushed
the
signals-nexus-python
branch
from
d11d1a0 to
83386cf
Compare
VegetarianOrc
left a comment
VegetarianOrc
left a comment
There was a problem hiding this comment.
Code looks okay to me! A few comments throughout.
Left a couple of comments to help me understand the motivation for the structure of the samples as well.
Would like to clear up the intention around removing the sync operation sample before approving.
| from temporalio import workflow | ||
| from temporalio.exceptions import ApplicationError | ||
|
|
||
| with workflow.unsafe.imports_passed_through(): |
There was a problem hiding this comment.
No need to use imports_passed_through() here.
There was a problem hiding this comment.
Was this same intentionally deleted? I don't think we should remove the basic sync operation example in favor of a messaging sample.
| from temporalio import workflow | ||
| from temporalio.exceptions import ApplicationError | ||
|
|
||
| with workflow.unsafe.imports_passed_through(): |
There was a problem hiding this comment.
Same as above. Shouldn't need imports_passed_through() here.
| The handler worker starts a `GreetingWorkflow` for a user ID. | ||
| `NexusGreetingServiceHandler` holds that ID and routes every Nexus operation to it. | ||
| The caller's input does not have that workflow ID as the caller doesn't know it -- but the caller | ||
| sends in the User ID, and `NexusGreetingServiceHandler` knows how to get the desired workflow ID | ||
| from that User ID (see the `get_workflow_id` call). | ||
|
|
||
| The handler worker uses the same `get_workflow_id` call to generate a workflow ID from a user ID | ||
| when it launches the workflow. |
There was a problem hiding this comment.
I'm worried this sample is a little complicated.
For example, if our goal is to demonstrate how to acquire a workflow handle in a nexus operation handler and signal/query/update a workflow I think abstracting workflow ID as a combo of prefix + userID just adds cognitive load and may be confusing.
If the goal is to show how Nexus can be used to abstract the underlying workflow away, then I think that might deserve a sample separate from how to use messaging via Nexus.
| The caller workflow: | ||
| 1. Queries for supported languages (`get_languages` -- backed by a `@workflow.query`) | ||
| 2. Changes the language to Arabic (`set_language` -- backed by a `@workflow.update` that calls an activity) | ||
| 3. Confirms the change via a second query (`get_language`) | ||
| 4. Approves the workflow (`approve` -- backed by a `@workflow.signal`) |
There was a problem hiding this comment.
Nit: The caller workflow only executes nexus operations and it feels a bit confusing to call out that they're backed by workflow.query/update/signal. IMO that info should be called out about or alongside the service handler.
| workflow_id = get_workflow_id(USER_ID) | ||
| await client.start_workflow( | ||
| GreetingWorkflow.run, | ||
| id=workflow_id, | ||
| task_queue=TASK_QUEUE, | ||
| id_conflict_policy=WorkflowIDConflictPolicy.USE_EXISTING, | ||
| ) | ||
| logging.info("Started greeting workflow: %s", workflow_id) |
There was a problem hiding this comment.
Nit: this just starts a workflow while no workers are running. I'd consider moving it in the below async with Worker(...) block
| # Routes to set_language_using_activity (not set_language) so that new languages not | ||
| # already in the greetings map can be fetched via an activity. | ||
| @nexusrpc.handler.sync_operation | ||
| async def set_language( | ||
| self, ctx: nexusrpc.handler.StartOperationContext, input: SetLanguageInput | ||
| ) -> Language: | ||
| return await self._get_workflow_handle(input.user_id).execute_update( | ||
| GreetingWorkflow.set_language_using_activity, input | ||
| ) |
There was a problem hiding this comment.
If GreetingWorkflow.set_language is unused, we should remove it IMO.
| ## On-demand pattern | ||
|
|
||
| No workflow is pre-started. The caller creates and controls workflow instances through Nexus | ||
| operations. `NexusRemoteGreetingService` adds a `run_from_remote` operation that starts a new | ||
| `GreetingWorkflow`, and every other operation includes a `user_id` so the handler knows which | ||
| instance to target. |
There was a problem hiding this comment.
Can you help me understand the motivation behind the two samples? This sample appears to demonstrate the same thing as the above sample in regards to messaging via Nexus.
The messaging via Nexus part remains using a sync operation to:
- determine workflow ID
- execute signal/query/update
The Nexus operations are not opinionated about where the workflow started from.
2 participants
This is sample code to show two ways to send messages (signals, queries, and updates) through Nexus.