Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove asyncio toolkit support #490

Merged
merged 3 commits into from
Dec 16, 2021
Merged

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Dec 16, 2021

This PR removes the asyncio and null entry points, and adjusts the tests accordingly. We're not removing the AsyncioEventLoop or its uses in testing: we're only removing the ability to do export ETS_TOOLKIT=asyncio and pick up an AsyncioEventLoop as a result. That functionality was questionable at best, is causing deprecation warnings in Python 3.10, and will become broken in future Python versions with the current code.

Rationale:

  • The idea of asyncio as a drop-in replacement for Qt or Wx at this level doesn't make a lot of sense right now. There are ways it could be made to make sense in the future, but there would need to be a significant design phase, and we'd likely want to consider the whole of ETS rather than just Traits Futures.
  • asyncio has a particular technical issue that sets it apart from Qt and Wx: in both Qt and Wx we have the idea of the "current event loop" (or at least, the event loop associated to the current thread), even if that event loop hasn't been started yet. But asyncio has been moving away from the idea of a current event loop, and has deprecated the asyncio.get_event_loop function. It's possible to set a current event loop using set_event_loop, but that event loop isn't then picked up by asyncio.run: asyncio.run instead creates a brand new event loop. This means that the (relatively common) pattern of placing tasks on the event loop's task queue before starting the event loop becomes problematic with asyncio. See related discussion in https://bugs.python.org/issue39529

The trigger for this PR was that our uses of get_event_loop are now causing warnings on Python 3.10, so we need to find a way to get rid of them. That change will happen in a separate PR from this one, but this one was necessary to enable getting rid of get_event_loop.

Related: #491

@corranwebster
Copy link

Do we need to keep the null backend even if only a stub, since it is a potentially valid backend that might be used in headless environments?

@mdickinson
Copy link
Member Author

Do we need to keep the null backend even if only a stub, since it is a potentially valid backend that might be used in headless environments?

I don't think so: I can't imagine what possible value that entry point could provide in its current form. There's no support for running headless via an ETS_TOOLKIT setting; these entry points only gave the illusion of that support.

There is support for running headless by explicitly creating an AsyncioEventLoop instance and passing it into a TraitsExecutor, but that completely bypasses the entry point machinery, so is unaffected by this PR.

@mdickinson
Copy link
Member Author

mdickinson commented Dec 16, 2021

a potentially valid backend that might be used in headless environments?

To be clear, imports from traits_futures.api won't fail if ETS_TOOLKIT is null, and even code that creates a TraitsExecutor without an explicit event loop will still work. The failure will come as soon as anyone attempts to submit a job using that TraitsExecutor. And at that point:

  • Prior to this PR, the failure mode is that the job submission succeeds, but changes to the associated future will only occur when the particular event loop created by the AsyncioEventLoop is running, and any user who's relying on that would have to be digging into the private internals to retrieve that event loop. In particular, it's important to note that if the user is running an asyncio event loop by some other means (e.g., asyncio.run), that event loop will likely not be the same one that the AsyncioEventLoop is using. (Though it could be, depending on whether the event loop is started before submitting the job or not. But it's fragile.)
  • With this PR, the failure mode is much more immediate: an exception at job submission time, with a message about not having an event loop available for the null toolkit.

@corranwebster
Copy link

To be clear, imports from traits_futures.api won't fail if ETS_TOOLKIT is null

That was my main concern. As long as you can import safely, it simplifies end-user code, even if that code might fail if it is actually used.

@mdickinson mdickinson merged commit df08004 into main Dec 16, 2021
@mdickinson mdickinson deleted the fix/remove-asyncio-entry-points branch December 16, 2021 17:24
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants