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

Adding FedProx as another model example #161

Merged
merged 20 commits into from
Feb 21, 2025
Merged

Adding FedProx as another model example #161

merged 20 commits into from
Feb 21, 2025

Conversation

lotif
Copy link
Collaborator

@lotif lotif commented Feb 10, 2025

PR Type

Feature

Short Description

Clickup Ticket(s): https://app.clickup.com/t/868caa1b3

Adding MNIST with FedProx as another option for a model in order to have a more compelling demo.

Some refactorings were necessary to make it work because we were hardcoding the FedAvg strategy and the FlServer. Both of those are different for FedProx, so some things had to be moved around and made more flexible. Here is a summary of changes:

  • Renamed florist.api.servers.common module to florist.api.servers.models
  • Renamed MNIST model to MNIST with FedAvg
  • Created a ServerFactory class that will provide a way to instantiate the "server constructor" for each one of the models.
  • Moved the code that makes the strategy and the server to the get_fedavg_server, and created a get_fedprox_server function which will make FedProx's strategy and server
  • Created FedProxConfigParser with the required config attributes for FedProx

Tests Added

Fully unit and integration tested

@@ -1,37 +0,0 @@
"""Common functions and definitions for servers."""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module has been renamed to models.py, not sure why it's not showing as renamed here.

@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 91.86992% with 10 lines in your changes missing coverage. Please review.

Project coverage is 94.65%. Comparing base (27e0971) to head (375a4b0).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
florist/api/servers/models.py 92.75% 5 Missing ⚠️
florist/api/monitoring/metrics.py 60.00% 4 Missing ⚠️
florist/api/clients/mnist.py 94.44% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #161      +/-   ##
==========================================
+ Coverage   94.29%   94.65%   +0.35%     
==========================================
  Files          25       24       -1     
  Lines        2647     2711      +64     
  Branches      149      149              
==========================================
+ Hits         2496     2566      +70     
+ Misses        151      145       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -0,0 +1,213 @@
"""Functions and definitions for server models."""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the old common.py but with a lot of added functionality like the ServerFactory definition and the functions to construct the servers.

Copy link
Collaborator

@emersodb emersodb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are really thorough and I like the refactors you made. However, I worry that we're too tightly coupling model, dataset, server, and clients with the factories such that for every new combination of these, we'll have to add in a new enum entry etc?

@@ -45,13 +49,36 @@ def parse(cls, config_json_str: str) -> Dict[str, Any]:
return config


class FedProxConfigParser(BasicConfigParser):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice. It feels like a user should specify an over-arching approach in the configuration that dictates what parser to use some how.

Copy link
Collaborator

@emersodb emersodb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on our conversations about next steps in the refactors and flexibility with the various components required to designate a full set of client/server definitions, I think we're good to merge this in and move to those ideas. 🙂

@lotif lotif merged commit 87d6217 into main Feb 21, 2025
8 checks passed
@lotif lotif deleted the fedprox branch February 21, 2025 21:57
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

Successfully merging this pull request may close these issues.

4 participants