Updates to straggler handling functionality #996
Conversation
Signed-off-by: Parth Mandaliya <[email protected]>
…lingFunction abstract class. Renamed start_timer and __timer_expired functions to start_straggler_cutoff_timer and _straggler_cutoff_time_elapsed respectively. Added docstring to both functions mentioned above. Signed-off-by: Parth Mandaliya <[email protected]>
If one or more collaborator(s) does not even 1 task results in time, all tasks results sent by that collaborator is excluded from aggregation. Signed-off-by: Parth Mandaliya <[email protected]>
removing start_straggler_cutoff_timer function from parent class StragglerHandlingFunction Signed-off-by: Parth Mandaliya <[email protected]>
Signed-off-by: Parth Mandaliya <[email protected]>
Signed-off-by: Parth Mandaliya <[email protected]>
Signed-off-by: Parth Mandaliya <[email protected]>
…lts for all tasks Changed straggler handling logs Added docstring for functions in all straggler handling policies Signed-off-by: Parth Mandaliya <[email protected]>
2. CutoffTimeBasedStragglerHandling: start_policy method implements a timer to wait
for cutoff-time and then call provided call callback method
3. Aggregator:
- sendlocaltaskresults: update collaborators_done to keep track of collaborators
that have finished ALL tasks
- _straggler_cutoff_time_elapsed: call back function that is called after cutoff
time has elapsed and applies the straggler policy
Signed-off-by: Parth Mandaliya <[email protected]>
Added logger argument to get_straggler_handling_policy in plan.py Signed-off-by: Parth Mandaliya <[email protected]>
Signed-off-by: Parth Mandaliya <[email protected]>
Signed-off-by: Parth Mandaliya <[email protected]>
Signed-off-by: Parth Mandaliya <[email protected]>
Signed-off-by: Parth Mandaliya <[email protected]>
minimum_reporting cannot be set 0 in any straggler policy Signed-off-by: Parth Mandaliya <[email protected]>
Signed-off-by: Parth Mandaliya <[email protected]>
Signed-off-by: Parth Mandaliya <[email protected]>
Logs updated Logger argument removed from straggler handling policy classes Signed-off-by: Parth Mandaliya <[email protected]>
Signed-off-by: Parth Mandaliya <[email protected]>
collaborator are completed or not Signed-off-by: Parth Mandaliya <[email protected]>
Signed-off-by: Parth Mandaliya <[email protected]>
Signed-off-by: Parth Mandaliya <[email protected]>
Signed-off-by: Parth Mandaliya <[email protected]>
…se class. Signed-off-by: Parth Mandaliya <[email protected]>
There was a problem hiding this comment.
I think the code in this PR would work, but it introduces some tech debt - esp. with regards to some encapsulation "leaks" of the straggler policy objects. I've suggested some improvements in this first review pass, but it may be necessary to further revise the straggler policy interface.
| # Stop timer from restarting until next round | ||
| self.straggler_handling_policy.is_timer_expired = True |
There was a problem hiding this comment.
It would be cleaner if the policy object itself sets is_timer_expired to True when appropriate. It doesn't seem safe to leave this responsibility with the callback function, as the implementor may not be aware of it.
| @@ -5,15 +5,49 @@ | |||
|
|
|||
| from abc import ABC | |||
| from abc import abstractmethod | |||
| from typing import Callable | |||
|
|
|||
|
|
|||
| class StragglerHandlingFunction(ABC): | |||
There was a problem hiding this comment.
This is no longer a pure functional interface - I'd suggest to rename it to StragglerHandlingPolicy
| if ( | ||
| not hasattr(self.straggler_handling_policy, "round_start_time") and | ||
| self.straggler_handling_policy.straggler_cutoff_check( | ||
| len(self.collaborators_done), self.authorized_cols | ||
| ) | ||
| ): |
There was a problem hiding this comment.
Regardless of the actual policy implementation, shouldn't we always add the lagging collaborators to the stragglers list (as soon as the straggler_cutoff_check triggers)?
| raise NotImplementedError | ||
|
|
||
| @abstractmethod | ||
| def straggler_cutoff_check(self, **kwargs) -> bool: |
There was a problem hiding this comment.
Right now it's not clear what kind of inputs this method expects. Why not define it a bit more explicitly, as in:
def straggler_cutoff_check(self, num_collaborators_done, all_collaborators, **kwargs) -> bool:or even
def straggler_cutoff_check(self, num_collaborators_done, num_all_collaborators, **kwargs) -> bool:(for those policies, we most likely only need the list lengths, rather than the actual collaborator names)
This PR to resolves issue #981
Summary of changes
Verification performed
Tested various scenarios for time based cutoff straggler handling policy
All Director based workflow and Aggregator based workflow test-cases