adap
FedAvgM expecting initial_parameters as Optional
Describe the bug
The current implementation of FedAvgM strategy considers the initial_parameters as Optional in the class init method (here):
initial_parameters: Optional[Parameters] = None
However, the FedAvgM strategy requires the initial_parameters (here):
assert (
self.initial_parameters is not None
), "When using server-side optimization, model needs to be initialized."
initial_weights = parameters_to_ndarrays(self.initial_parameters)
Steps/Code to Reproduce
strategy = fl.server.strategy.FedAvgM()
fl.simulation.start_simulation(
client_fn=client_fn,
num_clients=NUM_CLIENTS,
config=fl.server.ServerConfig(num_rounds=5),
strategy=strategy,
)
Expected Results
To run the a simulation with vanilla FedAvgM strategy without initial_parameters.
Actual Results
Assert exception: "When using server-side optimization, model needs to be initialized."
As far as I can tell, initial_parameters are required when using server side optimization in the FedAvgM strategy, as indicated by this if statement above the assertion clause.
Server side optimization is enabled whenever server_learning_rate or server_momentum constructor parameters are set to values different from the defaults (here). As such, calling the FedAvgM constructor without any parameters works fine, while calling it with server_learning_rate = 0.9, for example, don't.
Considering all of that, it could be nice to add this information to the constructors' documentation, as to avoid confusion in the future.
I guess the assert is an explanatory point and agree with @DanielCardeal where the user can understand that if there is a server side optimisation , the initial_parameters should be done , but , for more clarity we can add it in the docstrings of the strategy class
Yes, @gubertoli.
It also seems a bit counterintuitive to allow users to initialize the strategy without initial_parameters since, in such cases, both FedAvg and FedAvgM aggregate the results in the exact same way.
I propose the changes in #2416 as a possible fix.
Looking at how we can merge this, see new comment in https://github.com/adap/flower/pull/2416.
