Add Request/Response sample with update-based responses (fixes #6) - #166
Add Request/Response sample with update-based responses (fixes #6)#166barmoshe wants to merge 7 commits into
Conversation
|
#6 and the Go samples predate Workflow Update. We now recommend using workflow update for request/response instead of the "response activity" pattern. |
|
@barmoshe — checking in on this one. Are you still interested in picking it up? If we don't hear back in the next couple of weeks we'll close this to keep the queue tidy. |
|
@brianstrauch thanks for the ping, and yes, still interested. Happy to rework it around Before I write it I'd rather use your offer to help scope, because the obvious minimal version overlaps
Two questions:
I'll start on the version above so nothing is waiting on me, and adjust to whatever you say. I'll merge current main into the branch and push there, so #166 updates in place. |
…io#6) Issue temporalio#6 and the Go sample it was ported from predate Workflow Update. Per maintainer feedback, replace the response-activity port with an update-based sample: the update handler runs the uppercase activity and returns the result to the caller, removing the response task queue, the callback activity and the request IDs. Ports the durability parts of samples-go/reqrespupdate: a request counter, continue-as-new to bound history, and an update validator that rejects requests while a run drains so the requester backs off and retries against the fresh run. Uses workflow.all_handlers_finished() for the drain rather than Go's hand-rolled pending counter, matching message_passing/safe_message_handlers. message_passing/waiting_for_handlers notes the continue-as-new drain case is not illustrated there, so this sample covers it. Adds tests, which the original sample did not have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tests were skipping when env.supports_time_skipping, on the assumption that the Java test server does not support workflow update. It does: both tests pass against it, repeatedly. CI runs the suite twice, once per environment, so the guard was halving this sample's coverage for no reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
(1) Looks good, (2) I'd opt for parity with samples-go |
|
Thanks, that settles both. The branch already matches: the rework is pushed as One thing I can't do from a fork: |
There was a problem hiding this comment.
Pull request overview
Adds a new Temporal Python sample demonstrating request/response using Workflow Update (instead of activity/callback or query approaches), including continue-as-new for bounded history and validator-based backpressure while draining toward continue-as-new.
Changes:
- Introduces
reqrespupdate/sample: update handler executes an activity and returns the result directly to the caller, with bounded-history continue-as-new behavior. - Adds a requester/worker/starter runnable trio plus README documentation for running the sample locally.
- Adds tests covering basic request/response and repeated continue-as-new behavior; wires the new package into the top-level README and
pyproject.toml.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/reqrespupdate/workflow_test.py | New tests for update-based request/response and continue-as-new behavior |
| tests/reqrespupdate/init.py | Declares test package/module |
| reqrespupdate/workflow.py | Implements the update-based uppercase workflow with validator backpressure + continue-as-new |
| reqrespupdate/activities.py | Adds uppercase activity used by the workflow |
| reqrespupdate/worker.py | Adds runnable worker for the sample |
| reqrespupdate/starter.py | Adds runnable workflow starter for the sample |
| reqrespupdate/requester.py | Adds runnable requester client exercising updates and retry-on-backoff |
| reqrespupdate/README.md | Documents the sample and the continue-as-new/backpressure rationale |
| reqrespupdate/init.py | Declares package |
| README.md | Adds sample to the repository’s sample index |
| pyproject.toml | Adds reqrespupdate to packaged modules list |
Comments suppressed due to low confidence (2)
tests/reqrespupdate/workflow_test.py:55
- After changing
request_uppercaseto take(client, workflow_id, ...), the test should call it using the workflow ID (so it can follow the continue-as-new chain) and terminate via an unpinned handle for consistency.
handle = await client.start_workflow(
UppercaseWorkflow.run,
UppercaseWorkflowInput(),
id=f"reqrespupdate-{uuid.uuid4()}",
task_queue=task_queue,
)
try:
for text in ["foo", "bar", "baz"]:
assert await request_uppercase(handle, text) == text.upper()
finally:
await handle.terminate()
tests/reqrespupdate/workflow_test.py:84
- This test currently calls
request_uppercase(handle, ...)using the handle returned fromstart_workflow, which is typically pinned to the original run. Once the workflow continues-as-new, retries should target the workflow ID (unpinned) to reach the fresh run; otherwise the retry loop may never succeed. Align the test with the sample requester by callingrequest_uppercase(client, handle.id, ...)and terminating via an unpinned handle.
handle = await client.start_workflow(
UppercaseWorkflow.run,
# Low enough that the run continues as new several times over the
# requests below.
UppercaseWorkflowInput(requests_before_continue_as_new=2),
id=f"reqrespupdate-{uuid.uuid4()}",
task_queue=task_queue,
)
try:
# Whether any individual request is rejected depends on how the
# continue-as-new interleaves with it, so we assert the contract the
# requester actually relies on: every request eventually succeeds,
# and the workflow does continue as new underneath.
for i in range(6):
assert await request_uppercase(handle, f"foo{i}") == f"FOO{i}"
description = await client.get_workflow_handle(handle.id).describe()
assert description.run_id != handle.first_execution_run_id
finally:
await client.get_workflow_handle(handle.id).terminate()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Bound the test retry helper with a max attempt count and a short sleep, so a workflow that stops continuing as new fails the test instead of retrying forever and hanging CI. - Skip both tests on the time-skipping server, matching the other update tests in the repo (temporalio/sdk-java#1903). - Reject requests_before_continue_as_new < 1 in the workflow constructor. Zero made the run continue as new immediately and forever while rejecting every request. - Move TASK_QUEUE and WORKFLOW_ID into __init__.py, as sleep_for_days and message_passing/waiting_for_handlers do, instead of the worker hardcoding the task queue name. - Use the existing handle in the continue-as-new test rather than re-fetching it; start_workflow does not pin the handle to a run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed Kept |
Reworked around
@workflow.update, per @cretz's and @brianstrauch's feedback that #6 and the Go sample it was ported from both predate Workflow Update.reqrespactivity/is replaced byreqrespupdate/, a port of samples-go/reqrespupdate.What it shows
Where it differs from the Go sample
workflow.all_handlers_finished()replaces Go's hand-rolledpendingUpdatescounter, matchingmessage_passing/safe_message_handlers.message_passing/waiting_for_handlers/README.mdnotes the continue-as-new drain case is not illustrated there, so this sample covers it.Placement
Top-level
reqrespupdate/for parity with samples-go. Happy to move it undermessage_passing/instead if you would rather it live with the other message-passing samples.Testing
tests/reqrespupdate/workflow_test.py: a basic request/response test, and one that drives the threshold low and asserts every request still succeeds across a continue-as-new.uv run poe lintclean (ruff and mypy across the repo).temporal server start-dev: responses stream once per second, and killing the worker mid-stream stalls requests until it restarts, after which they resume.