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

Add local SubprocessCluster that runs workers in separate processes #7431

Merged
merged 14 commits into from
Dec 29, 2022

Conversation

hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Dec 22, 2022

Closes #xxxx

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

@github-actions
Copy link
Contributor

github-actions bot commented Dec 22, 2022

Unit Test Results

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

       22 files  +       1         22 suites  +1   9h 53m 54s ⏱️ + 1m 54s
  3 282 tests +       4    3 192 ✔️ +       1       86 💤 +  1  4 +2 
36 033 runs  +1 332  34 488 ✔️ +1 269  1 541 💤 +62  4 +1 

For more details on these failures, see this check.

Results for commit 3a2d42d. ± Comparison against base commit d87cea9.

♻️ This comment has been updated with latest results.

@hendrikmakait
Copy link
Member Author

This is currently broken on Windows because we use the SelectorEventLoop on Windows, which does not support subprocesses (https://docs.python.org/3/library/asyncio-platforms.html#windows). Using SelectorEventLoop seems to be necessary due to an old issue with tornado (

if WINDOWS:
# WindowsProactorEventLoopPolicy is not compatible with tornado 6
# fallback to the pre-3.8 default of Selector
# https://github.com/tornadoweb/tornado/issues/2608
asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
).

Upgrading to tornado>=6.1 might resolve this, is there a reason #5833 has not been merged? cc @graingert

Copy link
Member

@mrocklin mrocklin left a comment

Choose a reason for hiding this comment

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

In principle this looks sensible to me. I would add basic docstrings.

Maybe also a single line mentioning this class at the end of https://docs.dask.org/en/stable/deploying-python.html#localcluster ?

distributed/deploy/subprocess.py Outdated Show resolved Hide resolved
@mrocklin
Copy link
Member

This is currently broken on Windows because we use the SelectorEventLoop on Windows, which does not support subprocesses (https://docs.python.org/3/library/asyncio-platforms.html#windows). Using SelectorEventLoop seems to be necessary due to an old issue with tornado (

For something like this that's already supporting an odd corner case, I think that it's totally fine to just raise an informative error message on Windows and move on.

@hendrikmakait
Copy link
Member Author

For something like this that's already supporting an odd corner case, I think that it's totally fine to just raise an informative error message on Windows and move on.

Added a descriptive error message 👌

@hendrikmakait hendrikmakait marked this pull request as ready for review December 23, 2022 13:33
@hendrikmakait hendrikmakait self-assigned this Dec 23, 2022
@graingert
Copy link
Member

Upgrading to tornado>=6.1 might resolve this, is there a reason #5833 has not been merged? cc @graingert

I was waiting for a bokeh release so it might be worth resurrecting that PR

@mrocklin
Copy link
Member

Thanks @hendrikmakait . Seems good to me. I'm going to wait until end of day to see if we get any user feedback and then I'll merge in.

@mrocklin mrocklin merged commit b5a2078 into dask:main Dec 29, 2022
@tasansal
Copy link

tasansal commented Jan 14, 2023

This is great; one benefit I can think of is the overhead of moving data from the scheduler process back to the client process disappears. What are other benefits/use cases? I can help test this further and add some more to the documentation, which is very little now. Is this eventually going to replace LocalCluster?

@mrocklin
Copy link
Member

mrocklin commented Jan 15, 2023 via email

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.

4 participants