beanlab / quest_framework

A Python framework for fault-tolerant workflows
0 stars 0 forks source link

Andrew/manager upgrades #17

Closed ABCambridge closed 5 months ago

ABCambridge commented 5 months ago

This PR moves some of the inner behavior needed to use a WorkflowManager into the class itself, rather than through code outside the class. Primarily, the await asyncio.sleep(...) call necessary to prompt the start of a workflow is managed by the class and not through the user. I thought that is a good thing, but I could be wrong! I updated the test_manager.py file to use these changes correctly.

This PR also adds the option to submit updates to the WorkflowManager, which will call each update, followed by a prompt to the workflow. I am considering an option of whether to prompt the workflow after each action is run or to prompt the workflow after all actions are complete.

Effects:

byubean commented 5 months ago

This isn't quite what we want. Let me try to explain a few principles, and you let me know if that makes sense.

An important nuance about async is that it is not about parallelism. It can be used to create an effect like parallel execution, but only one task is ever running at a time (assuming a standard task executor).

So, when you kick off a task from your main task, the implicit understanding is that at some point your main task will suspend (awaiting something), and thus allow the child task the opportunity to run. If your main task never suspends, then without parallel threads your child task would never run—but why would you expect it to if you understood there was only one thread anyway?

In any real use-case of the workflow manager, the calling code will always go on to suspend at some point (and most likely a very near point), at which point the workflow will begin execution. So there is not need for a sleep (which would cause the calling task to suspend) because the calling task will suspend anyway.

The naming used by the async loop is "call soon" (https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.call_soon) which is a subtle reminder that when running a task, the execution does not start immediately, but when the current running task suspends. Perhaps we should rename our function to start_workflow_soon?

Also, building in a sleep into your code is almost always a bad idea. You are thus forcing a performance hit on anyone that uses the library. If your framework needs a sleep to work in its intended use-case, something is wrong.

Now, the tests are a special exception. While they attempt to simulate real use-cases, they have to do it all with one task, and they are trying to control the sequence of execution in a very specific way so as to test a very specific condition. So in this case we use the sleep hack as way to get careful control of the sequence of execution between the testing and tested code.

ABCambridge commented 5 months ago

Yes, that definitely makes sense! Thanks for explaining that so succinctly. I think the difference between the test cases and real use cases pointed out a line that was still blurry to me about async and parallelism. In the back of my head, I understood that only one thing would really run at a time, but I think that my normal assumptions about the use case for async is what moved me toward the changes I made. And yes, I definitely was thinking about how the sleep calls forced a bound on the performance time, so your explanation definitely resolves that. I keep having to remind myself that Python effectively really only has 1 thread 😆. Anyways, the tests make more sense now and it checks out that a real use case shouldn't have an issue with awaiting fairly soon after the workflow is started.

As far as renaming the function, start_workflow_soon would certainly be more explicit. What would you think of something like queue_workflow? Although it's a long one, in my head the most transparent method name would be something like queue_workflow_for_execution.