add signature def to model.json#2326
Conversation
dsmilkov
left a comment
There was a problem hiding this comment.
Nice!
Reviewed 5 of 5 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq, @dsmilkov, and @pyu10055)
tfjs-converter/python/tensorflowjs/converters/common.py, line 37 at r1 (raw file):
CONVERTED_BY_KEY = 'convertedBy' SIGNATURE = 'signature'
how about calling it SIGNATURE_KEY to signal that this is the key in a dictionary (like the keys above)
tfjs-converter/python/tensorflowjs/converters/converter_test.py, line 306 at r1 (raw file):
self.assertTrue(model_json['modelTopology']) self.assertIsNot(model_json['modelTopology']['versions'], None) self.assertIsNot(model_json['signature'], None)
can you assert a bit more about the contents of the signature? Otherwise if I change the implementation to store the wrong result, no unit tests will catch that
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py, line 417 at r1 (raw file):
output_dir, common.ARTIFACT_MODEL_JSON_FILE_NAME) saved_model_tags = saved_model_tags.split(',')
Is this meant to fix a bug? Can you add a unit test for it that fails without this fix?
pyu10055
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq and @dsmilkov)
tfjs-converter/python/tensorflowjs/converters/converter_test.py, line 306 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
can you assert a bit more about the contents of the signature? Otherwise if I change the implementation to store the wrong result, no unit tests will catch that
added check for inputs and outputs field for the signature.
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py, line 417 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
Is this meant to fix a bug? Can you add a unit test for it that fails without this fix?
we want to split on comma only not ', '. The fix seems to be reasonable. There are no easy way to add extra tags for saved model.
|
Thanks! Didn’t see any changes. push to origin? |
pyu10055
left a comment
There was a problem hiding this comment.
sorry, just pushed.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq and @dsmilkov)
dsmilkov
left a comment
There was a problem hiding this comment.
Sorry for the delay. I've been travelling today
Reviewed 5 of 5 files at r2.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @caisq)
This adds selected signature def structure into the model.json
It can be used by the model executor to find out the input list order or show user the signature to be called. The JS change will come in a follow up PR.
fixes #1733
ref #1603
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is