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

• [x] Implement proposed change
• [ ] Write tests
• [x] 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?

[WilliamLindskog]

@jafermarq something we can merge? Went through the comments and seems like most things have been cleared.

[WilliamLindskog]

Hi everyone, after revisiting this internally and reviewing our latest examples, we think there’s an argument for making initial_parameters required again.

• Most of the examples already initialize and pass the global model manually when starting the server.
• Requiring initial_parameters explicitly would remove the hidden logic (sampling from random client) and make behavior more predictable and transparent.

Thus, @DanielCardeal's original idea (making initial_parameters non-optional) is the right direction for FedAvgM.

@DanielCardeal would you be interested in bringing this PR up-to-date with current codebase e.g. location of file?

[WilliamLindskog]

Hi again,

@DanielCardeal just checking in. Would you be interested in bringing this PR up-to-date with current codebase e.g. location of file?