[NNVM][FRONTEND] Tensorflow frontend support - #1188
Conversation
|
@tqchen , @Huyuwei and @FrozenGene pls review. |
|
With respect to nnvm base it's all the same except tutorials/nnvm/ location changed now. |
| # Tensor flow doesn't have seperate list for params extraction. | ||
| # Operator name 'Const' is treated as a parameter to build NNVM params dict. | ||
| if node.op == "Placeholder": | ||
| # Assuming only one input graph with type 'Placeholder' |
There was a problem hiding this comment.
why only a Placeholder is supported?
There was a problem hiding this comment.
Unlike other frameworks Tensorflow doesn't list graph inputs.
Hence I assumed first 'Placeholder' or 'Const' node as input node for the graph.
|
|
||
| self._output_shapes[node.name] = \ | ||
| [tensor_util.TensorShapeProtoToList(shape) \ | ||
| for shape in self._parse_attr(node.attr)['_output_shapes']] |
There was a problem hiding this comment.
Does the code require a specific GraphDef version?
When I try your feature, a problem troubles me.
""
for shape in self._parse_attr(node.attr)['_output_shapes']]
KeyError: '_output_shapes'
""
There was a problem hiding this comment.
Yes, you need to freeze the tensorflow graph with add_shapes=True like below.
output_graph_def = tf.graph_util.convert_variables_to_constants(
sess,
sess.graph.as_graph_def(add_shapes=True),
['InceptionV3/Predictions/Reshape_1'],
)
It was discussed earlier @ dmlc/nnvm#472
There was a problem hiding this comment.
thanks! "add_shapes=True" can really avoid the error which happened on Placeholder, but the same error happens th on Const even if the fix is added.
There was a problem hiding this comment.
_output_shapes should exist for all nodes with above option.
Can you share the dump of the Const node ? Or Attribute dump of Const node ?
There was a problem hiding this comment.
"convert_variables_to_constants" method does not seem to add "output_shape" field
There was a problem hiding this comment.
@srkreddy1238 I want to cry! Looking forward to your updates
There was a problem hiding this comment.
I will try to share a patch for multi input later today :)
Can you share the protobuf file for reference ?
There was a problem hiding this comment.
@colin1988
I just checked and multi input work fine. No need of any change.
Ref. added a test case for the same.
There was a problem hiding this comment.
@srkreddy1238 thanks very much. I will try it right now.
There was a problem hiding this comment.
@srkreddy1238 I found the method to fix it. More precisely, I found an error in my code.
609 output_graph_def = tf.graph_util.convert_variables_to_constants(
610 sess, sess.graph.as_graph_def(add_shapes=True), ["dnn/title_scoring/final_score/Sigmoid"])
611 tf.reset_default_graph()
612 graph = tf.import_graph_def(output_graph_def, return_elements=["dnn/title_scoring/final_score/Sigmoid"], name='')
613 output_graph_def = tf.graph_util.convert_variables_to_constants(
614 sess, tf.get_default_graph().as_graph_def(add_shapes=True), ["dnn/title_scoring/final_score/Sigmoid"])
615 tf.train.write_graph(output_graph_def, 'export_model', "asq_title_score.pb.txt", as_text=True)
Line 611 is essential !!
|
@Huyuwei Can you help with a final review on this ? |
|
I tried from_tensorflow.py with MobileNet_v1_1.0_224 from https://github.com/tensorflow/models/blob/master/research/slim/nets/mobilenet_v1.md. ('x', (299, 299, 3)) Any idea? Thanks |
|
Regenerate the tensor flow graph with add_shapes=True. This is an assumption to have _output_shapes for all nodes. |
|
Do you have a python example to convert checkpoint to frozen model with shapes=True? Thanks, |
|
Ref tool for conversion: https://github.com/srkreddy1238/dmlc_data/blob/master/work/tf-to-nnvm.py I checked Mobilenet and below operations missing in frontend now. I am working on mapping these operator to existing nnvm top. Will share soon. |
|
I am able to convert mobilenet model with your python script. Waiting for your PR to support additional operators of mobilenetv1. Thanks, |
|
await approval from @Huyuwei |
|
Sorry for late response due to travelling. Will do a final review Tuesday. |
| from tensorflow.python.framework import tensor_util | ||
|
|
||
|
|
||
| repo_base = 'https://github.com/srkreddy1238/dmlc_data/raw/master/models/tensorflow/InceptionV1/' |
There was a problem hiding this comment.
dmlc/web-data#74
PR for support files @ web-data.
I will update the above paths after this merge.
There was a problem hiding this comment.
data paths changed to dmlc/web-data.
|
|
||
| ###################################################################### | ||
| # Download processed tensorflow model | ||
| # --------------------------------------------- |
There was a problem hiding this comment.
rst standard: make sure the line is the same as the title
| return graph_def | ||
|
|
||
| class NodeLookup(object): | ||
| """Converts integer node ID's to human readable labels.""" |
There was a problem hiding this comment.
This class is not very helpful in understanding in tutorials. Consider put it under nnvm.testiung
| self.node_lookup = self.load(label_lookup_path, uid_lookup_path) | ||
|
|
||
| def load(self, label_lookup_path, uid_lookup_path): | ||
| """Loads a human readable English name for each softmax node. |
There was a problem hiding this comment.
use numpydoc convention http://docs.tvm.ai/contribute/document.html#document-python
|
I could get through FusedBatchNorm, Relu6 and Shape operators, but depthwise convolution don't have a direct support now. |
f361f7f to
d42ae4f
Compare
|
If there is no depthwise conv support, how from_mxnet.py can support mobilenetv1 without any issue? Thanks, |
|
@kaishijeng |
|
tensorflow use # Gather to implement # embedding_lookup. Is there corresponding op in tvm/nnvm? I got the error belowTraceback (most recent call last): |
|
@kaishijeng Can you help to validate the result first? |
|
Gather is not supported in tensorflow now. |
|
Siva
See below which has mobilenet with tvm/nnvm performance data
https://github.com/merrymercy/tvm-mali
Thanks,
…On Tue, Jun 5, 2018 at 6:22 AM, Siva ***@***.***> wrote:
@kaishijeng <https://github.com/kaishijeng>
You said mxnet ??
I see some depthwise support in keras not in mxnet ?
Am I missing something here ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1188 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AMGg3tB7a9cqI8s-kErG9RQTNes5umbnks5t5oYPgaJpZM4UR8cu>
.
|
|
@srkreddy1238 Tensorflow supported gather early， probably starting from 0.10 |
|
How do I pull this patch srkreddy1238/tvm@c5e852e in my code? FC |
| test_forward_reshape() | ||
| test_forward_squeeze() | ||
| test_forward_concat_v2() | ||
| test_forward_multi_input() |
There was a problem hiding this comment.
Can you also test 2 or 3 end-to-end networks like Inception, ResNet?
There was a problem hiding this comment.
@Huyuwei
Added Inception V1 & V3 end-to-end test cases.
I will add mobilenet as a separate PR soon (once the dependencies are merged).
Later I will check Resnet too.
|
I mean our tensorflow frontend now doesn't support Gather now. I see "take" operator is added to tvm already and nnvm operator for the same is pending. |
|
@kaishijeng |
|
Apply patch and got the following error: Traceback (most recent call last): Stack trace returned 10 entries: |
|
@kaishijeng Mobilenet input node name is "input", hence shape_dict = {'input': x.shape} Ref. Below patch with changes to the utility for mobilenet. You just need to change tf model, image paths. |
| params: dict of str to ndarray | ||
| The parameter dictionary | ||
| """ | ||
| arg_list = graph.symbol.list_input_names() |
|
Thanks to all the code reviewers and contributors for help improving this PR, this is now merged! |
|
Thanks for reviewing to improve the code quality. |
|
What happens to below to support mobilenet? Thanks, |
|
@kaishijeng |
|
Which PR should I pull to add FusedBatchNorm operator? Thanks |
|
for FusedBatchNorm only the below line should work. srkreddy1238@c5e852e#diff-3845514369cc735d96dd63a274a21471R484 |
|
Is there a good reason why you don't create a PR to support mobilenet?
Thanks,
…On Thu, Jun 14, 2018 at 11:58 PM, Siva ***@***.***> wrote:
for only FusedBatchNorm only the below line should work.
***@***.***#diff-3845514369cc735d96dd63a274a21471R484
<srkreddy1238@c5e852e#diff-3845514369cc735d96dd63a274a21471R484>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1188 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AMGg3tLKeTQwdJLqnR27s_IRjij9fEGsks5t81sIgaJpZM4UR8cu>
.
|
|
:) no specific reason. I understand you wanted mobilenet on latest code. I will raise today for the frontend. **You may need to take both patches to your local code for mobilenet. |
|
@kaishijeng |
|
Mobilenet works properly with 1232PR. Thanks, |
|
@kaishijeng in fact, I supported ssd-mobilenet tensorflow model before tvm support it. My way is transform tensorflow’s ssd-mobilenet to CoreML, then use NNVM/TVM support it. And I don’t need any special ops to support ssd-mobilenet. |
|
@kaishijeng |
|
@srkreddy1238 if everything goes well, I think you will not use new ssd ops in tvm, which is for MXNet I think. |
|
Can you provide an instruction of your way to (via CoreML) enablessd-mobilenet? My platform is RK3399 which is arm-based linux? Thanks, |
|
@kaishijeng see: https://github.com/tf-coreml/tf-coreml/tree/master/examples Maybe you will implement some ops in from_coreml.py in nnvm. I previous implemented some ops haven’t contributed back to nnvm. |
|
@FrozenGene Are you able to share any of your code getting the CoreML converted Model to run on TVM? |
|
@madhavajay You could start from here: https://github.com/tf-coreml/tf-coreml/blob/master/examples/ssd_example.ipynb Then you could implement related ops to support this model. In my memory, there are not many ops to implement. Currently, my CoreML frontend has many different places compared than official TVM. (For example, my TVM support 4-D padding of Conv2D, but official TVM doesn't, we want to PR it soon). So I could not figure out what modification for SSD-Mobilenet model. We will also have plan PR our CoreML frontend, but doesn't have one detail time. |
7 participants
Tested for Inception V1 and V3