-
Notifications
You must be signed in to change notification settings - Fork 58
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
sleep-for-days sample #160
Conversation
b516d1e
to
c09a59c
Compare
c09a59c
to
0213238
Compare
sleep_for_days/workflows.py
Outdated
|
||
@workflow.run | ||
async def run(self, input: SleepForDaysInput) -> str: | ||
while(not self.is_complete): |
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.
This looks a bit JS-y :)
sleep_for_days/workflows.py
Outdated
await workflow.execute_activity(send_email, SendEmailInput(f"{input.numOfDays} until the next email"), start_to_close_timeout=timedelta(seconds=10)) | ||
await workflow.wait([ | ||
asyncio.create_task(workflow.sleep(input.numOfDays * 24 * 60 * 60)), | ||
asyncio.create_task(workflow.wait_condition(lambda: self.is_complete)) |
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.
It would be simpler to use the timeout argument of wait_condition
instead of racing these two tasks.
sleep_for_days/workflows.py
Outdated
from datetime import timedelta | ||
from temporalio import workflow | ||
from dataclasses import dataclass | ||
from sleep_for_days.activities import send_email, SendEmailInput |
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.
In workflow code we import activity code with the sandbox disabled, as follows:
with workflow.unsafe.imports_passed_through():
from ...
It's hard to remember to do it though... would be good to improve that.
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.
Ah I noticed that with the other samples, but the unsafe
scared me so I imported it normally.
One more thing -- can you add a test? (I'd like us to have tests for all samples) |
Added a test |
# Expect no more activity calls to have been made - workflow is complete. | ||
assert num_activity_executions == 4 | ||
# Expect more than 90 days to have passed. | ||
assert (await env.get_current_time() - start_time) > timedelta(days=90) |
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.
Nice!
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.
LGTM!
await workflow.wait( | ||
[ | ||
asyncio.create_task(workflow.sleep(timedelta(days=30))), | ||
asyncio.create_task( | ||
workflow.wait_condition(lambda: self.is_complete) | ||
), | ||
], | ||
return_when=asyncio.FIRST_COMPLETED, | ||
) |
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.
wait_condition
has a timeout which I think would have been clearer here
This sample demonstrates how to use Temporal to run a workflow that periodically sleeps for a number of days.
What was changed
added sleep-for-days sample
Any docs updates needed?
I believe so @webchick