-
Notifications
You must be signed in to change notification settings - Fork 36
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
Refactor/SK-1144 | Simplify on_train and on_validate #731
Conversation
…ed abstract methods
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.
Good improvements overall, nice work! I have some input (nothing major), check out the comments!
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 think we could include a callback function for training in this README example. It doesn't need to do anything more than return the input model. The end goal is to make users understand how they can develop clients that help with their use case.
receiver_name=receiver_name, | ||
receiver_role=receiver_role, | ||
receiver_name=request.sender.name, | ||
receiver_role=request.sender.role, |
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.
Not sure about these changes... Don't know if a TaskRequest as input is the way to go. It creates a dependency that did not exist previously... If you think it's cleaner, keep it.
Key changes
grpc_handler.py
send_model_update()
client_api.py
_subscribers
withtrain_callback
andvalidate_callback
train_callback
needs to take in_model as parameter and return out_model and metadata dictvalidate_callback
needs to take in_model as parameter and return metrics dict_task_stream_callback
now invokesupdate_local_model
orvalidate_global_model
depending on task request typeupdate_local_model
andvalidate_global_model
contain boiler plate code that was previously inside theon_train
andon_validate
callbacks (getting/sending model from/to combiner, generating uuid, assembling metadata, sending model update/validation)train
andvalidate
run
method creates a child thread for heart beats and listens to task stream in main threadclient_v2.py
README.rst