Add retry to shuffle broadcast #8900
Conversation
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 29m 27s ⏱️ + 7m 4s For more details on these failures and errors, see this check. Results for commit fc6095d. ± Comparison against base commit 1205a70. ♻️ This comment has been updated with latest results. |
|
Closes #8659 |
|
This got a little more ugly than I wanted it to but now we're also distinguishing real from OSError |
fjetter
force-pushed
the
shuffle_broadcast_retry
branch
from
b81591b to
04e74c9
Compare
| ({0: (5, OSError), 1: (1, OSError)}, P2PConsistencyError), | ||
| ], | ||
| ) | ||
| @pytest.mark.slow |
There was a problem hiding this comment.
All included this is about 2.5s so not incredibly slow. The backoff is just 0.1 after all. If that issue persists we can add configuration and an exponential backoff, etc. but I'd rather not increase complexity further (but rather would like to get rid of the broadcast instead)
| if isinstance(r, OSError): | ||
| workers.append(w) | ||
| else: | ||
| raise P2PConsistencyError( |
There was a problem hiding this comment.
Why raise a P2PConsistencyError here? I think we should just propagate the original error instead.
| if len(workers) == before: | ||
| no_progress += 1 | ||
| if no_progress >= 3: | ||
| raise P2PConsistencyError( |
There was a problem hiding this comment.
I would raise a different type of error here. The P2P run is still internally consistent, it just failed.
There was a problem hiding this comment.
are you fine with a generic RuntimeError?
There was a problem hiding this comment.
Yes, that would work. For context, I've imagined the P2PConsistencyError to be exclusively used for state/runtime inconsistencies. For transfer and unpack tasks, these actually cause us to reschedule the task instead of erring. (Come to think of it, I'm wondering if we should reschedule when encountering these in the barrier as well.)
There was a problem hiding this comment.
(Come to think of it, I'm wondering if we should reschedule when encountering these in the barrier as well.)
I think the retry should be sufficient for now. Either way, this is out of scope for this PR
There was a problem hiding this comment.
I just checked and realized that we do not reschedule upon P2PConsistencyErrors but raise hard. We only reschedule on ShuffleClosedError.
One of our P2P stress tests is failing pretty regularly
distributed.tests.test_stress.test_close_connectionsThis failure is somewhat expected because the broadcast is connecting to all workers and the connection attempts may time out if the workers are too busy.
This is not an ideal fix (instead, removing broadcast would be terrific).
I opted to not implement a retry in broadcast itself since this would've more serious implications and the broadcast API provides everything for users to implement this themselves
I'm still missing a test but confirmed this with some manual patches. @hendrikmakait if you have an idea on how to put together an easy test this would be helpful