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

Add a Redis hook to update status #40

Merged
merged 31 commits into from
May 23, 2024
Merged

Add a Redis hook to update status #40

merged 31 commits into from
May 23, 2024

Conversation

lotif
Copy link
Collaborator

@lotif lotif commented May 13, 2024

PR Type

Feature

Short Description

Clickup Ticket(s): https://app.clickup.com/t/8688aj7hy

First of all, sorry for the lengthy PR :)

The functionality is a bit complex and also I used the opportunity to do some refactoring. Below is the list of things in this PR.

Refactoring:

  • Moved the code to get data from Redis to its own function in metrics.py
  • Moved all the database queries for the job into the Job entity. The idea is to kinda follow the DAO design pattern and have all database operations centralized inside the modules in the db folder.
  • I had to add a synchronous pymongo.MongoClient database connection (in addition to the Motor one) because FastAPI's BackgroundTask does not work with async functions 😢

Main work:

  • Added a call to publish a message to a Redis channel when data is saved in the RedisMetricsReporter
  • Added a get_subscriber function in metrics.py
  • Added server_uuid and server_metrics to the Job, as well as uuid and metrics to ClientInfo.
  • Changed the training/start endpoint to:
    • Check the job status is NOT_STARTED
    • Change the job status to IN_PROGRESS once the training is kicked off
    • Save the server and client UUIDs in the database
    • Added a background task to monitor the Redis channel for the current server_uuid. It will have a listener to that channel which will keep checking if the metrics have fit_end and once they do, change the status of the job to FINISHED_SUCCESSFULLY and save the client and server metrics in the database.

Tests Added

Fully integration- and unit-tested, so a whole lot.

@lotif lotif requested review from jewelltaylor and emersodb May 13, 2024 19:55
@codecov-commenter
Copy link

codecov-commenter commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.57%. Comparing base (808ea53) to head (c5e826c).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
+ Coverage   90.01%   91.57%   +1.55%     
==========================================
  Files          20       20              
  Lines         591      700     +109     
  Branches       11       11              
==========================================
+ Hits          532      641     +109     
  Misses         59       59              

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

Copy link
Collaborator

@jewelltaylor jewelltaylor left a comment

Choose a reason for hiding this comment

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

All in all, it looks pretty good to me! I only included a few small suggestions and also a few clarifying questions. Once you have a chance to respond, I will take another pass through and if nothing comes up we can get this merged in :)

@lotif
Copy link
Collaborator Author

lotif commented May 22, 2024

@jewelltaylor thanks for the review! I've addressed your comments, please check again.

Copy link
Collaborator

@jewelltaylor jewelltaylor left a comment

Choose a reason for hiding this comment

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

LGTM! As always, sweet tests :)

@lotif lotif merged commit 5b64bd9 into main May 23, 2024
8 checks passed
@lotif lotif deleted the redis-pub-sub branch May 23, 2024 14:56
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