aggregator rework - Githubissues

epfml / disco

DISCO is a code-free and installation-free browser platform that allows any non-technical user to collaboratively train machine learning models without sharing any private data.

https://discolab.ai

Apache License 2.0

141 stars 25 forks source link

aggregator rework #722

Open tharvik opened 1 month ago

tharvik commented 1 month ago

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

martinjaggi commented 1 month ago

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 commented 1 month ago

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)

JulienVig commented 1 month ago

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 commented 1 month ago

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.