Skip to content
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

Workflow Update and Signal handlers concurrency sample #123

Merged
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
5d0307d
Atomic message handlers sample
drewhoskins Jun 19, 2024
bca534a
Remove resize jobs to reduce code size
drewhoskins Jun 19, 2024
8b0a6ed
Misc polish
drewhoskins Jun 19, 2024
fb7b32f
Add test
drewhoskins Jun 19, 2024
42d1f12
Format code
drewhoskins Jun 19, 2024
c96f06d
Continue as new
drewhoskins Jun 20, 2024
6944099
Formatting
drewhoskins Jun 20, 2024
ec1fb89
Feedback, readme, restructure files and directories
drewhoskins Jun 22, 2024
dd58c64
Format
drewhoskins Jun 22, 2024
37e56ed
More feedback. Add test-continue-as-new flag.
drewhoskins Jun 24, 2024
a1506b1
Feedback; throw ApplicationFailures from update handlers
drewhoskins Jun 24, 2024
2cad3dd
Formatting
drewhoskins Jun 24, 2024
d5db7d7
__init__.py
drewhoskins Jun 24, 2024
f39841c
Fix lint issues
drewhoskins Jun 24, 2024
344d694
Dan Feedback
drewhoskins Jun 25, 2024
fc74a69
More typehints
drewhoskins Jun 25, 2024
0b84c25
s/atomic/safe/
drewhoskins Jun 25, 2024
c8e9075
Fix and demo idempotency
drewhoskins Jun 26, 2024
4fc6dac
Compatibility with 3.8
drewhoskins Jun 26, 2024
3ba8882
More feedback
drewhoskins Jun 27, 2024
f47369e
Re-add tests
drewhoskins Jun 27, 2024
5dc6185
Fix flaky test
drewhoskins Jun 27, 2024
5b45b21
Improve update and tests
drewhoskins-temporal Jul 8, 2024
ce4d384
Ruff linting
drewhoskins-temporal Jul 8, 2024
52429bd
Use consistent verbs, improve health check
drewhoskins-temporal Jul 8, 2024
74867f1
poe format
drewhoskins-temporal Jul 8, 2024
c6bdd12
Minor sample improvements
drewhoskins-temporal Jul 8, 2024
62f24a2
Skip update tests under Java test server
dandavison Jul 22, 2024
d933042
Merge pull request #1 from dandavison/drewhoskins_concurrency_sample-dan
drewhoskins-temporal Jul 24, 2024
31e2d59
Merge branch 'main' into drewhoskins_concurrency_sample
drewhoskins-temporal Jul 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ Some examples require extra dependencies. See each sample's directory for specif
* [hello_signal](hello/hello_signal.py) - Send signals to a workflow.
<!-- Keep this list in alphabetical order -->
* [activity_worker](activity_worker) - Use Python activities from a workflow in another language.
* [safe_message_handlers](updates_and_signals/safe_message_handlers/) - Safely handling updates and signals.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should keep this list in alphabetical order (see comment a couple of lines above). Also not a fan of nesting these non-hello samples beneath a directory unnecessarily (you'll note we don't do this much in other samples here or in many samples repos). If you must inconsistently nest this sample, you may want nested bullets here.

