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

• Validate OpenFL Security through regression testing

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,

[ishant162]

Accepted. Will incorpoate

[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.

[ishant162]

Accepted. Will incorpoate

[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.

[ishant162]

Incorporated the suggestion you provided.

[MasterSkepticista]

Why is CutoffPolicy mentioned here?

[ishant162]

To showcase the CutoffPolicy's start_policy implementation as a reference for users.

[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.

[ishant162]

Accepted. Will incorpoate

[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.

[ishant162]

The arguments specified are intended for the constructor rather than the class attributes, so they are correctly placed. I have removed the kwargs as per your suggestion.

[MasterSkepticista]

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

[ishant162]

IMO, assert statements are primarily intended for debugging purposes.
Using raise with a ValueError more clearly indicates that the error is due to an invalid argument

[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.

[ishant162]

Accepted. Will incorporate

[MasterSkepticista]

Save lines!

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

[ishant162]

Accepted. Will incorporate

[MasterSkepticista]

Is this default? What is the default behavior?

[ishant162]

Yes. In case no policy is specified in plan then straggler handling policy is disabled.