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

Actually support uvloop in distributed #5531

Merged
merged 6 commits into from
Nov 23, 2021
Merged

Conversation

jcrist
Copy link
Member

@jcrist jcrist commented Nov 19, 2021

Previously distributed relied on the AnyThreadEventLoopPolicy,
which configuring uvloop would override, preventing distributed from
actually working with uvloop. We remove the need for
AnyThreadEventLoopPolicy and fixup uvloop configuration to fix this.

This also standardizes the .asynchronous and sync methods on classes
that support them. This is currently Client, Cluster, and
Semaphore for some reason. Event/Lock/... don't have these
methods, not sure why semaphore does, but I left it as is for now.

Fixes #5309, supersedes #5312.

@jcrist
Copy link
Member Author

jcrist commented Nov 19, 2021

oof, looks like we were accidentally relying on creating event loops in lots of places. More to do here.

@jcrist
Copy link
Member Author

jcrist commented Nov 19, 2021

Hmmm, this will now pass (at least on linux) any specific test, but fails on a full run. Will have to dig in more later.

Previously distributed relied on the AnyThreadEventLoopPolicy,
which configuring uvloop would override, preventing distributed from
actually working with uvloop. We remove the need for
`AnyThreadEventLoopPolicy` and fixup uvloop configuration to fix this.

This also standardizes the `.asynchronous` and `sync` methods on classes
that support them. This is currently `Client`, `Cluster`, and
`Semaphore` for some reason. `Event`/`Lock`/... don't have these
methods, not sure why semaphore does, but I left it as is for now.
@jcrist jcrist force-pushed the uvloop-actually-works branch from def51f6 to 538ab99 Compare November 22, 2021 18:36
@jcrist
Copy link
Member Author

jcrist commented Nov 22, 2021

I believe the windows failures are due to flaky tests. This should be ready for review.

@fjetter
Copy link
Member

fjetter commented Nov 23, 2021

The win py3.7 failure is a known flaky test.

I cannot classify the win py3.9 failure. It fails during teardown since some client instance is still around. I don't think this is a flaky test but it is hard to tell since there is also a timeout involved

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.

These changes look great. I think it's been long overdue to clean this up, especially the code you pulled out into SyncMethodMixin and in_async_call.

I'm not entirely sure about the win py3.9 test failure but otherwise this can be merged from my POV

@jcrist jcrist merged commit 6118683 into dask:main Nov 23, 2021
@jcrist jcrist deleted the uvloop-actually-works branch November 23, 2021 15:22
@jcrist
Copy link
Member Author

jcrist commented Nov 23, 2021

Thanks for the review Florian!

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.

get_client within a task creates a new (unused) event loop for every thread
2 participants