python / cpython

The Python programming language
https://www.python.org
Other
63.35k stars 30.34k forks source link

Deprecate get_event_loop() #83710

Closed asvetlov closed 3 years ago

asvetlov commented 4 years ago
BPO 39529
Nosy @asvetlov, @bdarnell, @serhiy-storchaka, @1st1, @minrk, @tirkarthi, @aeros, @johnmellor
PRs
  • python/cpython#23554
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = created_at = labels = ['type-feature', '3.10', 'expert-asyncio'] title = 'Deprecate get_event_loop()' updated_at = user = 'https://github.com/asvetlov' ``` bugs.python.org fields: ```python activity = actor = 'johnmellor' assignee = 'none' closed = True closed_date = closer = 'serhiy.storchaka' components = ['asyncio'] creation = creator = 'asvetlov' dependencies = [] files = [] hgrepos = [] issue_num = 39529 keywords = ['patch'] message_count = 21.0 messages = ['361229', '361245', '361247', '361248', '361249', '361609', '362487', '362674', '381860', '381881', '382044', '391851', '394732', '394821', '407429', '407430', '407432', '407441', '407448', '407453', '407600'] nosy_count = 8.0 nosy_names = ['asvetlov', 'Ben.Darnell', 'serhiy.storchaka', 'yselivanov', 'minrk', 'xtreak', 'aeros', 'johnmellor'] pr_nums = ['23554'] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue39529' versions = ['Python 3.10'] ```

    asvetlov commented 4 years ago

    Yuri proposed it for Python 3.8 but at that time the change was premature. Now we can reconsider it for 3.9

    The problem is that asyncio.get_event_loop() not only returns a loop but also creates it on-demand if the thread is main and the loop doesn't exist.

    It leads to weird errors when get_event_loop() is called at import-time and asyncio.run() is used for asyncio code execution.

    get_running_loop() is a much better alternative when used *inside* a running loop, run() should be preferred for calling async code at top-level. Low-level new_event_loop()/loop.run_until_complete() are still present to run async code if top-level run() is not suitable for any reason.

    asyncio.run() was introduced in 3.7, deprecation on get_event_loop() in 3.8 was able to complicate support of 3.5/3.6 by third-party libraries. 3.5 now reached EOL, 3.6 is in the security-fix mode and going close to EOL. Most people are migrated to newer versions already if they care. The maintenance burden of the introduced deprecation should be pretty low.

    serhiy-storchaka commented 4 years ago

    Will asyncio.get_event_loop() be removed or made an alias of asyncio.get_running_loop()? The latter could minimize the disruption of the existing code.

    asvetlov commented 4 years ago

    Currently, I'm talking about adding a deprecation only. The asyncio.get_event_loop() function will stay in Python for a while in deprecated status. I don't know the exact period but it should be 3 releases at least I guess, with possibly extending to 5 releases if needed.

    asvetlov commented 4 years ago

    Serhiy, maybe I had not understood your proposal properly.

    If you are asking about deprecating get_event_loop() call from outside of async code but implicit call get_running_loop() without a warning if called from async function -- it sounds like a brilliant idea.

    Something like:

    def get_event_loop():
        current_loop = _get_running_loop()
        if current_loop is not None:
            return current_loop
        warnings.warn("...", DeprecationWarning)  
        return get_event_loop_policy().get_event_loop()
    serhiy-storchaka commented 4 years ago

    Yes, it is what I meant.

    Many code written before 3.7 use get_event_loop() because there was no get_running_loop() yet. Now, to avoid deprecation (and making the code more robust) it should be rewritten with using get_running_loop() or get_event_loop() depending on Python version. It is cumbersome, and causes a code churn.

    1st1 commented 4 years ago

    I think we still use get_event_loop() in asyncio code, no? If we do, we should start by raising deprecation warnings in those call sites, e.g. if a Future or Lock is created outside of a coroutine and no explicit event loop is passed. We should do this in 3.9. We can then think about deprecating get_event_loop in 3.10.

    aeros commented 4 years ago

    FWIW, I agree with get_event_loop() being problematic with its error messages, and having a bit too much functionality in a single function from an API design perspective. It commonly comes up as an issue among asyncio users in communities that I'm active in.

    Typically, I advise them to use get_runningloop() where possible, as it can be directly substituted within a coroutine func. For \_init__ methods, I recommend setting their internal loop attribute to None and then setting it to get_running_loop() in either a coro start() method or coro factory method. Outside of directly managing the event loop, this works for many use cases. When it's needed to actually create one, I advise them to use new_event_loop(), but mention that it's already handled if they're using asyncio.run().

    Andrew Svetlov wrote:

    The asyncio.get_event_loop() function will stay in Python for a while in deprecated status. I don't know the exact period but it should be 3 releases at least I guess, with possibly extending to 5 releases if needed.

    With how many libraries that rely on it, I suspect it will likely be a very slow transition from deprecation to removal. 4 versions seems like a reasonable period to me, but I think that 3 may be too short (assuming we retain the newer annual release cycle).

    Yury Selivanov wrote:

    I think we still use get_event_loop() in asyncio code, no?

    Indeed, we currently use it in several places throughout asyncio. From a brief glace using git grep "get_event_loop()":

    Lib/asyncio/futures.py:76: self._loop = events.get_event_loop() Lib/asyncio/futures.py:390: loop = events.get_event_loop() Lib/asyncio/locks.py:81: self._loop = events.get_event_loop() Lib/asyncio/locks.py:177: self._loop = events.get_event_loop() Lib/asyncio/locks.py:244: self._loop = events.get_event_loop() Lib/asyncio/locks.py:375: self._loop = events.get_event_loop() Lib/asyncio/queues.py:35: self._loop = events.get_event_loop() Lib/asyncio/streams.py:45: loop = events.get_event_loop() Lib/asyncio/streams.py:82: loop = events.get_event_loop() Lib/asyncio/streams.py:104: loop = events.get_event_loop() Lib/asyncio/streams.py:120: loop = events.get_event_loop() Lib/asyncio/streams.py:147: self._loop = events.get_event_loop() Lib/asyncio/streams.py:403: self._loop = events.get_event_loop() Lib/asyncio/subprocess.py:206: loop = events.get_event_loop() Lib/asyncio/subprocess.py:227: loop = events.get_event_loop() Lib/asyncio/tasks.py:69: loop = events.get_event_loop() Lib/asyncio/tasks.py:129: loop = events.get_event_loop() Lib/asyncio/tasks.py:590: loop = events.get_event_loop() Lib/asyncio/tasks.py:669: loop = events.get_event_loop() Lib/asyncio/tasks.py:751: loop = events.get_event_loop()

    For brevity, I omitted the docs, tests, and the function definition for get_event_loop().

    Based on Serhiy's idea (of making get_event_loop() an alias for get_running_loop() without warning inside of a coro func, but warning for using it outside of one), many of the above could remain as is to reduce some code churn. We just have to make sure the documentation is updated to reflect get_event_loop() becoming an alias for get_running_loop(), at the same time as the deprecation warning is added for using it outside of a coro func. Otherwise, I suspect it could lead to significant confusion from users that have warnings enabled.

    That being said, I think we should eventually remove asyncio.get_event_loop() entirely from the asyncio internals, including the ones that wouldn't raise deprecation warnings (if/when it's made an alias to get_running_loop()) for improved clarity. Personally, I find that even the name get_event_loop() is rather vague; get_running_loop() is much more obvious as to its purpose and what it does from a readability perspective.

    Yury Selivanov wrote:

    If we do, we should start by raising deprecation warnings in those call sites, e.g. if a Future or Lock is created outside of a coroutine and no explicit event loop is passed.

    For asyncio.Lock (plus other synchronization primitives) and asyncio.Queue, this would be added in https://github.com/python/cpython/pull/18195. Currently waiting on emanu (author of the PR) to finish up some changes, but it's mostly complete. Could I work on adding it to asyncio.Future and other classes in the meantime?

    One point to be clarified though: you mention "created outside of a coroutine and no explicit event loop is passed". However, there are already several deprecations in place for passing an explicit event loop for most (if not all) of the __init__ methods for objects across asyncio's high-level API. In those cases, should the deprecation for creating the object outside of a coroutine function care about whether or not an explicit event loop is passed?

    I can see why it would matter for the lower level parts of the API (such as asyncio.Future) where passing the event loop explicitly is still allowed, but IMO it shouldn't be a factor for ones where passing the event loop explicitly is already deprecated. Especially considering that the loop argument will be removed from those entirely in 3.10 (according to the version listed in the current deprecation warnings added in 3.8).

    1st1 commented 4 years ago

    For asyncio.Lock (plus other synchronization primitives) and asyncio.Queue, this would be added in https://github.com/python/cpython/pull/18195. Currently waiting on emanu (author of the PR) to finish up some changes, but it's mostly complete. Could I work on adding it to asyncio.Future and other classes in the meantime?

    I think the approach should be different:

      # leading underscore is significant:
      loop = asyncio._get_running_loop()  
      if loop is None:
        issue_deprecation_warning()
        loop = asyncio.get_event_loop()
    serhiy-storchaka commented 3 years ago

    I am writing a code (it is not trivial to emit correct warning) and I have a question. What to do if the loop was set by set_event_loop()? Currently get_event_loop() can return a loop without creating a new loop while get_running_loop() raises an error. Should get_event_loop() still support this in future or set_event_loop() will be deprecated?

    asvetlov commented 3 years ago

    I think the deprecation of set_event_loop() is a good idea. The function is not required by asyncio.run() implementation.

    serhiy-storchaka commented 3 years ago

    The most visible effect in tests is that asyncio.gatcher() and some other synchronous functions will need a running loop if any of arguments is a coroutine.

    serhiy-storchaka commented 3 years ago

    New changeset 172c0f2752d8708b6dda7b42e6c5a3519420a4e8 by Serhiy Storchaka in branch 'master': bpo-39529: Deprecate creating new event loop in asyncio.get_event_loop() (GH-23554) https://github.com/python/cpython/commit/172c0f2752d8708b6dda7b42e6c5a3519420a4e8

    5c0d0cd9-bbdc-4a00-8830-a38a1efc5f8a commented 3 years ago

    The maintenance burden of the introduced deprecation should be pretty low.

    This is going to cause an unpleasant amount of churn in the Tornado community. It's been idiomatic (going back 12 years now) to do all your setup in synchronous code before starting the event loop. I'm tempted to restore the implicit loop creation in Tornado's IOLoop.current() (which is now essentially a wrapper around asyncio.get_event_loop()).

    It leads to weird errors when get_event_loop() is called at import-time and asyncio.run() is used for asyncio code execution.

    Checks asyncio.run's implementation Oh, I can see how that's a problem (and I've been giving people bad advice about the interchangeability of Tornado's IOLoop.run_sync and asyncio.run). But why does asyncio.run unconditionally create a new event loop instead of running on asyncio.get_event_loop? If asyncio.run used asyncio.get_event_loop it seems to me that we'd have the expected behavior and wouldn't have to make breaking changes to get_event_loop.

    Low-level new_event_loop()/loop.run_until_complete() are still present to run async code if top-level run() is not suitable for any reason.

    What if you're using run_forever instead of run_until_complete? (this is the common mode of operation for Tornado) There are solutions, of course (my preferred one is await asyncio.Event().wait()), but it's an extra conceptual hurdle to throw at users in a "hello world" example and this is why I've stuck with the model that uses (the equivalent of) run_forever instead of asyncio.run.

    aeros commented 3 years ago

    But why does asyncio.run unconditionally create a new event loop instead of running on asyncio.get_event_loop?

    AFAIK, it does so for purposes of compatibility in programs that need multiple separate event loops and providing a degree of self-dependency. asyncio.run() is entirely self-reliant in that it creates all needed resources at the start and closes them in finalization, rather than depending on existing resources. I believe this to be significantly safer and better guaranteed to function as intended, although perhaps at some cost to convenience in cases like your own where there only needs to be one event loop.

    97d49212-90f1-472f-a5f7-34cb9d6fce6c commented 2 years ago

    The comments in this thread suggest that set_event_loop should also be deprecated, but it hasn't been. It doesn't seem to have any use without get_event_loop().

    I'm trying to understand the consequences of these changes for IPython, and make the changes intended by asyncio folks, but am not quite clear, yet.

    If I understand it correctly, this means that the whole concept of a 'current' event loop is deprecated while no event loop is running?

    My interpretation of these changes is that it means any persistent handles on any event loop while it isn't running is fully the responsibility of individual libraries (e.g. tornado, IPython).

    This is coming up in IPython where we need a handle on the event loop and advance it with run_until_complete for each iteration (it should be the same loop to maintain persistent state across advances, so asyncio.run() would not be appropriate). We previously relied on get_event_loop to manage this handle, but I think we have to now shift to tracking our own handle, and can no longer rely on standard APIs to track a shared instance across packages.

    97d49212-90f1-472f-a5f7-34cb9d6fce6c commented 2 years ago

    Further digging reveals that policy.get_event_loop() is not deprecated while asyncio.get_event_loop() is. Is that intentional? Does that mean switching our calls to get_event_loop_policy().get_event_loop() should continue to work without deprecation?

    asvetlov commented 2 years ago

    IMHO, asyncio.set_event_loop() and policy.get_event_loop()/policy.set_event_loop() are not deprecated by oversight.

    In IPython, I think you could use new_event_loop() for getting a new loop instance. Then, save the loop reference somewhere as a direct attribute, threading.local or ContextVar. Calling loop.run_until_complete() looks pretty normal in your situation.

    At my job, we have Runner class, the basic usage is: with Runner() as runner: runner.run(async_func())

    The implementation is pretty close to asyncio.run() but runner.run(...) can be called multiple times. Async context manager interface is responsible for resource closing.

    Maybe I should extract the implementation into a third-party library, I've found this concept useful for CLI applications at least.

    97d49212-90f1-472f-a5f7-34cb9d6fce6c commented 2 years ago

    Thank you! I think I have enough information to update.

    IMHO, asyncio.set_event_loop()...[is] not deprecated by oversight.

    I'm curious, what is an appropriate use of asyncio.set_event_loop() if you can never get the event loop with get_event_loop()? If you always have to pass the handle around anyway, I'm not sure what the use case for a write-only global would be.

    asvetlov commented 2 years ago

    Ages ago, get_event_loop() did not return the current running loop if called from async function context; it returned a loop instance registered with previous set_event_loop(...) call instead.

    Later we fixed get_event_loop() behavior in 3.5.4 IIRC and added get_running_loop() in 3.7

    set_event_loop() doesn't make sense now from my perspective.

    97d49212-90f1-472f-a5f7-34cb9d6fce6c commented 2 years ago

    Oops, I interpreted "not deprecated by oversight" as the opposite of what you meant. Sorry! All clear, now.

    5c0d0cd9-bbdc-4a00-8830-a38a1efc5f8a commented 2 years ago

    In IPython, I think you could use new_event_loop() for getting a new loop instance. Then, save the loop reference somewhere as a direct attribute, threading.local or ContextVar. Calling loop.run_until_complete() looks pretty normal in your situation.

    That works if you're a leaf in the dependency tree, but what about a library like Tornado (which IPython depends on)? If intermediate libraries (say Tornado and Twisted) introduce their own thread-local loops for backwards compatibility of their existing interfaces, the ecosystem gets fragmented again. Having the thread-local live in asyncio is useful for seamless interoperability across frameworks.

    asyncio.run() is entirely self-reliant in that it creates all needed resources at the start and closes them in finalization, rather than depending on existing resources. I believe this to be significantly safer and better guaranteed to function as intended, although perhaps at some cost to convenience in cases like your own where there only needs to be one event loop.

    Whether or not this is more likely to function as intended depends on assumptions about user expectations. In Tornado (and other async systems I have used in the past), it is the norm to set up handlers on an event loop while it is not running and then start it afterwards. The behavior of asyncio.run is surprising to me. Maybe I'm weird, but regardless of which behavior is surprising to the fewest people, my point is that this change is very disruptive to Tornado and most applications using it, contrary to the claim that the maintenance burden should be pretty low.

    I realize it may be too late to change course, but my preference would have been to resolve the conflict between get_event_loop and asyncio.run by making asyncio.run essentially an alias for asyncio.get_event_loop().run_until_complete. This would relax the isolation of asyncio.run, but that doesn't seem very important to me. The comparable idioms in Tornado (using the IOLoop.run_sync method) all work this way and I've never seen any confusion related to this or a situation in which creating a brand-new event loop in run_sync would be desirable.

    graingert commented 2 years ago

    get_event_loop was deprecated in 3.10, does this mean it will be an alias for get_running_loop in 3.12?

    serhiy-storchaka commented 2 years ago

    It was not deprecated. Only its use outside of the event loop was deprecated. It will be an alias of get_running_loop.

    graingert commented 2 years ago

    @serhiy-storchaka Yes, on 3.12 will it be an alias of get_running_loop?