-
Notifications
You must be signed in to change notification settings - Fork 64
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
Convert hello activities to sync #173
base: main
Are you sure you want to change the base?
Changes from 8 commits
db87dcc
31c7d41
c5b9565
e3741e0
b367478
45d78a0
b191546
2299e65
eb0f5b7
7b2035e
015a182
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 |
---|---|---|
@@ -0,0 +1,71 @@ | ||
import asyncio | ||
from dataclasses import dataclass | ||
from datetime import timedelta | ||
|
||
from temporalio import activity, workflow | ||
from temporalio.client import Client | ||
from temporalio.worker import Worker | ||
|
||
|
||
# While we could use multiple parameters in the activity, Temporal strongly | ||
# encourages using a single dataclass instead which can have fields added to it | ||
# in a backwards-compatible way. | ||
@dataclass | ||
class ComposeGreetingInput: | ||
greeting: str | ||
name: str | ||
|
||
|
||
# Basic activity that logs and does string concatenation | ||
@activity.defn | ||
async def compose_greeting(input: ComposeGreetingInput) -> str: | ||
activity.logger.info("Running activity with parameter %s" % input) | ||
return f"{input.greeting}, {input.name}!" | ||
|
||
|
||
# Basic workflow that logs and invokes an activity | ||
@workflow.defn | ||
class GreetingWorkflow: | ||
@workflow.run | ||
async def run(self, name: str) -> str: | ||
workflow.logger.info("Running workflow with parameter %s" % name) | ||
return await workflow.execute_activity( | ||
compose_greeting, | ||
ComposeGreetingInput("Hello", name), | ||
start_to_close_timeout=timedelta(seconds=10), | ||
) | ||
|
||
|
||
async def main(): | ||
# Uncomment the lines below to see logging output | ||
# import logging | ||
# logging.basicConfig(level=logging.INFO) | ||
|
||
# Start client | ||
client = await Client.connect("localhost:7233") | ||
|
||
# Run a worker for the workflow | ||
async with Worker( | ||
client, | ||
task_queue="hello-activity-task-queue", | ||
workflows=[GreetingWorkflow], | ||
activities=[compose_greeting], | ||
# If the worker is only running async activities, you don't need | ||
# to supply an activity executor because they run in | ||
# the worker's event loop. | ||
): | ||
|
||
# While the worker is running, use the client to run the workflow and | ||
# print out its result. Note, in many production setups, the client | ||
# would be in a completely separate process from the worker. | ||
result = await client.execute_workflow( | ||
GreetingWorkflow.run, | ||
"World", | ||
id="hello-activity-workflow-id", | ||
task_queue="hello-activity-task-queue", | ||
) | ||
print(f"Result: {result}") | ||
|
||
|
||
if __name__ == "__main__": | ||
asyncio.run(main()) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,9 @@ | ||
import asyncio | ||
import time | ||
from concurrent.futures import ThreadPoolExecutor | ||
from dataclasses import dataclass | ||
from datetime import timedelta | ||
from threading import Thread | ||
|
||
from temporalio import activity, workflow | ||
from temporalio.client import Client | ||
|
@@ -18,7 +21,7 @@ def __init__(self, client: Client) -> None: | |
self.client = client | ||
|
||
@activity.defn | ||
async def compose_greeting(self, input: ComposeGreetingInput) -> str: | ||
def compose_greeting(self, input: ComposeGreetingInput) -> str: | ||
# Schedule a task to complete this asynchronously. This could be done in | ||
# a completely different process or system. | ||
print("Completing activity asynchronously") | ||
|
@@ -27,29 +30,28 @@ async def compose_greeting(self, input: ComposeGreetingInput) -> str: | |
# So we store the tasks ourselves. | ||
# See https://docs.python.org/3/library/asyncio-task.html#creating-tasks, | ||
# https://bugs.python.org/issue21163 and others. | ||
_ = asyncio.create_task( | ||
self.complete_greeting(activity.info().task_token, input) | ||
) | ||
Thread( | ||
target=self.complete_greeting, | ||
args=(activity.info().task_token, input), | ||
).start() | ||
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. Is this definitely our recommendation or is this a case where it feels more natural for the user to use an 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. This whole approach is not our recommendation (to just async complete in the background and raise complete). It's meant to demonstrate that it can be done somewhere else. So hacking in a whole new thread like this is probably ok since it's a demonstration of something you would never do anyways, but I would suggest removing the asyncio comments above that are asyncio specific and don't apply to this new code. 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'm okay either way on this! Up to you folks. I went ahead and removed the comments though ✅ |
||
|
||
# Raise the complete-async error which will complete this function but | ||
# does not consider the activity complete from the workflow perspective | ||
activity.raise_complete_async() | ||
|
||
async def complete_greeting( | ||
self, task_token: bytes, input: ComposeGreetingInput | ||
) -> None: | ||
def complete_greeting(self, task_token: bytes, input: ComposeGreetingInput) -> None: | ||
# Let's wait three seconds, heartbeating each second. Note, heartbeating | ||
# during async activity completion is done via the client directly. It | ||
# is often important to heartbeat so the server can know when an | ||
# activity has crashed. | ||
handle = self.client.get_async_activity_handle(task_token=task_token) | ||
for _ in range(0, 3): | ||
print("Waiting one second...") | ||
await handle.heartbeat() | ||
await asyncio.sleep(1) | ||
asyncio.run(handle.heartbeat()) | ||
time.sleep(1) | ||
|
||
# Complete using the handle | ||
await handle.complete(f"{input.greeting}, {input.name}!") | ||
asyncio.run(handle.complete(f"{input.greeting}, {input.name}!")) | ||
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. Using 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. Yes, this is actually bad and a bug. This can cause our client to run on two different event loops which is wrong (but may technically work most of the time). To do this right, you should pass the event loop alongside the client in the constructor of this class, and then use 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. Thanks so much for catching this! Should be fixed 👍 |
||
|
||
|
||
@workflow.defn | ||
|
@@ -77,6 +79,7 @@ async def main(): | |
task_queue="hello-async-activity-completion-task-queue", | ||
workflows=[GreetingWorkflow], | ||
activities=[composer.compose_greeting], | ||
activity_executor=ThreadPoolExecutor(5), | ||
): | ||
|
||
# While the worker is running, use the client to run the workflow and | ||
|
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 think all of these samples are going to give warnings of:
Can you confirm you are getting these warnings? Can you up the value passed here to 100?
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 don't think i got those warnings. (Chad and I subsequently discussed off-PR)
Or I can just go ahead and up it to 100 anyway
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.
Interesting, I wonder why they aren't showing, we may have to investigate, but lack of warnings appearing is not a blocker for this issue of course