-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Log worker_client
event
#8819
Log worker_client
event
#8819
Conversation
distributed/worker_client.py
Outdated
"timeout": timeout, | ||
"separate_thread": separate_thread, |
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.
Not sure how useful this extra info actually is
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 22m 24s ⏱️ + 7m 15s For more details on these failures, see this check. Results for commit 26a8afe. ± Comparison against base commit ad5f98c. ♻️ This comment has been updated with latest results. |
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.
The change itself looks good to me, but it's missing the case that is more critical to me:
@delayed
def bla(i):
arr = da.random.random((500, 500, 100), chunks=(10, 10, 10))
result = dask.compute(arr.sum())
return result
if __name__ == "__main__":
cluster = LocalCluster()
client = cluster.get_client()
f = bla(1)
dask.compute(f)
the accidental worker client creation. I think this happens somewhere in _get_client
on the worker
Ah, yeah, we definitely want to capture that case too. I've updated things to capture both the implicit client creation case via |
Could we log an event where you have it now and then a different event in the worker_client context manager? Subtracting both counters would then tell us if it's intentional or accidental? |
Sure, we now have a |
thx |
Using
worker_client
is supported but using them well can require some care. This PR makes sure we log an event when creating a client viaworker_client
, which can be used later when debugging, etc.