-
Notifications
You must be signed in to change notification settings - Fork 0
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
Instrument metrics reporting to Redis #9
Conversation
# Conflicts: # poetry.lock # pyproject.toml
# Conflicts: # poetry.lock # pyproject.toml
@@ -1,4 +1,4 @@ | |||
name: tests | |||
name: integration-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry i should have done this! didnt think to split them up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problems! Things were still in flux with my open PR on aieng-template changes.
from flwr.common.logger import log | ||
|
||
|
||
class RedisMetricsReporter(MetricsReporter): # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious why we have to type ignore this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhh no output folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was this weird error:
https://stackoverflow.com/questions/49888155/class-cannot-subclass-qobject-has-type-any-using-mypy
I tried solving it a couple of different ways without luck, so I ignored it. Seems to be something mypy is not picking up from fl4health's package settings and not really a big issue. Not sure if it's worth trying to find a fix in fl4health. We can also disable this check if it becomes too annoying.
""" | ||
Init an instance of RedisMetricsReporter. | ||
|
||
:param redis_connection: (redis.client.Redis) the connection object to a Redis. Should be the output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed you are using sphinx docstring format and I used the google format in the last PR. I don't have a strong preference and would be happy to adopt sphinx, do you think we should use the same format? Or does it not matter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just finished setting up automatic documentation with sphinx (following aieng-template changes) so I think we should stick to the sphinx format.
https://vectorinstitute.github.io/FLorist/reference/api/florist.api.launchers.launch.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhhhh I seee, makes sense! I will keep that in mind going forward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one comment to clarify if we should stay consistent on docstring format. Other than that, everything looks great and runs on my local machine
PR Type
Feature
Short Description
Clickup Ticket(s): https://app.clickup.com/t/8687c8jj1
Adding a subclass of
MetricsReporter
that saves the metrics to Redis instead of a file.Tested by starting a server with it:
After training, check if the metrics have been saved to Redis successfully:
Tests Added
Unit tests for RedisMetricsReporter.