-
-
Notifications
You must be signed in to change notification settings - Fork 717
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
Fix timeout in client.restart #4690
Conversation
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.
Good catch @mdwint! You're correct that timeout
in asyncio.wait_for
is the number of seconds to wait.
Could you add a small test to distributed/tests/test_client.py
to ensure that we're timing out as expected?
Also, I know it predates the changes in this PR, but could you add a call to parse_timedelta
so timeout=
can accept inputs like "10min", "13s" instead of just an integer or float number of seconds? For instance, here's an example of where we use parse_timedelta
in another location in client.py
:
distributed/distributed/client.py
Line 605 in be2d495
timeout = parse_timedelta(timeout, "s") |
Thanks for your guidance @jrbourbeau. I've added that test, as well as the call to |
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 looks great! Just a heads up, I pushed a small commit to use "0.5s"
instead of 0.5
in the test you added to make sure the parse_timedelta
addition is working as expected -- hope that's okay. Will merge once CI finishes
Also I noticed this was your first code contribution to this repository. Welcome! |
Thanks, @jrbourbeau! |
I noticed
client.restart()
would hang sometimes, and then I discovered the timeout passed toasyncio.wait_for
being expressed as a "deadline" instead. Am I correct in assuming this is wrong?