Skip to content

Add local SubprocessCluster that runs workers in separate processes#7431

Merged
mrocklin merged 14 commits into
dask:mainfrom
hendrikmakait:subprocess-cluster
Dec 29, 2022
Merged

Add local SubprocessCluster that runs workers in separate processes#7431
mrocklin merged 14 commits into
dask:mainfrom
hendrikmakait:subprocess-cluster

Conversation

@hendrikmakait

@hendrikmakait hendrikmakait commented Dec 22, 2022

Copy link
Copy Markdown
Member

Closes #xxxx

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

@github-actions

github-actions Bot commented Dec 22, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

@mrocklin mrocklin left a comment

Copy link
Copy Markdown
Member

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 ?

Comment thread distributed/deploy/subprocess.py Outdated
@mrocklin

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

tasansal commented Jan 14, 2023

Copy link
Copy Markdown

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

mrocklin commented Jan 15, 2023 via email

Copy link
Copy Markdown
Member

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