-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Adding support for Control flow v2 in converter and executor #3346
Conversation
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.
Thank you for the amazing work. I left some nit suggestions, overall LGTM!
Reviewed 15 of 22 files at r1.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128 and @pyu10055)
.vscode/settings.json, line 144 at r1 (raw file):
"__refstring": "cpp" }, "editor.formatOnSave": true
Do we need this? Different person may have different preference. I think they can set this in their preference settings.
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py, line 203 at r1 (raw file):
Args: nodes: list of tf.NodeDef TensorFlow NodeDef proto object.
Also add Returns. Something like: A list of node. Node consists of two fields, name and data.
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2_test.py, line 685 at r1 (raw file):
tf_saved_model_conversion_v2.convert_tf_saved_model( os.path.join(self._tmp_dir, SAVED_MODEL_DIR),
This can be just tfjs_path, right?
tfjs-converter/src/operations/executors/control_executor.ts, line 44 at r1 (raw file):
if (condValue[0] === 1) { return context.functionMap[thenFunc].executeFunctionAsync(args); } else {
Should it be else if (condValue[0] === 0) {}, for anything else, it should throw an error.
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.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128 and @pyu10055)
.vscode/settings.json, line 144 at r1 (raw file):
Previously, lina128 (Na Li) wrote…
Do we need this? Different person may have different preference. I think they can set this in their preference settings.
sounds good, let me revert this.
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2_test.py, line 685 at r1 (raw file):
Previously, lina128 (Na Li) wrote…
This can be just tfjs_path, right?
this is where the test saved model is created at, is that what you asking about?
tfjs-converter/src/operations/executors/control_executor.ts, line 44 at r1 (raw file):
Previously, lina128 (Na Li) wrote…
Should it be else if (condValue[0] === 0) {}, for anything else, it should throw an error.
actually by definition, the cond value should be a boolean type, we are representing bool as int32. TF also support string tensor, if the string is empty it will be false.
maybe written it as
if (condValue[0]) {} else {}
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.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128 and @pyu10055)
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2_test.py, line 685 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
this is where the test saved model is created at, is that what you asking about?
It's just my observation that this path, and the next path, and the tfjs_path is all the same in this file. If this is intended, can just use one variable for all the places.
tfjs-converter/src/operations/executors/control_executor.ts, line 44 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
actually by definition, the cond value should be a boolean type, we are representing bool as int32. TF also support string tensor, if the string is empty it will be false.
maybe written it as
if (condValue[0]) {} else {}
Thank you for the explanation Ping. If condValue[0] is always a boolean type, then this LGTM.
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.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128)
.vscode/settings.json, line 144 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
sounds good, let me revert this.
Done.
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2.py, line 203 at r1 (raw file):
Previously, lina128 (Na Li) wrote…
Also add Returns. Something like: A list of node. Node consists of two fields, name and data.
Done.
tfjs-converter/python/tensorflowjs/converters/tf_saved_model_conversion_v2_test.py, line 685 at r1 (raw file):
Previously, lina128 (Na Li) wrote…
It's just my observation that this path, and the next path, and the tfjs_path is all the same in this file. If this is intended, can just use one variable for all the places.
Done.
tfjs-converter/src/operations/executors/control_executor.ts, line 44 at r1 (raw file):
Previously, lina128 (Na Li) wrote…
Thank you for the explanation Ping. If condValue[0] is always a boolean type, then this LGTM.
basically TF does not through exception if the value is not boolean, it will try to convert it to boolean.
Based on preliminary benchmark, control flow v2 greatly improves the inference performance on branching/loop logic:
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is