Conversation
github-actions
bot
changed the title
markwallace-microsoft
added
the
documentation
| finally: | ||
| await self._run_cleanup(checkpoint_storage) | ||
|
|
||
| async def _resume() -> asyncio.Task[None]: # noqa: RUF029 |
There was a problem hiding this comment.
The # noqa: RUF029 comment is unnecessary here. RUF029 warns about unused async functions, but _resume is clearly used on line 634 where it's passed to BackgroundRunHandle. This suppression should be removed.
| async def _resume() -> asyncio.Task[None]: # noqa: RUF029 | |
| async def _resume() -> asyncio.Task[None]: |
| from ._events import WorkflowEvent | ||
| from ._runner_context import RunnerContext | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
The logger variable is imported but never used in this module. Consider removing the unused import or adding appropriate debug/error logging where it might be useful (e.g., in the respond method when resuming after idle).
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class BackgroundRunHandle: |
There was a problem hiding this comment.
It feels like this class is essentially a wrapper around asyncio primitives: create_task and an asyncio.Queue. Why do we need to wrap these well-known constructs?
There was a problem hiding this comment.
and this also adds another concept that people have to learn, I do not think this is needed
| return response_stream | ||
| return response_stream.get_final_response() | ||
|
|
||
| def run_in_background( |
There was a problem hiding this comment.
Do we genuinely need a polling-based consumption pattern? Are we getting feedback that this is missing today?
We're now pushing more concerns onto the caller. Every consumer has to:
- Write the poll loop
- Pick a sleep interval (and get it "wrong", too slow adds latency, too fast wastes cycles)
- Route events by type manually
- Track which request IDs map to which responses
- Remember to drain after idle
- Handle the resume-after-idle edge case
There was a problem hiding this comment.
I very much agree, this is not needed and leads to un-pythonic code
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class BackgroundRunHandle: |
There was a problem hiding this comment.
and this also adds another concept that people have to learn, I do not think this is needed
| # Single poll loop: process all events and respond to requests inline. | ||
| # The workflow continues running in the background while we process events. | ||
| outputs: list[str] = [] | ||
| while not handle.is_idle: |
There was a problem hiding this comment.
I see almost no value in this, because it makes people have to understand this whole idle, poll, etc. Also a user having to write asyncio.sleep does not seem right, a much simpler pattern to solve for this scenario is to use something like a callback for response required, that's a well known concept in Python and doesn't require as much complexity as this. And when I compare these samples to existing sample that just use streaming then that is enough, that already allows you to do other stuff in the meantime, and if you really need it, a user can call that in a separate thread and then you ahve this with Python primitives instead of another new thing.
| return response_stream | ||
| return response_stream.get_final_response() | ||
|
|
||
| def run_in_background( |
There was a problem hiding this comment.
I very much agree, this is not needed and leads to un-pythonic code
Motivation and Context
Currently, we have a very limited way of handling workflow runs and responding to events. Users have to wait until a workflow converges to process events, such as requests.
Description
Contribution Checklist