Skip to content
This repository has been archived by the owner on Aug 15, 2019. It is now read-only.

added no_op_check flag to allow skipping op validation #160

Merged
merged 9 commits into from
Jul 9, 2018
Merged

Conversation

pyu10055
Copy link
Collaborator

@pyu10055 pyu10055 commented Jun 22, 2018

It is useful to allow skipping the op validation for converter, especially in node.js
env, where all op could be supported.
It is also useful for development, converting models with not implmented ops.


This change is Reviewable

@pyu10055 pyu10055 requested a review from caisq June 22, 2018 17:48
@caisq
Copy link
Collaborator

caisq commented Jun 23, 2018

Review status: 0 of 1 LGTMs obtained


python/tensorflowjs/converters/tf_saved_model_conversion.py, line 77 at r1 (raw file):

 no_op_check: Bool whether to perform the op check.

The negation her is confusing. Consider renaming the keyword argument to "skip_op_check".


python/tensorflowjs/converters/tf_saved_model_conversion_test.py, line 91 at r1 (raw file):

with graph.as_default():

Can you add a comment to mark which op is unsupported here?


python/tensorflowjs/converters/tf_saved_model_conversion_test.py, line 187 at r1 (raw file):

 # Load the saved weights as a JSON string.

nit: This is not weights per se, strictly speaking. It's the manifest of weights.


Comments from Reviewable

@caisq
Copy link
Collaborator

caisq commented Jun 23, 2018

Review status: 0 of 1 LGTMs obtained


python/tensorflowjs/converters/converter.py, line 187 at r1 (raw file):

--no_op_check

How about calling it --skip_op_check. Negation increases cognitive load.


Comments from Reviewable

@pyu10055
Copy link
Collaborator Author

Review status: 0 of 1 LGTMs obtained


python/tensorflowjs/converters/converter.py, line 187 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…
--no_op_check

How about calling it --skip_op_check. Negation increases cognitive load.

Done.


python/tensorflowjs/converters/tf_saved_model_conversion.py, line 77 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…
 no_op_check: Bool whether to perform the op check.

The negation her is confusing. Consider renaming the keyword argument to "skip_op_check".

Done.


python/tensorflowjs/converters/tf_saved_model_conversion_test.py, line 91 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…
with graph.as_default():

Can you add a comment to mark which op is unsupported here?

Done.


python/tensorflowjs/converters/tf_saved_model_conversion_test.py, line 187 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…
 # Load the saved weights as a JSON string.

nit: This is not weights per se, strictly speaking. It's the manifest of weights.

Done.


Comments from Reviewable

@caisq
Copy link
Collaborator

caisq commented Jun 25, 2018

:lgtm_strong:


Review status: :shipit: complete! 1 of 1 LGTMs obtained


python/tensorflowjs/converters/tf_saved_model_conversion.py, line 182 at r2 (raw file):

perform

s/perform/skip/


Comments from Reviewable

@pyu10055 pyu10055 merged commit fde7bd1 into master Jul 9, 2018
@pyu10055 pyu10055 deleted the no_op_check branch July 9, 2018 19:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants