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

split/rework clients #723

Open
6 tasks
tharvik opened this issue Jul 23, 2024 · 1 comment
Open
6 tasks

split/rework clients #723

tharvik opened this issue Jul 23, 2024 · 1 comment
Milestone

Comments

@tharvik
Copy link
Collaborator

tharvik commented Jul 23, 2024

there is currently a mix between a Client communicating with the server and Clients training on a given task.

  • create a new dedicated ServerClient which can list tasks and upon joining one returns a specific DecentralizedClient or FederatedClient
    • allow for clearer path on where the model is created
    • allow to have a varied set of Task as this would be the only way to get theses (see split default tasks from core #647)
    • separation of concerns, what else
  • DecentralizedClient and FederatedClient would be split in multiple clients
    • {Decentralized,Federated}TrainingClient which only do the training
    • SecureAggregatorClient for secure aggregation (see aggregator rework #722)
    • allows for more "local" messaging and general clearer names of messages/fields
  • straighten up client usage by avoiding onRound* callbacks indirections
  • make it dumber
    • should not wrap an Aggregator
    • simply expose network calls, such as sendLocalWeights(weights: WeightsContainer) & receiveNetworkWeights*(): AsyncGenerator<WeightsContainer>
@tharvik tharvik added this to the v4.0.0 milestone Jul 23, 2024
@JulienVig
Copy link
Collaborator

JulienVig commented Jul 24, 2024

I think this would create a lot of Client classes with non-self-explanatory relationships. It is unclear if the word client refers to a federated user (usually called a client) or how it's currently used, which is an object only handling communication with server/peers.

Additionally, upon onboarding disco I found it very unintuitive that the Client class represented only the communication logic while I was expecting it to represent a whole user/peer partaking in distributed learning (which is the Disco object)

Some raw ideas to remove/clarify the use of "client":

ServerClient -> TaskUserBuilder

TaskUserBuilder would returns a DiscoUser (equivalent to teh current Disco or the {Decentralized,Federated}Client you are suggesting if I understood correctly).

{Decentralized,Federated}Client -> DecentralizedPeer and FederatedClient

In order to use respective naming conventions and clarify the meaning of Client. Both inherent DiscoUser.

{Decentralized,Federated}TrainingClient -> {Decentralized,Federated}Trainer

Remove the use of client. A DecentralizedPeer has a DecentralizedTrainer while a FederatedClient has a FederatedTrainer. I think these classes should handle the collaborative weight sharing schemes.

Current Client -> NetworkHandler

Simplify the current Client objects handling communication and distribution logic to only handle network communication. Can be subclassed into FederatedNetworkHandler and DecentralizedNetworkHandler. As mentioned above, the collaborative weight update logic should be handled in the {Decentralized,Federated}Trainer and not in this one (as it currently is)

Sorry I got way too involved in this, let me know what you think. It may be easier to have a call about it.

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

2 participants