Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[query] ensure nest_asyncio is applied, but only when necessary #13899

Merged
merged 9 commits into from
Oct 25, 2023

Conversation

danking
Copy link
Contributor

@danking danking commented Oct 25, 2023

CHANGELOG: Fix RuntimeError: This event loop is already running error when running hail in a Jupyter Notebook.

Man this is really complicated.

OK, so, things I learned:

  1. asyncio will not create a new event loop if set_event_loop has been called even if set_event_loop(None) has since been called.
  2. asyncio will not create a new event loop in a thread other than the main thread.
  3. aiohttp.ClientSession stashes a copy of the event loop present when it starts. This can cause all manner of extremely confusing behavior if you later change the event loop or use that session from a different thread.

The fix, in the end, wasn't that complicated. Anywhere Hail explicitly asks for an event loop (so that we can run async code), we apply nest asyncio if the event loop is already running. Otherwise we do nothing. Nest asyncio appears to no longer require apply to be called before the event loop starts running.

This PR does not address:

  1. Hail nesting async code in sync code in async code. I think we should avoid this, but the hailtop.fs and hailtop.batch APIs, among others, need async versions before we can do that.
  2. This aiohttp.ClientSession nonsense. We really should take pains to ensure we create one ClientSession per loop and we never mix loops.

@danking danking merged commit a53f4ff into hail-is:main Oct 25, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants