-
Notifications
You must be signed in to change notification settings - Fork 219
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
Convert protobuf model to native format #550
Convert protobuf model to native format #550
Conversation
…(to be refactored)
We want to introduce an additional piece of functionality in two forms:
We start with
With all said above, at this point, we can not guarantee |
As a solution to the abovementioned problems I propose the following approach:
These signatures allow us to mitigate issues caused by the diversity of model definition forms in our TuskRunners and yet deliver the expected functionality. At the same time, the data problem mentioned under point 5 is ignored, as we know it is already solved prior to executing Work left in this PR:
|
openfl/interface/model.py
Outdated
@option('-i', '--input', 'model_protobuf_path', required=True, | ||
help='The model protobuf to convert', | ||
type=ClickPath(exists=True)) | ||
@option('-o', '--output', 'output_filepath', required=False, |
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.
We could require this option as well for users to make sure they provide the path for the saved model. In case of someone forgot to specify it, it would be easier to notify them about the wrong command call and ask for the path. Otherwise, they have to search the save path in the console 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 understand the intention, but I would argue that we should make arguments required
only if they are literally required.
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.
output_filepath
is required by the _save
function for calling TaskRunner.save_native
.
For example, torch.save
and tf.keras.Model.save_weights
do require a file argument.
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.
Yes, but those functions are python function calls, in our case we have a CLI command which is called from a specific place, namely, an experiment workspace.
Honestly, I am not against making this argument required. @mansishr, what do you think?
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 believe that here is used good approach with default value. It is checked that file already exists and is asked user for the confirmation to rewrite. At the end, it is written to the output in witch path the model is saved.
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 agree, specifying a default path helps for our CLI command.
Signed-off-by: igor-davidyuk <igor.davidyuk@intel.com>
Signed-off-by: igor-davidyuk <igor.davidyuk@intel.com>
Co-authored-by: Ilya Trushkin <ilya.trushkin@intel.com>
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 to me
* Initial implementation of CLI command to save model in native format (to be refactored) * method functioning * updated function and docs * Add check for overwriting output file * add unit and integrational tests * fix test * signed-off commit Signed-off-by: igor-davidyuk <igor.davidyuk@intel.com> * support calling python command outside workspace Signed-off-by: igor-davidyuk <igor.davidyuk@intel.com> * Illiyas typo fix Co-authored-by: Ilya Trushkin <ilya.trushkin@intel.com> * Update tests/openfl/interface/test_model_api.py * Update tests/openfl/interface/test_model_api.py Signed-off-by: igor-davidyuk <igor.davidyuk@intel.com> Co-authored-by: igor-davidyuk <igor.davidyuk@intel.com> Co-authored-by: Ilya Trushkin <ilya.trushkin@intel.com> Signed-off-by: Aleksandr Mokrov <aleksandr.mokrov@intel.com>
This PR:
fx model save
, that takes as input the.pbuf
model file produced by a federated experiment and converts it to the native PyTorch / TensorFlow model representation for future use.This PR is WIP. The core functionality to be refactored by @igor-davidyuk into theutilities
folder, so this can also be used via API.closes #552