Skip to content

Conversation

@pvaneck
Copy link
Contributor

@pvaneck pvaneck commented Feb 19, 2020

Previously, only the tfjs_layers_model to tfjs_layers_model conversion pair worked with the weight_shard_size_bytes flag. This PR allows the flag to be used for any conversion where the output_format is tfjs_layers_model or tfjs_graph_model.


This change is Reviewable

Copy link
Collaborator

@pyu10055 pyu10055 left a 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 great contribution. The PR looks great, one high level comment, we have the wizard.ts file https://github.com/tensorflow/tfjs/blob/master/tfjs-converter/python/tensorflowjs/converters/wizard.py
can you add support for the shard size param too? thanks

Reviewed 1 of 6 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)

@pvaneck
Copy link
Contributor Author

pvaneck commented Feb 26, 2020

@pyu10055 Thanks for the review. I added support for the shard size param in the wizard.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained (waiting on @pvaneck)


tfjs-converter/python/tensorflowjs/converters/wizard.py, line 486 at r2 (raw file):

          'when': lambda answers: (value_in_list(answers, common.OUTPUT_FORMAT,
                                                 (common.TFJS_LAYERS_MODEL,
                                                  common.TFJS_GRAPH_MODEL)) or

should this be 'and' instead of 'or'?

Copy link
Contributor Author

@pvaneck pvaneck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)


tfjs-converter/python/tensorflowjs/converters/wizard.py, line 486 at r2 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

should this be 'and' instead of 'or'?

Ideally, we would only need the first condition before the or, however, if the input format is TF_SAVED_MODEL or TF_HUB_MODEL, the implication is that the model will be converted to TFJS_GRAPH_MODEL. Since this is implied, the user isn't asked in the wizard to select an output format, so the OUTPUT_FORMAT key is never set in answers. Thus, the first condition where we just check OUTPUT_FORMAT would fail.

So, that second or condition is just another way of checking if the OUTPUT_FORMAT is TFJS_GRAPH_MODEL based on the input format.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @pvaneck)


tfjs-converter/python/tensorflowjs/converters/wizard.py, line 486 at r2 (raw file):

Previously, pvaneck (Paul Van Eck) wrote…

Ideally, we would only need the first condition before the or, however, if the input format is TF_SAVED_MODEL or TF_HUB_MODEL, the implication is that the model will be converted to TFJS_GRAPH_MODEL. Since this is implied, the user isn't asked in the wizard to select an output format, so the OUTPUT_FORMAT key is never set in answers. Thus, the first condition where we just check OUTPUT_FORMAT would fail.

So, that second or condition is just another way of checking if the OUTPUT_FORMAT is TFJS_GRAPH_MODEL based on the input format.

make sense, thank you.

@pyu10055 pyu10055 merged commit af46b87 into tensorflow:master Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants