Convert hello activities to sync #173
Conversation
|
|
|
|
GSmithApps
changed the title
There was a problem hiding this comment.
Looks great, thanks! We should do this to other samples too, but this is a good start. @dandavison - thoughts?
| Thread( | ||
| target=self.complete_greeting, | ||
| args=(activity.info().task_token, input), | ||
| ).start() |
There was a problem hiding this comment.
Is this definitely our recommendation or is this a case where it feels more natural for the user to use an asyncio task than to have to explicitly create a thread, seeing as they use asyncio elsewhere with the SDK?
There was a problem hiding this comment.
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.
I'm okay either way on this! Up to you folks. I went ahead and removed the comments though ✅
|
|
||
| # 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.
Using asyncio.run twice in a function in a expositional example surprises me (but I'm new to the sync vs async activity recommendation discussion)
There was a problem hiding this comment.
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 run_coroutine_threadsafe on the loop per https://docs.python.org/3/library/asyncio-dev.html#asyncio-multithreading.
There was a problem hiding this comment.
Thanks so much for catching this! Should be fixed 👍
GSmithApps
force-pushed
the
convert-activities-to-sync
branch
from
4362c80 to
eb0f5b7
Compare
|
Ready if you guys are 🚀 |
| # 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_coroutine_threadsafe(handle.heartbeat(), self.loop) |
There was a problem hiding this comment.
| asyncio.run_coroutine_threadsafe(handle.heartbeat(), self.loop) | |
| asyncio.run_coroutine_threadsafe(handle.heartbeat(), self.loop).result() |
Here and on the one below so you actually wait on the future. Also we recognize a lack of tests on these hello samples, so can you at least confirm when manually running them they work as expected?
There was a problem hiding this comment.
Thank you!
(we discussed the run confirmation off-PR)
| # Non-async activities require an executor; | ||
| # a thread pool executor is recommended. | ||
| # This same thread pool could be passed to multiple workers if desired. | ||
| activity_executor=ThreadPoolExecutor(5), |
There was a problem hiding this comment.
I think all of these samples are going to give warnings of:
Worker max_concurrent_activities is 100 but activity_executor's max_workers is only 5
Can you confirm you are getting these warnings? Can you up the value passed here to 100?
There was a problem hiding this comment.
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.
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
What was changed
Changed the activities in
helloto be synchronousWhy?
To conform to our recommendation of using sync activities unless developers are very sure that their async activities aren't blocking
Checklist
How was this tested:
ran all the samples and their behavior was unchanged
Any docs updates needed?
no need
Notes to reviewers
This is a proposal. Please feel free to edit however you see fit. Thank you!