Straggler handling Follow-up

[ishant162]

This pull request is a follow-up to the Straggler Handling PR #996 . Based on the feedback from @MasterSkepticista and @teoparvanov , the review comments have been addressed and incorporated accordingly.

Summary of changes:

• Relocated straggler_handling to the openfl/component/aggregator directory.
• Consolidated all policies into openfl/component/aggregator/straggler_handling.py
• Renamed CutoffTimeBasedStragglerHandling to CutoffPolicy and PercentageBasedStragglerHandling to PercentagePolicy
• Removed the attribute deletion functionality from CutoffPolicy

PR is currently in draft

[teoparvanov]

@ishant162 , did you test for regression by raising a PR in OpenFL-Security?

[scngupta-dsp]

@ishant162 , did you test for regression by raising a PR in OpenFL-Security?

This needs to be done @teoparvanov . We will feedback once it is done

[MasterSkepticista]

This is a good improvement over the previous layout.

Most suggestions, though seem like nitpicks, have measurable changes on developers and users in the look-and-feel of the framework. Please take a look.

[MasterSkepticista]

Add a typehint.

straggler_handling_policy: BasePolicy = CutoffPolicy,

[MasterSkepticista]

Suggest removing the mention of default. Most IDEs show this when defaults are provided. One less line to maintain if default changes in the future.

[MasterSkepticista]

I suggest creating a DefaultPolicy. This simplifies:

1. self.straggler_handling_policy = straggler_handling_policy() regardless of what is given, saves 4 lines.
2. DefaultPolicy would be the equivalent of providing no straggler handling capabilities. Dummy class.

Also the condition check is wrong. What if PercentagePolicy is provided? It will go to else block and fail in code, it seems.

[MasterSkepticista]

Why is CutoffPolicy mentioned here?

[MasterSkepticista]

If this is the case for subclassed policies, please eliminate empty lines. It suffices to say Resets policy for the next round and not using 6 lines to say it takes or returns nothing. The -> None and no args make it obvious to the user in an IDE.

Also on the info itself: It is better to mention what exactly is being reset without leaking into the detail. Example Resets countdown timer for collaborator reporting until next round, or along those lines.

[MasterSkepticista]

Class docstring should go near attributes, not in __init__. Example:

class PercentagePolicy(StragglerHandlingPolicy):
    """Percentage based Straggler Handling function.

    Args:
            percent_collaborators_needed (float, optional): The percentage of
                collaborators required before concluding the round. Defaults to 1.0.
            minimum_reporting (int, optional): The minimum number of
                collaborators that should report, regardless of percentage threshold. Defaults to 1.
            **kwargs: Variable length argument list.
    """

    def __init__(self, percent_collaborators_needed=1.0, minimum_reporting=1, **kwargs):
        if minimum_reporting <= 0:
            raise ValueError("minimum_reporting must be >0")

        self.percent_collaborators_needed = percent_collaborators_needed
        self.minimum_reporting = minimum_reporting
        self.logger = getLogger(__name__)

On the **kwargs: Why would a user provide this? Where does it get used, please mention like keyword arguments forwarded to some.module.class). If they are never forwarded or stored (which seems to be the case looking at certain classes, they should not be accepted from the user either.

[MasterSkepticista]

One liner: assert minimum_reporting > 0, "Minimum reporting must be >0"

[MasterSkepticista]

Can we start defining logger = getLogger(__name__) below imports and use it globally within this file?
We should slowly move away from these tiny design choices.

[MasterSkepticista]

Save lines!

def reset_policy_for_round(self) -> None:
  """Ignored in `PercentagePolicy`."""
  pass

[MasterSkepticista]

Is this default? What is the default behavior?