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

Upgrade fl4heath and made appropriate changes #110

Merged
merged 14 commits into from
Nov 7, 2024
Merged

Conversation

jewelltaylor
Copy link
Collaborator

PR Type

[Feature | Fix | Documentation | Other ]

Short Description

Clickup Ticket(s): Upgrade FL4Health Version

The current version depends on fl4health = "^0.1.15". fl4health 0.2.1 was just released. Since then a few things have changed, mostly in regards to reporter functionality, dependencies and utilities being shuffled around. Marcelo also needs the upgraded FL4Health for #104.

Most of the diff is stems from the poetry.lock. Otherwise, just some slight modifications had to be made to account for the new reporter and other small changes.

Tests Added

...

from flwr.server import ServerConfig


DEFAULT_FORMATTER = logging.Formatter("%(levelname)s %(name)s %(asctime)s | %(filename)s:%(lineno)d | %(message)s")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess flwr removed the DEFAULT_FORMATTER they were exporting previously.

Here there reference it in the documentation so I just sourced it from there: https://flower.ai/docs/framework/how-to-configure-logging.html

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 89.13043% with 5 lines in your changes missing coverage. Please review.

Project coverage is 94.20%. Comparing base (d0604c4) to head (c9c1f39).

Files with missing lines Patch % Lines
florist/api/monitoring/metrics.py 90.00% 3 Missing ⚠️
florist/api/clients/mnist.py 66.66% 1 Missing ⚠️
florist/api/servers/utils.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #110   +/-   ##
=======================================
  Coverage   94.20%   94.20%           
=======================================
  Files          23       23           
  Lines        2087     2107   +20     
  Branches      177      119   -58     
=======================================
+ Hits         1966     1985   +19     
- Misses        121      122    +1     

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

Copy link
Collaborator

@lotif lotif left a comment

Choose a reason for hiding this comment

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

Looks mostly good, thanks for putting the effort to upgrade!! I'll just wait for you to fix the failing test and I'll approve it :)

@@ -60,7 +62,6 @@ def start_server(
config=ServerConfig(num_rounds=n_server_rounds),
)
server.shutdown()
server.metrics_reporter.dump()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we are dumping on every call to report I figured we can remove these?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess, yes... I put it there just in case, it doesn't hurt, but I guess it's better to remove if it's not really necessary :)

Copy link
Collaborator

@lotif lotif left a comment

Choose a reason for hiding this comment

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

Small comment, but otherwise looks good :)

florist/api/db/entities.py Show resolved Hide resolved
@jewelltaylor jewelltaylor requested a review from lotif November 7, 2024 16:15
@jewelltaylor
Copy link
Collaborator Author

Small comment, but otherwise looks good :)

Should be good to go @lotif! Let me know if there are any issues.

Copy link
Collaborator

@lotif lotif left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

@jewelltaylor jewelltaylor merged commit 59c8cef into main Nov 7, 2024
8 checks passed
@jewelltaylor jewelltaylor deleted the upgrade-fl4health branch November 7, 2024 17:32
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.

3 participants