-
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
Changes from all commits
87ceb10
584c8d8
23dc51e
a218c9c
c06e7b4
9c78fe5
47c5d6e
17bd428
6829a04
ab1c2db
fb996a0
6378929
4fcfd62
18ebd79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
name: tests | ||
name: integration-tests | ||
|
||
on: | ||
push: | ||
|
@@ -33,7 +33,7 @@ on: | |
- '**.md' | ||
|
||
jobs: | ||
python-tests: | ||
integration-tests: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/[email protected] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
name: tests | ||
name: unit-tests | ||
|
||
on: | ||
push: | ||
|
@@ -33,7 +33,7 @@ on: | |
- '**.md' | ||
|
||
jobs: | ||
python-tests: | ||
unit-tests: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/[email protected] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
"""Classes for the instrumentation of metrics reporting from clients and servers.""" | ||
import json | ||
from logging import DEBUG | ||
from typing import Any, Dict, Optional | ||
|
||
import redis | ||
from fl4health.reporting.metrics import DateTimeEncoder, MetricsReporter | ||
from flwr.common.logger import log | ||
|
||
|
||
class RedisMetricsReporter(MetricsReporter): # type: ignore | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. It was this weird error: 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. |
||
"""Save the metrics to a Redis instance while it records them.""" | ||
|
||
def __init__( | ||
self, | ||
redis_connection: redis.client.Redis, | ||
run_id: Optional[str] = None, | ||
): | ||
""" | ||
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 commentThe 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 commentThe 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 commentThe 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 |
||
of redis.Redis(host=host, port=port) | ||
:param run_id: (Optional[str]) the identifier for the run which these metrics are from. | ||
It will be used as the name of the object in Redis. Optional, default is a random UUID. | ||
""" | ||
super().__init__(run_id) | ||
self.redis_connection = redis_connection | ||
|
||
def add_to_metrics(self, data: Dict[str, Any]) -> None: | ||
""" | ||
Add a dictionary of data into the main metrics dictionary. | ||
|
||
At the end, dumps the current state of the metrics to Redis. | ||
|
||
:param data: (Dict[str, Any]) Data to be added to the metrics dictionary via .update(). | ||
""" | ||
super().add_to_metrics(data) | ||
self.dump() | ||
|
||
def add_to_metrics_at_round(self, fl_round: int, data: Dict[str, Any]) -> None: | ||
""" | ||
Add a dictionary of data into the metrics dictionary for a specific FL round. | ||
|
||
At the end, dumps the current state of the metrics to Redis. | ||
|
||
:param fl_round: (int) the FL round these metrics are from. | ||
:param data: (Dict[str, Any]) Data to be added to the round's metrics dictionary via .update(). | ||
""" | ||
super().add_to_metrics_at_round(fl_round, data) | ||
self.dump() | ||
|
||
def dump(self) -> None: | ||
"""Dump the current metrics to Redis under the run_id name.""" | ||
encoded_metrics = json.dumps(self.metrics, cls=DateTimeEncoder) | ||
log(DEBUG, f"Dumping metrics to redis at key '{self.run_id}': {encoded_metrics}") | ||
self.redis_connection.set(self.run_id, encoded_metrics) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
import datetime | ||
import json | ||
from unittest.mock import Mock | ||
|
||
from fl4health.reporting.metrics import DateTimeEncoder | ||
from freezegun import freeze_time | ||
|
||
from florist.api.monitoring.metrics import RedisMetricsReporter | ||
|
||
|
||
@freeze_time("2012-12-11 10:09:08") | ||
def test_add_to_metrics() -> None: | ||
mock_redis_connection = Mock() | ||
test_run_id = "123" | ||
test_data = {"test": "data", "date": datetime.datetime.now()} | ||
|
||
redis_metric_reporter = RedisMetricsReporter(mock_redis_connection, test_run_id) | ||
redis_metric_reporter.add_to_metrics(test_data) | ||
|
||
mock_redis_connection.set.assert_called_once_with(test_run_id, json.dumps(test_data, cls=DateTimeEncoder)) | ||
|
||
|
||
@freeze_time("2012-12-11 10:09:08") | ||
def test_add_to_metrics_at_round() -> None: | ||
mock_redis_connection = Mock() | ||
test_run_id = "123" | ||
test_data = {"test": "data", "date": datetime.datetime.now()} | ||
test_round = 2 | ||
|
||
redis_metric_reporter = RedisMetricsReporter(mock_redis_connection, test_run_id) | ||
redis_metric_reporter.add_to_metrics_at_round(test_round, test_data) | ||
|
||
expected_data = { | ||
"rounds": { | ||
str(test_round): test_data, | ||
} | ||
} | ||
mock_redis_connection.set.assert_called_once_with(test_run_id, json.dumps(expected_data, cls=DateTimeEncoder)) | ||
|
||
|
||
@freeze_time("2012-12-11 10:09:08") | ||
def test_dump() -> None: | ||
mock_redis_connection = Mock() | ||
test_run_id = "123" | ||
test_data = {"test": "data", "date": datetime.datetime.now()} | ||
test_round = 2 | ||
|
||
redis_metric_reporter = RedisMetricsReporter(mock_redis_connection, test_run_id) | ||
redis_metric_reporter.add_to_metrics(test_data) | ||
redis_metric_reporter.add_to_metrics_at_round(test_round, test_data) | ||
redis_metric_reporter.dump() | ||
|
||
expected_data = { | ||
**test_data, | ||
"rounds": { | ||
str(test_round): test_data, | ||
}, | ||
} | ||
assert mock_redis_connection.set.call_args_list[2][0][0] == test_run_id | ||
assert mock_redis_connection.set.call_args_list[2][0][1] == json.dumps(expected_data, cls=DateTimeEncoder) |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.