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

Use default scheduler port for LocalCluster #8760

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tmi
Copy link

@tmi tmi commented Jul 11, 2024

Closes #4429. I sorta pick where #4431 left off and handle the conflicting case.

In detail:

  1. Unify multiple occurrences of 8786 into a single constant DEFAULT_SCHEDULER_PORT.
  2. Change the default for the local cluster to use that instead of 0 (random port).
  3. Introduce a fallback address for scheduler start in case of no port having been given.

The third point makes this overall more complicated, and exists just
to make the following work:

>>> c1 = LocalCluster() # starts on 8786
>>> c2 = LocalCluster() # prints warning, starts on random port

Without the third point, it would crash on port collision (unlike the current main which has the random default and thus works).
The behaviour of dask-scheduler command is not changed -- running two at once crashed before, and still crashes.

Overall this PR attempts to be unifying:

  • default 8786 is more consistently used,

  • port binding for dashboard and scheduler port follows the same fallbacking logic,
    while keeping the defaults "strict" for dask-scheduler command and "benevolent" for LocalCluster

  • Tests added / passed

  • Passes pre-commit run --all-files

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

Copy link
Contributor

github-actions bot commented Jul 11, 2024

Unit Test Results

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

    29 files  ±0      29 suites  ±0   11h 23m 18s ⏱️ + 11m 59s
 4 083 tests ±0   3 962 ✅  -  4    112 💤 ±0   8 ❌ + 3   1 🔥 + 1 
55 230 runs  ±0  52 742 ✅  - 50  2 432 💤  - 1  42 ❌ +37  14 🔥 +14 

For more details on these failures and errors, see this check.

Results for commit 3a94185. ± Comparison against base commit b802237.

♻️ This comment has been updated with latest results.

fallback_port_or_addr = kwargs.get("fallback_port_or_addr", None)
if not fallback_port_or_addr:
raise
warnings.warn(
Copy link
Author

Choose a reason for hiding this comment

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

this line is what causes the test distributed/deploy/tests/test_local.py::test_Client_twice to fail, if I just comment the warning, it passes. Should I remove the warning, change it to logger invocation instead, or is there some means of making the test accept a warning occurrence?

1. Unify multiple occurrences of 8786 into a single constant
   `DEFAULT_SCHEDULER_PORT`.
2. Change the default for the local cluster to use that instead
   of `0` (random port).
3. Introduce a fallback address for scheduler start in case of no
   port having been given.
@tmi tmi force-pushed the localClusterDefaultPort branch from fcb7223 to 3a94185 Compare July 11, 2024 16:27
@tmi tmi marked this pull request as ready for review July 11, 2024 17:28
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.

LocalCluster not using default ports
2 participants