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.
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
force-pushed
the
localClusterDefaultPort
branch
from
fcb7223 to
3a94185
Compare
| host=None, | ||
| ip=None, | ||
| scheduler_port=0, | ||
| scheduler_port=None, |
There was a problem hiding this comment.
Sorry, why is this changed to None from 0?
There was a problem hiding this comment.
so 0 stands for "random port", whereas None stands for "user did not give value, and wishes for default behaviour" -- we can thus decide later whether default behaviour means "use the default hardcoded port" (as I did in this PR) or "use random port" (eg if that would be for any reason preferred in a later PR). Leaving 0 makes it undistinguishable between "explicitly desiring random port" and "implicitly desiring default behaviour"
There was a problem hiding this comment.
Is this documented somewhere? We could probably use some Enum between the two choices?
There was a problem hiding this comment.
the default port itself is mentioned somewhere in the docs. Going with enum is possible, though I'd not call it worth it in this case, I mean, the None => default is a popular idiom across python packages
mind that this PR is just meant as a small incremental improvement to unify behaivours a bit
There was a problem hiding this comment.
OK, fair enough. Then maybe we can add a comment to clarify what 0 and None mean in this context.
4 participants
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
mainwhich has the random default and thus works).The behaviour of
dask-schedulercommand 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-schedulercommand and "benevolent" forLocalClusterTests added / passed
Passes
pre-commit run --all-files