-
-
Notifications
You must be signed in to change notification settings - Fork 725
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
Track reason of workers closing and restarting #7166
Conversation
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files ± 0 15 suites ±0 6h 11m 7s ⏱️ - 13m 47s For more details on these failures, see this check. Results for commit 459f0d3. ± Comparison against base commit 6afce9c. ♻️ This comment has been updated with latest results. |
distributed/nanny.py
Outdated
@@ -255,7 +256,7 @@ def __init__( # type: ignore[no-untyped-def] | |||
handlers = { | |||
"instantiate": self.instantiate, | |||
"kill": self.kill, | |||
"restart": self.restart, | |||
"restart": self.restart, # TODO: Is this being used anywhere? |
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.
Client.restart
uses this handler
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.
#6714 removed the call to Nanny.restart
in Scheduler.restart
which in turn is used by Client.restart
(see #6714 (comment) for the reason behind this decision). I see that #7154 reintroduces using Nanny.restart
, see #7154 (review) for my thoughts on that.
CI failures: |
This PR adds a
reason
kwarg to multiple places where we close/kill/restart workers to provide better insights into the causes of those.Note:
This basically implements a poor man's of tracing, we may want to look into #6201, #4762 and #4718 to improve upon this with less manual repetition.
pre-commit run --all-files