* [bedrock](bedrock) - Orchestrate a chatbot with Amazon Bedrock.
* [cloud_export_to_parquet](cloud_export_to_parquet) - Set up schedule workflow to process exported files on an hourly basis
* [context_propagation](context_propagation) - Context propagation through workflows/activities via interceptor.
Expand Down
107 changes: 107 additions & 0 deletions tests/updates_and_signals/safe_message_handlers_test.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We prefer tests to be in the same directory under tests that they are in the top level. So /custom_converter/ tests are in /tests/custom_converter/ and therefore /updates_and_signals/safe_message_handlers/ tests should be under /tests/updates_and_signals/safe_message_handlers/ (granted as mentioned in comments before, I don't think we should nest sample dirs).

Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
import uuid

from temporalio import common, workflow
from temporalio.client import Client, WorkflowUpdateFailedError
from temporalio.exceptions import ApplicationError
from temporalio.worker import Worker

from updates_and_signals.safe_message_handlers.activities import (
allocate_nodes_to_job,
deallocate_nodes_for_job,
find_bad_nodes,
)
from updates_and_signals.safe_message_handlers.starter import do_cluster_lifecycle
from updates_and_signals.safe_message_handlers.workflow import (
ClusterManagerAllocateNNodesToJobInput,
ClusterManagerInput,
ClusterManagerWorkflow,
)


async def test_safe_message_handlers(client: Client):
task_queue = f"tq-{uuid.uuid4()}"
async with Worker(
client,
task_queue=task_queue,
workflows=[ClusterManagerWorkflow],
activities=[allocate_nodes_to_job, deallocate_nodes_for_job, find_bad_nodes],
):
cluster_manager_handle = await client.start_workflow(
ClusterManagerWorkflow.run,
ClusterManagerInput(),
id=f"ClusterManagerWorkflow-{uuid.uuid4()}",
task_queue=task_queue,
)
await do_cluster_lifecycle(cluster_manager_handle, delay_seconds=1)
result = await cluster_manager_handle.result()
assert result.max_assigned_nodes == 12
assert result.num_currently_assigned_nodes == 0


async def test_update_idempotency(client: Client):
task_queue = f"tq-{uuid.uuid4()}"
async with Worker(
client,
task_queue=task_queue,
workflows=[ClusterManagerWorkflow],
activities=[allocate_nodes_to_job, deallocate_nodes_for_job, find_bad_nodes],
):
cluster_manager_handle = await client.start_workflow(
ClusterManagerWorkflow.run,
ClusterManagerInput(),
id=f"ClusterManagerWorkflow-{uuid.uuid4()}",
task_queue=task_queue,
)

await cluster_manager_handle.signal(ClusterManagerWorkflow.start_cluster)

nodes_1 = await cluster_manager_handle.execute_update(
ClusterManagerWorkflow.allocate_n_nodes_to_job,
ClusterManagerAllocateNNodesToJobInput(num_nodes=5, job_name=f"jobby-job"),
)
# simulate that in calling it twice, the operation is idempotent
nodes_2 = await cluster_manager_handle.execute_update(
ClusterManagerWorkflow.allocate_n_nodes_to_job,
ClusterManagerAllocateNNodesToJobInput(num_nodes=5, job_name=f"jobby-job"),
)
# the second call should not allocate more nodes (it may return fewer if the health check finds bad nodes
# in between the two signals.)
assert nodes_1 >= nodes_2


async def test_update_failure(client: Client):
task_queue = f"tq-{uuid.uuid4()}"
async with Worker(
client,
task_queue=task_queue,
workflows=[ClusterManagerWorkflow],
activities=[allocate_nodes_to_job, deallocate_nodes_for_job, find_bad_nodes],
):
cluster_manager_handle = await client.start_workflow(
ClusterManagerWorkflow.run,
ClusterManagerInput(),
id=f"ClusterManagerWorkflow-{uuid.uuid4()}",
task_queue=task_queue,
)

await cluster_manager_handle.signal(ClusterManagerWorkflow.start_cluster)

await cluster_manager_handle.execute_update(
ClusterManagerWorkflow.allocate_n_nodes_to_job,
ClusterManagerAllocateNNodesToJobInput(num_nodes=24, job_name=f"big-task"),
)
try:
# Try to allocate too many nodes
await cluster_manager_handle.execute_update(
ClusterManagerWorkflow.allocate_n_nodes_to_job,
ClusterManagerAllocateNNodesToJobInput(
num_nodes=3, job_name=f"little-task"
),
)
except WorkflowUpdateFailedError as e:
assert isinstance(e.cause, ApplicationError)
assert e.cause.message == "Cannot allocate 3 nodes; have only 1 available"
finally:
await cluster_manager_handle.signal(ClusterManagerWorkflow.shutdown_cluster)
result = await cluster_manager_handle.result()
assert result.num_currently_assigned_nodes + result.num_bad_nodes == 24
Empty file added updates_and_signals/__init__.py
Empty file.
21 changes: 21 additions & 0 deletions updates_and_signals/safe_message_handlers/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Atomic message handlers

This sample shows off important techniques for handling signals and updates, aka messages. In particular, it illustrates how message handlers can interleave or not be completed before the workflow completes, and how you can manage that.

* Here, using workflow.wait_condition, signal and update handlers will only operate when the workflow is within a certain state--between cluster_started and cluster_shutdown.
* You can run start_workflow with an initializer signal that you want to run before anything else other than the workflow's constructor. This pattern is known as "signal-with-start."
* Message handlers can block and their actions can be interleaved with one another and with the main workflow. This can easily cause bugs, so we use a lock to protect shared state from interleaved access.
* Message handlers should also finish before the workflow run completes. One option is to use a lock.
* An "Entity" workflow, i.e. a long-lived workflow, periodically "continues as new". It must do this to prevent its history from growing too large, and it passes its state to the next workflow. You can check `workflow.info().is_continue_as_new_suggested()` to see when it's time. Just make sure message handlers have finished before doing so.
* Message handlers can be made idempotent. See update `ClusterManager.allocate_n_nodes_to_job`.

To run, first see [README.md](../../README.md) for prerequisites.

Then, run the following from this directory to run the sample:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes people get confused that they can't just run these two commands in the same terminal because the first blocks. In our sample READMEs we usually make clear that the starter needs to be in a separate terminal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copy-pasted this. Looks like I got unlucky in which one I chose!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Yeah we are admittedly not consistent, this is not a blocker or anything.


```bash
poetry run python worker.py
poetry run python starter.py
```

This will start a worker to run your workflow and activities, then start a ClusterManagerWorkflow and put it through its paces.
Empty file.
45 changes: 45 additions & 0 deletions updates_and_signals/safe_message_handlers/activities.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import asyncio
from dataclasses import dataclass
from typing import List

from temporalio import activity


@dataclass
class AllocateNodesToJobInput:
nodes: List[str]
job_name: str


@activity.defn
async def allocate_nodes_to_job(input: AllocateNodesToJobInput):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should provide type hints for every activity return, even if -> None, it helps callers

print(f"Assigning nodes {input.nodes} to job {input.job_name}")
await asyncio.sleep(0.1)


@dataclass
class DeallocateNodesForJobInput:
nodes: List[str]
job_name: str


@activity.defn
async def deallocate_nodes_for_job(input: DeallocateNodesForJobInput):
print(f"Deallocating nodes {input.nodes} from job {input.job_name}")
await asyncio.sleep(0.1)


@dataclass
class FindBadNodesInput:
nodes_to_check: List[str]


@activity.defn
async def find_bad_nodes(input: FindBadNodesInput) -> List[str]:
await asyncio.sleep(0.1)
bad_nodes = [n for n in input.nodes_to_check if int(n) % 5 == 0]
if bad_nodes:
print(f"Found bad nodes: {bad_nodes}")
else:
print("No new bad nodes found.")
return bad_nodes
80 changes: 80 additions & 0 deletions updates_and_signals/safe_message_handlers/starter.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import argparse
import asyncio
import logging
import uuid
from typing import Optional

from temporalio import client, common
from temporalio.client import Client, WorkflowHandle

from updates_and_signals.safe_message_handlers.workflow import (
ClusterManagerAllocateNNodesToJobInput,
ClusterManagerDeleteJobInput,
ClusterManagerInput,
ClusterManagerWorkflow,
)


async def do_cluster_lifecycle(wf: WorkflowHandle, delay_seconds: Optional[int] = None):

await wf.signal(ClusterManagerWorkflow.start_cluster)

allocation_updates = []
for i in range(6):
allocation_updates.append(
wf.execute_update(
ClusterManagerWorkflow.allocate_n_nodes_to_job,
ClusterManagerAllocateNNodesToJobInput(
num_nodes=2, job_name=f"task-{i}"
),
)
)
await asyncio.gather(*allocation_updates)

if delay_seconds:
await asyncio.sleep(delay_seconds)

deletion_updates = []
for i in range(6):
deletion_updates.append(
wf.execute_update(
ClusterManagerWorkflow.delete_job,
ClusterManagerDeleteJobInput(job_name=f"task-{i}"),
)
)
await asyncio.gather(*deletion_updates)

await wf.signal(ClusterManagerWorkflow.shutdown_cluster)


async def main(should_test_continue_as_new: bool):
# Connect to Temporal
client = await Client.connect("localhost:7233")

cluster_manager_handle = await client.start_workflow(
ClusterManagerWorkflow.run,
ClusterManagerInput(test_continue_as_new=should_test_continue_as_new),
id=f"ClusterManagerWorkflow-{uuid.uuid4()}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other samples we have used fixed workflow IDs, but don't technically have to here, but it makes the id_reuse_policy have no value since this is always unique

task_queue="atomic-message-handlers-task-queue",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Task queue was not changed to match the sample name

id_reuse_policy=common.WorkflowIDReusePolicy.TERMINATE_IF_RUNNING,
)
delay_seconds = 10 if should_test_continue_as_new else 1
await do_cluster_lifecycle(cluster_manager_handle, delay_seconds=delay_seconds)
result = await cluster_manager_handle.result()
print(
f"Cluster shut down successfully. It peaked at {result.max_assigned_nodes} assigned nodes ."
f" It had {result.num_currently_assigned_nodes} nodes assigned at the end."
)


if __name__ == "__main__":
logging.basicConfig(level=logging.INFO)
parser = argparse.ArgumentParser(description="Atomic message handlers")
parser.add_argument(
"--test-continue-as-new",
help="Make the ClusterManagerWorkflow continue as new before shutting down",
action="store_true",
default=False,
)
args = parser.parse_args()
asyncio.run(main(args.test_continue_as_new))
41 changes: 41 additions & 0 deletions updates_and_signals/safe_message_handlers/worker.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import asyncio
import logging

from temporalio import activity, common, workflow
from temporalio.client import Client, WorkflowHandle
from temporalio.worker import Worker

from updates_and_signals.safe_message_handlers.workflow import (
ClusterManagerWorkflow,
allocate_nodes_to_job,
deallocate_nodes_for_job,
find_bad_nodes,
)

interrupt_event = asyncio.Event()


async def main():
# Connect client
client = await Client.connect("localhost:7233")

async with Worker(
client,
task_queue="atomic-message-handlers-task-queue",
workflows=[ClusterManagerWorkflow],
activities=[allocate_nodes_to_job, deallocate_nodes_for_job, find_bad_nodes],
):
# Wait until interrupted
logging.info("ClusterManagerWorkflow worker started, ctrl+c to exit")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent use of logging vs print

await interrupt_event.wait()
logging.info("Shutting down")


if __name__ == "__main__":
logging.basicConfig(level=logging.INFO)
loop = asyncio.new_event_loop()
try:
loop.run_until_complete(main())
except KeyboardInterrupt:
interrupt_event.set()
loop.run_until_complete(loop.shutdown_asyncgens())
Loading
Loading