-
Notifications
You must be signed in to change notification settings - Fork 34
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
Make choice of aggregator dynamic at session level and additional aggregators (FedOpt) #498
Conversation
…plot validation results
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.
looks good! Some comments though
command: /venv/bin/python entrypoint validate $ENTRYPOINT_OPTS | ||
helper: pytorchhelper |
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.
what is 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.
Ah, it sneakt in. I was thinking about a new way to specify the helper. Will remove for now.
examples/notebooks/API_Example.ipynb
Outdated
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 would add this to gitignore after it has been commited
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.
Done. I also moved it into the mnist-pytorch example
@@ -134,7 +134,7 @@ def __init__(self, config): | |||
self.server = Server(self, self.modelservice, grpc_config) | |||
|
|||
# Set up round controller | |||
self.control = RoundController(config['aggregator'], self.repository, self, self.modelservice) | |||
self.control = RoundController(self.repository, self, self.modelservice) |
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.
to avoid confusion with controller I think it should be called round_control
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.
Agree, it is confusing. I changed it the RoundHandler (which is a more appropriate name)
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.
Some minor docs string ref errors
@@ -20,14 +20,14 @@ class Aggregator(AggregatorBase): | |||
:type server: class: `fedn.network.combiner.Combiner` | |||
:param modelservice: A handle to the model service :class: `fedn.network.combiner.modelservice.ModelService` | |||
:type modelservice: class: `fedn.network.combiner.modelservice.ModelService` | |||
:param control: A handle to the :class: `fedn.network.combiner.round.RoundController` | |||
:type control: class: `fedn.network.combiner.round.RoundController` | |||
:param control: A handle to the :class: `fedn.network.combiner.round.RoundHandler` |
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.
roundhelper
@@ -14,15 +14,15 @@ class Aggregator(AggregatorBase): | |||
:type server: class: `fedn.network.combiner.Combiner` | |||
:param modelservice: A handle to the model service :class: `fedn.network.combiner.modelservice.ModelService` | |||
:type modelservice: class: `fedn.network.combiner.modelservice.ModelService` | |||
:param control: A handle to the :class: `fedn.network.combiner.round.RoundController` | |||
:type control: class: `fedn.network.combiner.round.RoundController` | |||
:param control: A handle to the :class: `fedn.network.combiner.round.RoundHandler` |
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.
roundhelper
@@ -141,8 +141,8 @@ def get_aggregator(aggregator_module_name, storage, server, modelservice, contro | |||
:type server: class: `fedn.network.combiner.Combiner` | |||
:param modelservice: A handle to the model service :class: `fedn.network.combiner.modelservice.ModelService` | |||
:type modelservice: class: `fedn.network.combiner.modelservice.ModelService` | |||
:param control: A handle to the :class: `fedn.network.combiner.round.RoundController` | |||
:type control: class: `fedn.network.combiner.round.RoundController` | |||
:param control: A handle to the :class: `fedn.network.combiner.round.RoundHandler` |
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.
roundhelper
@@ -19,18 +19,18 @@ class AggregatorBase(ABC): | |||
:type server: class: `fedn.network.combiner.Combiner` | |||
:param modelservice: A handle to the model service :class: `fedn.network.combiner.modelservice.ModelService` | |||
:type modelservice: class: `fedn.network.combiner.modelservice.ModelService` | |||
:param control: A handle to the :class: `fedn.network.combiner.round.RoundController` | |||
:type control: class: `fedn.network.combiner.round.RoundController` | |||
:param control: A handle to the :class: `fedn.network.combiner.round.RoundHandler` |
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.
roundhelper
Description
This pull request implements the foundation for more flexible experimentation with different optimization algorithms by making the aggregator used dynamically configurable on session level. Also introduces a scaffold for aggregators from the FedOpt family with an implementation of FedAdam.
New features:
Major changes: