try more complete lifecycle

[wild-endeavor]

into #2737

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.78%. Comparing base (7b4899a) to head (c16b4d7).

Additional details and impacted files

@@                  Coverage Diff                  @@
##           async-loop-issue    #2744       +/-   ##
=====================================================
+ Coverage             45.40%   90.78%   +45.38%     
=====================================================
  Files                   194       57      -137     
  Lines                 19685     2842    -16843     
  Branches               2854        0     -2854     
=====================================================
- Hits                   8937     2580     -6357     
+ Misses                10301      262    -10039     
+ Partials                447        0      -447     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[thomasjpfan]

If there is an existing event loop running, then I do not think a library should be modifying the global event loop.

If there is another event loop running with tasks from another library, then switching out the event loop from underneath them can cause problems. For example, if another library is actively scheduling work, they will have task end up in two different loops.

[wild-endeavor]

I can add a try block around this then, to look for an existing event loop, and use that if found. But I think that is not a good idea. Still learning, but I feel libraries should not be creating event loops on import. As of today, this is true of flytekit's dependencies (except for the unionai library).

The lack of an event loop is why the error came about in the first place right? asyncio.run() creates and then destroys/cleans up the event loop. But if there's one that already exists (which is the case in the main issue with these unionfses), it will basically ignore it, and create a new one. After asyncio.run() completes, it does not restore the existing one, which is why when the data persistence layer goes to look for one, it can't find one and errors.

My issue with adding a try block... and then using the event loop if we find one.

• asyncio.run doesn't do this.
• In our current case, we end up using an event loop in the union library. If the union library is not installed, then we use one created by flytekit. Difference in behavior is not good I feel. If we want to differentiate, I feel the way to do it is: if user runs pyflyte-execute, then flytekit creates and manages its own event loop, if the user runs union-execute (which i know doesn't exist today), then unionai creates and manages its own event loop. Changing where the event loop is managed based on what library is installed feels bad. Also if a user installs some other library that we don't know about that creates its own event loop, and that is somehow triggered before this code is (yes i know this is unlikely since we load all flytekit code before loading user code), then flytekit ends up using some random event loop.
• If we had had code in flytekit to use an existing loop, and the unionai library is as it stands, then flytekit would've ended up using the event loop created as part of a grpc.aio channel. That channel may get cleaned up or deleted right?
• When we get to more advanced async stuff, we'll may very well want to explicitly handle cleanup in case of termination. Relying on unionai's loop to clean up things in flytekit feels incorrect also.
• I'm not suggesting we add this code every where in flytekit/union... this is only on the main entrypoint of an executable. I think in this scenario, it's okay to take ownership. In other parts of the code, yes, definitely use the loop available, and maybe even crash if one's not found (rather than grpcio's behavior).

Event loops are thread singletons, there's one per thread. I feel that if your library needs an event loop on import (not on calling an executable like pyflyte or pyflyte-execute), then you should run it in a different thread.

Alternatively we can also check to see if there's an event loop, save it to a variable if so, and then restore it later, basically what async run does, but with one extra step, but honestly I'd rather see it fail. Seeing the failure allowed us to find an issue in the union library.

[thomasjpfan]

unionfs does not create the event loop during import. The event loop is created when union:// fsspec protocol is used which initializes union's fsspec implementation.

if the user runs union-execute (which i know doesn't exist today), then unionai creates and manages its own event loop.

In principle, I am okay with this, but async python libraries do different things:

1. https://github.com/grpc/grpc/blob/1f70b34fb15ffb409553c5f7055cfd5cca61e98e/src/python/grpcio/grpc/_cython/_cygrpc/aio/common.pyx.pxi#L177-L194 : If there is no global event loop, then create one
2. https://github.com/fsspec/filesystem_spec/blob/76ca4a68885d572880ac6800f079738df562f02c/fsspec/asyn.py#L318-L321: If there is no loop passed in, then create a event loop on another thread
3. https://github.com/Tinche/aiofiles/blob/7582fda077e0e5b58a4f7fa3f11d0f53ef36eed5/src/aiofiles/threadpool/__init__.py#L79-L80: Errors if there is not a running event loop

My preferred solution is 2, if there was a good way to pass in the new event loop into a library that does 1.

[wild-endeavor]

Thinking about this more, yeah shouldn't we do 2 always? We can't prevent user code from arbitrarily running asyncio.run in their code and as we've seen with our own usage, that will destroy the then-current event loop. So basically the rule is:
Anytime an event loop is needed by flytekit/union, including when it needed by a downstream library like in the case of async grpc (and we'll just have to know), always create a loop on another thread and

There will probably be bugs related to usage of id in artifacts and maybe need more contextvars in general but we can address those.

pass in the new event loop into a library that does 1.

in the case of grpc aio, doesn't just having the loop set count as passing it in?

[wild-endeavor]

@thomasjpfan @pingsutw what do you think about this pr + https://github.com/unionai/unionai/pull/402 and maybe taking into account the windows policy?