Fix bug in converter to load saved model #1981
Conversation
|
@dsmilkov Per offline discussion, I'll await your new unit test(s). |
|
@caisq PTAL. Added a unit test that emulates the Saved Model by adding a hash table op that is not used by the inference signature. Verified that this test fails at master. Simply creating a |
dsmilkov
left a comment
dsmilkov
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq and @pyu10055)
tfjs-converter/python/run-python-tests.sh, line 27 at r2 (raw file):
pip install virtualenv TMP_VENV_DIR="$(mktemp -d --suffix=_venv)"
mktemp on Mac doesn't have a suffix param, and since this is not relevant, I removed it.
caisq
left a comment
caisq
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq, @dsmilkov, and @pyu10055)
tfjs-converter/python/run-python-tests.sh, line 27 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
mktemp on Mac doesn't have a suffix param, and since this is not relevant, I removed it.
It's nice to have a suffix, because it makes it easy to manually identify / clean up the directory in case the test fails midway. Can you do this: mktemp /tmp/banana.XXXXXXXXXXXXXXXXXXXXXXX.venv I think that works on both Linux and Mac.
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py, line 280 at r2 (raw file):
input_graph_def = saved_model_utils.get_meta_graph_def( saved_model_dir, ','.join(saved_model_tags)).graph_def
Is it possible to move these lines down to immediately before where input_graph_def is used, for logical clarity?
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py, line 282 at r2 (raw file):
_ =
Might be a naive question: since you don't use the return value, why do you have to get it? Does pylint complain?
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2_test.py, line 234 at r2 (raw file):
os.path.join(self._tmp_dir, SAVED_MODEL_DIR),
nit: Since the two arguments are exactly the same, there is no need to compute them twice. Just compute them once.
BTW, not directly related to this PR: it's a little weird that the input and output directories are the same. It's possible that in the future this may cause some clashes. It's only fortuitous that this works without error right now.
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2_test.py, line 238 at r2 (raw file):
weights
Call this expected_weights_manifest to be clear
dsmilkov
left a comment
dsmilkov
left a comment
There was a problem hiding this comment.
PTAL.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq and @pyu10055)
tfjs-converter/python/run-python-tests.sh, line 27 at r2 (raw file):
Previously, caisq (Shanqing Cai) wrote…
It's nice to have a suffix, because it makes it easy to manually identify / clean up the directory in case the test fails midway. Can you do this:
mktemp /tmp/banana.XXXXXXXXXXXXXXXXXXXXXXX.venvI think that works on both Linux and Mac.
Done.
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py, line 280 at r2 (raw file):
Previously, caisq (Shanqing Cai) wrote…
input_graph_def = saved_model_utils.get_meta_graph_def( saved_model_dir, ','.join(saved_model_tags)).graph_defIs it possible to move these lines down to immediately before where
input_graph_defis used, for logical clarity?
Done.
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py, line 282 at r2 (raw file):
Previously, caisq (Shanqing Cai) wrote…
_ =Might be a naive question: since you don't use the return value, why do you have to get it? Does pylint complain?
Done. (I took the line from the freeze_graph util)
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2_test.py, line 234 at r2 (raw file):
Previously, caisq (Shanqing Cai) wrote…
os.path.join(self._tmp_dir, SAVED_MODEL_DIR),nit: Since the two arguments are exactly the same, there is no need to compute them twice. Just compute them once.
BTW, not directly related to this PR: it's a little weird that the input and output directories are the same. It's possible that in the future this may cause some clashes. It's only fortuitous that this works without error right now.
Done. I took this code from a previous unit test. Changed both unit tests to use the subdirectory "js" to store the output
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2_test.py, line 238 at r2 (raw file):
Previously, caisq (Shanqing Cai) wrote…
weightsCall this
expected_weights_manifestto be clear
Done.
caisq
left a comment
caisq
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @caisq and @pyu10055)
BUG
Fix a bug that prohibits the conversion of a Saved Model produced by AutoML object detection.
The error was produced by an internal
convert_variables_to_constants() method:ValueError: Cannot find the variable that is an input to the ReadVariableOp.Comparing our code to the
freeze_graphutil showed that we were not loading the graph properly into a Session object, before callingconvert_variables_to_constants().The unit test emulates the Saved Model by adding a hash table op that is not used by the inference signature. Verified that this test fails at master.
Cloud will make an integration test internally that asserts that this conversion continuous to work.
This change is