Simple samples #4
Conversation
| async def run(self, name: str) -> str: | ||
| # Use execute_activity_method which lets us reference a method instead | ||
| # of a function | ||
| return await workflow.execute_activity_method( |
There was a problem hiding this comment.
This is waiting on temporalio/sdk-python#69
|
|
||
|
|
||
| class GreetingComposer: | ||
| def __init__(self, client: Client) -> None: |
There was a problem hiding this comment.
Might as well put the client on activity context, would be super useful.
I want to do this in TS too. Even though the worker uses a different connection type the relevant connection options are similar.
There was a problem hiding this comment.
Disagree. People can put whatever activity dependencies they want. They create the client, they can pass it everywhere they want, be it an activity or non-activity. I don't want any public activity API to have anything to do with public client API. I don't even want the former package to reference the latter.
There was a problem hiding this comment.
The package reference is definitely something to consider but overall I think adding this definitely provides value.
There was a problem hiding this comment.
May be subject to a general discussion outside of this PR.
I don't see why we can't ask users to pass all things they need to their activities at class instantiation/registration time (be it a Temporal client, another HTTP client, a DB client, etc).
This would be a new thing for all SDKs which all, today, allow one to do this if they want but don't provide this convenience thing to do it for them because it's not necessary and it's not difficult to do themselves.
| @workflow.defn | ||
| class GreetingWorkflow: | ||
| @workflow.run | ||
| async def run(self, name: str) -> str: |
There was a problem hiding this comment.
No dataclass??
Kidding, I think we should show that those aren't strictly required.
Same way we don't bother with data classes for return values either.
There was a problem hiding this comment.
Yeah, I'm being a bit practical in several places. The dataclass-everywhere, while recommended in production, just isn't how quick scripters will do things (and lots of Python people seem to be scripters).
hello/hello_continue_as_new.py
Outdated
| # new when the history is growing too large. Continue as new is | ||
| # essentially a workflow restart (though another workflow can be used). | ||
| try: | ||
| await workflow.wait_condition(lambda: self.exit_requested, timeout=2) |
There was a problem hiding this comment.
Hmm.. timeout should be a timedelta or the are name should have a unit.
There was a problem hiding this comment.
This is built to match https://docs.python.org/3/library/asyncio-task.html#asyncio.wait. Really, none of the Python APIs use timedeltas (https://docs.python.org/3/library/asyncio-task.html#asyncio.sleep, https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.call_later, etc).
I am trying to avoid accepting Union[float, timedelta] everywhere to avoid confusion, so I figured following other asyncio APIs on just this call makes sense. But I can change if wanted. (I wish we had reviewed these before)
There was a problem hiding this comment.
I wish we had reviewed this earlier too, seeing these samples now puts this in my face so more suggestions come up.
Nothing critical, if matching asyncio APIs is a goal then I'd say it's fine as is.
| @@ -0,0 +1,57 @@ | |||
| import asyncio | |||
There was a problem hiding this comment.
In TS we marked local activities as experimental since they require some more time in production before we consider them stable.
Python is still considered alpha so I wouldn't strictly bother to mention this but since we're pretty confident about the rest of the features I'd consider mentioning.
There was a problem hiding this comment.
Not following this comment. Are you saying I should mark local activities in Python experimental? Why do they require more time in production than any other feature of the Python SDK?
There was a problem hiding this comment.
Are you saying I should mark local activities in Python experimental?
Yes, because of the complexity here and the fact that they're so new even in TS.
The Core part of local activities has barely been used in production making this the least stable feature in the SDK.
Python is still considered alpha so I wouldn't strictly bother to mention this but since we're pretty confident about the rest of the features I'd consider mentioning.
But also noting this ^
| @workflow.run | ||
| async def run(self) -> List[str]: | ||
| # Run 5 activities at the same time | ||
| results = await asyncio.gather( |
There was a problem hiding this comment.
I'd mention as_completed as an alternative
There was a problem hiding this comment.
That has nothing to do w/ Temporal. Not sure we should teach Python here lest we be tempted to comment about all the other asyncio uses and their alternatives everywhere we use them (that would be a lot of commenting).
There was a problem hiding this comment.
I think you'll be surprised by the amount of users that will ask you this question, leaving it up to you to decide
| return greetings | ||
|
|
||
| @workflow.signal | ||
| async def submit_greeting(self, name: str) -> None: |
There was a problem hiding this comment.
Doesn't need to be async right?
I'm considering disallowing async signal handler in TS altogether because users don't understand that they need to wait for all handlers before completing their workflow and end up losing progress.
It's much easier to reason about workflow code if all "io" is done in the main workflow thread.
There was a problem hiding this comment.
It does need to be async because I use await for putting on the queue. I could use put_nowait since the queue does not have a max size, but this is clear too.
It's much easier to reason about workflow code if all "io" is done in the main workflow thread.
A caller choice IMO.
There was a problem hiding this comment.
Yes, right now it's the caller's choice but it's also way too easy to get wrong.
There was a problem hiding this comment.
A lot of my comments here are not blocking for this PR or not relevant for actual samples, just random thoughts from seeing how the SDK is used.
I think we could use a sample showing how to run sync activities (ones that run in an executor).
Also cancelling from workflow should be covered eventually, I assume you're aware, also not blocking this PR.
I can add this. There about 100 things I could think of. EDIT: Added threading and multiprocesing examples
Can you clarify what you mean by cancelling from workflow? Handling a cancel? Cancelling a child? Cancelling an external workflow? Can you point to the TS sample you'd like me to emulate? |
|
Running I believe it has to do with the |
|
@Tomperez98 - Are you using Python 3.10+? We recently fixed some of this in some Also, we plan to overhaul some of the dataclass type hinting inference, see temporalio/sdk-python#78. That will likely happen before these samples land. |
No. I'm running |
What was changed
Added some basic feature samples
Why?
Java has these and in Go we've needed them because they are great to point people to. Docs often don't do as good of a job as code does at demonstrating code features to coders.
Closes #8