-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 distributed
default clients even if no config is set
#9808
Conversation
if getattr(thread_state, "key", False): | ||
from distributed.worker import get_worker | ||
|
||
return get_worker().client.get |
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 is equivalent to the get_client call I introduced below if getattr(thread_state, "key", False)
is True so there is no need to have this additional indirection.
By having only a single call, we're changing precedence slightly s.t. a class providing the __dask_scheduler__
attribute wins but I believe this "more correct" considering it is passed explicitly. I doubt this has any relevance.
dask/base.py
Outdated
try: | ||
from distributed import get_client | ||
|
||
return get_client().get | ||
except ValueError: | ||
pass |
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.
i.e. if we're not requesting anything specific but are executing the default we will always try to get a global distributed client before checking the collections default.
The precedence there is
- Worker client
- Current client
there is effectively no performance penalty in case there is no distributed worker or client other than raise+except
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.
Thanks @fjetter
Test failure is #9793 |
distributed
default clients even if no config is set
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.
Thanks @fjetter
Partially addresses #9807
Will follow up with the symmetric change in distributed after merging.