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

configure asyncio loop using loop_factory kwarg rather than using the set_event_loop_policy #7969

Merged
merged 3 commits into from Jul 13, 2023

Conversation

graingert
Copy link
Member

@graingert graingert commented Jul 5, 2023

Closes #7492

it would be nicer to not need the ProactorEventLoop at all, but there's still some issues using it (see failures in #7494)

This PR backports the new "loop_factory" kwarg for asyncio.run introduced in Python 3.12 and uses it instead of set_event_loop_policy this PR is still useful even if #7494 as this avoids calling uvloop.install which also mutates the global event loop policy

  • Tests added / passed
  • Passes pre-commit run --all-files

@graingert graingert requested a review from fjetter as a code owner July 5, 2023 14:57
@graingert graingert force-pushed the use-loop-factory-to-set-loop-type branch 3 times, most recently from 62b9e7d to 1b1fae7 Compare July 5, 2023 15:20
@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2023

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       20 files  ±0         20 suites  ±0   11h 19m 45s ⏱️ + 18m 9s
  3 704 tests ±0    3 596 ✔️ +1     106 💤 ±0  2  - 1 
35 826 runs  ±0  34 076 ✔️ +3  1 748 💤  - 1  2  - 2 

For more details on these failures, see this check.

Results for commit 05dfece. ± Comparison against base commit 8e6c287.

♻️ This comment has been updated with latest results.

@graingert graingert force-pushed the use-loop-factory-to-set-loop-type branch 2 times, most recently from c6668dd to 397bbb2 Compare July 6, 2023 14:29
@graingert graingert force-pushed the use-loop-factory-to-set-loop-type branch from 397bbb2 to 05dfece Compare July 6, 2023 16:18
Copy link
Member

@fjetter fjetter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +232 to +233
if loop_factory is None:
asyncio.set_event_loop(loop)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this not necessary when using a loop_factory?

Copy link
Member Author

@graingert graingert Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the behaviour from 3.11+ when loop_factory was introduced to asyncio.Runner

https://github.com/python/cpython/blob/b03755a2347325a89a48b08fc158419000513bcb/Lib/asyncio/runners.py#L136-L142

it isn't called when the loop_factory is specified because otherwise it would call asyncio.get_event_loop_policy().set_event_loop(loop) on the default policy with a loop from another policy which isn't supported

set_event_loop is currently only used by the posix _UnixDefaultEventLoopPolicy to set an eventloop for listening to child process events with a child process watcher, which is deprecated and not used on windows or uvloop where we use the loop_factory kwarg

Comment on lines +243 to +244
if loop_factory is None:
asyncio.set_event_loop(None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@graingert graingert merged commit de50c90 into dask:main Jul 13, 2023
24 of 27 checks passed
@graingert graingert deleted the use-loop-factory-to-set-loop-type branch July 13, 2023 14:06
@zanieb
Copy link
Contributor

zanieb commented Jul 13, 2023

Nice! Thank you @graingert :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import of Dask prevents async subprocess management on Windows
3 participants