TensorFlow Split and LRN operator support for Alexnet Model #1572

Dayananda-V · 2018-08-08T15:58:27Z

Tensorflow input parse logic updated
LRN bugfix
Split operator added
Input shapes parse logic updated
To support alexnet on tensorflow(https://www.cs.toronto.edu/~guerzhoy/tf_alexnet/)

Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from others in the community.

1. Tensorflow input parse logic updated 2. LRN bugfix 3. Split operator added 4. Input shapes parse logic updated To support alexnet on tensorflow(https://www.cs.toronto.edu/~guerzhoy/tf_alexnet/)

1. Compilation issue fix 2. '_input_shapes' handle fix across operator frontend

1. Pylint issue fix

1. Comment refactored

Dayananda-V · 2018-08-09T10:24:44Z

This PR adds Split, LRN operator and shape parse logic,
@srkreddy1238 @yzhliu Could you please review?

srkreddy1238 · 2018-08-09T11:19:09Z

nnvm/python/nnvm/frontend/tensorflow.py

-        attr_new['alpha'] = attr.get('alpha', 1) * size
-        attr_new['beta'] = attr.get('beta', 0.5)
-        return AttrCvt(op_name='lrn')(new_inputs, attr_new)
+        depth_radius = attr.get('depth_radius', 2)


LRN is already implemented earlier. Any reason for this modify ?

Previously implemented LRN was wrong as per TF formula and input tensor is never pass to operator, this leak observe after run the AlexNet model

I think that was a bug passing empty inputs new_inputs = []
Removing new_inputs and using inputs instead should work for alexnet as well.
Keep the remaining implementation default value handling.

@siju-samuel can you confirm the depth_radius default value as 5 which results size=11 ? or is it a typo !!

depth_radius = attr.get('depth_radius', 5) size = (depth_radius * 2) + 1

Test case was passed as the input was taken from params instead.
#1546 should fix this issue by replacing all const inputs to Placeholders.

@srkreddy1238 as per tensorflow, the default value of depth_radius is 5. So the size will become 11.
https://www.tensorflow.org/api_docs/python/tf/nn/local_response_normalization

srkreddy1238 · 2018-08-09T11:19:51Z

nnvm/python/nnvm/frontend/tensorflow.py

            if attr['data_format'] == 'NHWC':
-                in_h = input_shapes[0][1]
-                in_w = input_shapes[0][2]
+                in_h = input_shapes[1]


input_shapes -> input_shape

srkreddy1238 · 2018-08-09T11:21:51Z

nnvm/python/nnvm/frontend/tensorflow.py


        # Extract kernel shape from params
-        conv_param_weights = params[inputs[1].list_output_names()[0]]
+        if inputs[1] in attr['_input_shapes']:


Can you share details of use case where convolution weights not in params ?

This case is observe while running AlexNet model.

Is weights output of another node in runtime?

Can u share serialised tensor flow graph?

fine tuned Alexnet can be found here for the above reference

srkreddy1238 · 2018-08-09T11:23:47Z

nnvm/python/nnvm/frontend/tensorflow.py

+        pop_node = inputs.pop(0)
+        axis = params[pop_node.list_output_names()[0]].asnumpy()[0]
+        return AttrCvt(op_name="split",
+                       ignores=['num_split'],


Suggest to use transforms instead of ignores & extras.

Have you reached consensus the changes here is good?

srkreddy1238 · 2018-08-09T11:25:45Z

nnvm/python/nnvm/frontend/tensorflow.py

+                            input_sym = self._nodes[node_input_key].__getitem__(slot_num)
+                        except NNVMError:
+                            # TODO: Fancy node name invalid slot neglect
+                            input_sym = self._nodes[node_input_key].__getitem__(0)


Can you share details on this fall back?
I see slot_num is initialized to 0 above already.

Some of Layer implementation in frontend(LSTM, Ptb) this assign fancy node name (eg. Model/RNN/cell_0/RnnCell:6 ), in this case 6 is slot updated as per our logic and only one node will be present, in this fancy naming convention we need to parse the default(0) node.

srkreddy1238 · 2018-08-09T11:29:19Z

nnvm/python/nnvm/frontend/tensorflow.py

+                        input_shapes[input_sym] = self._output_shapes[
+                            node_input_key].__getitem__(slot_num)
+                        attr['_input_shapes'] = input_shapes
+                    except KeyError:


This except is for the same statement below where the self._nodes doesn't have key node_input_key
input_sym = self._nodes[node_input_key].__getitem__(slot_num)

Suggest to make one try block with multiple except instead of nested.

as address to review comment, we need nested try block to handle fancy node name convention logic.

It's the same statement which cause 2 exceptions in two different cases, one try on input_sym = self._nodes[node_input_key].__getitem__(slot_num) with two except should work. Pls check.

First try block we are trying to access the slot base input and after caught NNVMError we are trying to access default(0) location input, in this case if we caught error should be thrown to user.

srkreddy1238 · 2018-08-09T11:30:42Z

nnvm/tests/python/frontend/tensorflow/test_forward.py

+        tf_output = run_tf_graph(sess, [np_data], ['in_data:0'], 'split:0')
+        tvm_output = run_tvm_graph(final_graph_def, [np_data], ['in_data'],
+                                   tf_output.shape, dtype)
+        np.testing.assert_allclose(tf_output, tvm_output, atol=1e-5, rtol=1e-5)


Split results in multiple outputs, hence suggest to enhance run methods to return multiple outputs and compare the same.

Split returns the split result with list so comparing with List content result.

run_tf_graph is not designed to result a list. Also we pass 'split:0' to return only first result.
run_tvm_graph can return list if output_shape, output_dtypes are lists.

Please check.

Yeah missed scenario, will work on it.

srkreddy1238 · 2018-08-09T11:36:20Z

nnvm/tests/python/frontend/tensorflow/test_forward.py

+def test_forward_split():
+    '''test split operator'''
+    _test_split((2, 3), 2, axis=0)
+    _test_split((6, 3), 3, axis=0)


Pls add a case for -ve axis

Split axis should be range (0, rank(value)). so negative axis is not added

srkreddy1238 · 2018-08-09T11:40:45Z

nnvm/python/nnvm/frontend/tensorflow.py


+def _split():
+    def _impl(inputs, attr, params):
+        pop_node = inputs.pop(0)


Ref. Split
tensorflow has two variations for split as num_split and size_split.
Please handle if possible or raise exception accordingly.

If handling size_split please handle num attribute if possible or raise exception accordingly.

Split had num_split attribute and SplitV had size_splits attribute. As of this PR supported Split operator

srkreddy1238 · 2018-08-09T11:41:22Z

nnvm/python/nnvm/frontend/tensorflow.py

+        if inputs[1] in attr['_input_shapes']:
+            conv_param_weights = tuple(attr['_input_shapes'][inputs[1]])
+        else:
+            conv_param_weights = params[inputs[1].list_output_names()[0]].shape


conv_param_weights -> weights_shape (it's no more a param hence.)

srkreddy1238 · 2018-08-09T13:25:13Z

nnvm/python/nnvm/frontend/tensorflow.py

-        attr_new['alpha'] = attr.get('alpha', 1) * size
-        attr_new['beta'] = attr.get('beta', 0.5)
-        return AttrCvt(op_name='lrn')(new_inputs, attr_new)
+        depth_radius = attr.get('depth_radius', 2)


I think that was a bug passing empty inputs new_inputs = []
Removing new_inputs and using inputs instead should work for alexnet as well.
Keep the remaining implementation default value handling.

@siju-samuel can you confirm the depth_radius default value as 5 which results size=11 ? or is it a typo !!

depth_radius = attr.get('depth_radius', 5) size = (depth_radius * 2) + 1

Test case was passed as the input was taken from params instead.
#1546 should fix this issue by replacing all const inputs to Placeholders.

srkreddy1238 · 2018-08-09T13:31:48Z

nnvm/python/nnvm/frontend/tensorflow.py

+                        input_shapes[input_sym] = self._output_shapes[
+                            node_input_key].__getitem__(slot_num)
+                        attr['_input_shapes'] = input_shapes
+                    except KeyError:


It's the same statement which cause 2 exceptions in two different cases, one try on input_sym = self._nodes[node_input_key].__getitem__(slot_num) with two except should work. Pls check.

1. Review comment reworked.

srkreddy1238 · 2018-08-09T16:29:13Z

nnvm/tests/python/frontend/tensorflow/test_forward.py

+        tf_output = run_tf_graph(sess, [np_data], ['in_data:0'], 'split:0')
+        tvm_output = run_tvm_graph(final_graph_def, [np_data], ['in_data'],
+                                   tf_output.shape, dtype)
+        np.testing.assert_allclose(tf_output, tvm_output, atol=1e-5, rtol=1e-5)


run_tf_graph is not designed to result a list. Also we pass 'split:0' to return only first result.
run_tvm_graph can return list if output_shape, output_dtypes are lists.

Please check.

1. run_tf_graph handle for multiple output node names support. 2. Split test case review comment reworked.

Dayananda-V · 2018-08-09T20:14:23Z

thanks @srkreddy1238.

Reworked for accepted review comments. Welcome more review from @yzhliu

srkreddy1238 · 2018-08-13T03:48:39Z

nnvm/tests/python/frontend/tensorflow/test_forward.py

-    output_data = sess.run(tensor, input_dict)
+    if isinstance(output_node, list):
+        output_list = []
+        for node_name in output_node:


Can run one time by passing list of tensors to run method.

Address the problem with update rework solution.

1. Tensorflow fetch output handle for list of node output as per review comments.

… into tf_lrn_split_fix

yzhliu · 2018-08-26T06:37:59Z

@dayanandasiet Can you confirm the attr implement (where @srkreddy1238 comment 'Suggest to use transforms instead of ignores & extras') is good? Otherwise LGTM. Thanks.

srkreddy1238 · 2018-08-27T03:51:50Z

nnvm/tests/python/frontend/tensorflow/test_forward.py

+                                        bias=bias,
+                                        name="local_response_normalization")
+    np_data = np.random.uniform(size=ip_shape).astype(dtype)
+    with tf.Session() as sess:


#1546 adds compare_tf_with_tvm which does comparing tf and tvm.

Suggest to use it.

srkreddy1238 · 2018-08-27T04:49:50Z

nnvm/tests/python/frontend/tensorflow/test_forward.py

+#######################################################################
+# LRN (Local Response Normalization)
+# ----------------------------------
+def _test_lrn(ip_shape, depth_radius=2, alpha=1e-05, beta=0.75, bias=1.0):


Ref. we don't need to explicitly set the default values. TF can handle them if not set.

Suggest to ignore to allow TF default values play role here.

yuruofeifei · 2018-09-06T18:37:57Z

Hi @dayanandasiet ,
Do you have any update on this PR?

yzhliu · 2018-09-18T01:01:24Z

Any update @dayanandasiet ?

yuruofeifei · 2018-09-27T00:00:24Z

Hi @dayanandasiet , do you have any update on this PR?
I'm currently working on TF convertor which will need split operator.

Dayananda-V · 2018-10-23T03:50:55Z

Will fix review comment and push this changes with new PR. As of now closing this PR.

TensorFlow Split and LRN operator support

cef9484

1. Tensorflow input parse logic updated 2. LRN bugfix 3. Split operator added 4. Input shapes parse logic updated To support alexnet on tensorflow(https://www.cs.toronto.edu/~guerzhoy/tf_alexnet/)

Dayananda-V mentioned this pull request Aug 8, 2018

Tensorflow Split operator supported for finetuned Alexnet model #1531

Closed

Dayananda-V added 4 commits August 9, 2018 13:04

TensorFlow Split and LRN operator support

313000d

1. Compilation issue fix 2. '_input_shapes' handle fix across operator frontend

TensorFlow Split and LRN operator support

d4cb1a9

1. Pylint issue fix

Merge branch 'master' into tf_lrn_split_fix

f1a5376

TensorFlow Split and LRN operator support

9f6f77d

1. Comment refactored

srkreddy1238 requested changes Aug 9, 2018

View reviewed changes

TensorFlow Split and LRN operator support

da194ce

1. Review comment reworked.

srkreddy1238 requested changes Aug 9, 2018

View reviewed changes

TensorFlow Split and LRN operator support

937d040

1. run_tf_graph handle for multiple output node names support. 2. Split test case review comment reworked.

Merge branch 'master' into tf_lrn_split_fix

04f51bf

srkreddy1238 requested changes Aug 13, 2018

View reviewed changes

yzhliu added status: review in progress status: need update need update based on feedbacks labels Aug 13, 2018

Dayananda-V added 6 commits August 14, 2018 10:11

TensorFlow Split and LRN operator support

d06ca78

1. Tensorflow fetch output handle for list of node output as per review comments.

Merge branch 'tf_lrn_split_fix' of https://github.com/dayanandasiet/tvm…

78e4363

… into tf_lrn_split_fix

Merge branch 'master' into tf_lrn_split_fix

7de2d28

Merge branch 'master' into tf_lrn_split_fix

64f7635

Merge branch 'master' into tf_lrn_split_fix

68d9111

Merge branch 'master' into tf_lrn_split_fix

c1e374a

yzhliu self-assigned this Aug 20, 2018

Dayananda-V added 2 commits August 21, 2018 08:45

Merge branch 'master' into tf_lrn_split_fix

fbed5b1

Merge branch 'master' into tf_lrn_split_fix

47eca2c

srkreddy1238 requested changes Aug 27, 2018

View reviewed changes

Dayananda-V closed this Oct 23, 2018

TensorFlow Split and LRN operator support for Alexnet Model #1572

TensorFlow Split and LRN operator support for Alexnet Model #1572

Uh oh!

Conversation

Dayananda-V commented Aug 8, 2018

Uh oh!

Dayananda-V commented Aug 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dayananda-V Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dayananda-V commented Aug 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yzhliu commented Aug 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dayananda-V Aug 9, 2018 •

edited

Loading