Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aggregator rework #722

Open
5 tasks
tharvik opened this issue Jul 23, 2024 · 4 comments
Open
5 tasks

aggregator rework #722

tharvik opened this issue Jul 23, 2024 · 4 comments
Milestone

Comments

@tharvik
Copy link
Collaborator

tharvik commented Jul 23, 2024

we currently have two aggregators

  • mean which perform a mean of cleartext weights
    • also allow for weights over multiple rounds
  • secure which do a sum-mask of the weights

it is quite hard to follow where and how the aggregated weights are generated.

  • get rid of communication rounds which interact weirdly with Client
    • pass it an object with only the needed methods
  • have a better naming than "robust" or "secure" which are confusing
  • do not mutate aggregator while it's running (no Aggregator.add), not externally
  • should be started at beginning of round and stopped at the end, yielding the network weights if available
    • cross round support is another matter, we still don't have a good fix for slow nodes as we don't really have a catchup mechanism
@tharvik tharvik added this to the v4.0.0 milestone Jul 23, 2024
@martinjaggi
Copy link
Member

sounds good, just a quick comment that robust in our context always means Byzantine robust basically, so seems fine i guess.

ah and the robust aggregator currently inactive, did it use median, or mean after clipping?

@tharvik
Copy link
Collaborator Author

tharvik commented Jul 23, 2024

just a quick comment that robust in our context always means Byzantine robust basically, so seems fine i guess.

yeah, we just need to be more descriptive with the names, "SecureAggregator" is not really clear, smth like MaskedWeightsAggregator and ByzantineWeightsRobutsAggregator perhaps

ah and the robust aggregator currently inactive, did it use median, or mean after clipping?

I think that it is using mean of the centered weights. (not very clear to me what a centered weights but I guess it's taken from the paper itself)

@tharvik tharvik mentioned this issue Jul 23, 2024
6 tasks
@JulienVig
Copy link
Collaborator

WDYT of having a separate aggregator (or whatever may replace it) dedicated to the federated clients? Since they are always expecting a single update from the server and simply overwrite their local weights with the global ones, I don't think it makes sense to use a mean aggregator the way it currently is.

@tharvik
Copy link
Collaborator Author

tharvik commented Jul 24, 2024

having a separate aggregator (or whatever may replace it) dedicated to the federated clients?

yeah, that clearly make sense. I won't even be called "aggregator" on the federated client side, it will simply be the way we communicate weights to the server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants