-
-
Notifications
You must be signed in to change notification settings - Fork 718
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
base: main
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? Admins can comment |
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 25 files ±0 25 suites ±0 10h 40m 24s ⏱️ + 25m 33s For more details on these failures and errors, see this check. Results for commit 1a07788. ± Comparison against base commit c3482ee. ♻️ 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( |
There was a problem hiding this comment.
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.
fcb7223
to
3a94185
Compare
Closes #4429. I sorta pick where #4431 left off and handle the conflicting case.
In detail:
DEFAULT_SCHEDULER_PORT
.0
(random port).The third point makes this overall more complicated, and exists just
to make the following work:
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" forLocalCluster
Tests added / passed
Passes
pre-commit run --all-files