Set initial_parameters as non-optional for FedAvgM (#2369)

[DanielCardeal]

Issue

#2369

Description

The constructor parameter initial_parameters of the FedAvgM strategy is typed as Optional, but is required whenever server-side optimization is enabled. This can be misleading, since server-side optimization is enabled "automatically" and in a non-transparent way.

Related issues/PRs

Proposal

Change type of intial_parameters from Optional[Parameters] to Parameters in FedAvgM. Also, documents which conditions must be met for server-side optimization to be enabled.

Explanation

This change aims to better communicate to users the necessity of initial_parameters in case of server-side optimization, as well as expose which conditions must be met to enable server-side optimization.

Checklist

• Implement proposed change
• Write tests
• Update documentation
• Update changelog
• Make CI checks pass
• Ping maintainers on Slack (channel #contributions)

Any other comments?

[jafermarq]

Hey @DanielCardeal, thanks for helping with the issue! The proposed change sounds reasonable. Would it make sense to keep it as optional and, in the first round, override the configure_fit() to initialise a local copy of the global parameters? (for context, when the initial_parameters are not set, the sever will sample one random client for its model parameters and those will become the initial state of the global model.)

I presume it's easier for people to not worry with the conversion of their model (e.g. PyTorch) to the parameters type when instantiating their strategy. I think this is more user-friendly. On the other hand, the solution you propose seems cleaner.

Any thoughts on this @danieljanes @charlesbvll @adam-narozniak @panh99 @gubertoli

[gubertoli]

@jafermarq, I aggree that from a user perspective retrieving the parameters from a random client would be much easier.

If possible (probably as a new issue) doing the same for FedOpt would also be great. Initializing these parameters is a recurrent question in Slack.

[DanielCardeal]

@jafermarq @gubertoli Great idea!

I will try to add this changes in the the next couple of days :)

[adam-narozniak]

Hi, great catch. I'm in favor of @jafermarq 's idea.

I think we should also add a warning saying if at least one of the default values for momentum_vector or server_learning_rate is not changed, then it is just a vanilla FedAvg running. I think this will help the users to understand that aspect too.

[DanielCardeal]

Hi

I added the following changes to the PR:

• Automatic initialization of intial_parameters using the global model
• Warn users when either server_momentum and server_learning_rate are set to default values and server side optimization is disabled.

Are there any further changes that need to be implemented?

[jafermarq]

Hi @DanielCardeal,

just a minor comment. I think we still want to not force users to pass initial_parameters.

I like the WARNING_NO_SERVER_OPT message you wrote to alert users. Do we want it to be displayed right upon strategy instantiation? Maybe yes? thoughts @adam-narozniak @danieljanes
It looks something like the below:

python run.py
WARNING flwr 2023-11-16 16:11:34,348 | fedavgm.py:132 | 
Setting both `server_momentum` and `server_learning_rate` to default values
cause FedAvgM to work as a vanilla FedAvg strategy. Server optimization with
momentum is enabled if either `server_momentum` is set to a value greater than
0.0 or `server_learning_rate` is set to a value lower than 1.0.

INFO flwr 2023-11-16 16:11:34,349 | app.py:178 | Starting Flower simulation, config: ...
....