[NNVM][TENSORFLOW] LSTM operator and PTB word prediction frontend #1389

joyalbin · 2018-07-06T04:15:14Z

This PR contain:

LSTM operator implementation and layer based operator parsing in Tensorflow
Single LSTMBlockCell layer model support
PTB LSTMBlockCell based model frontend script
Frontend testcases

joyalbin · 2018-07-07T12:07:17Z

@Huyuwei @masahi @srkreddy1238 Please help to review this PR

srkreddy1238

Some initial review.

srkreddy1238 · 2018-07-08T06:04:58Z

nnvm/python/nnvm/frontend/tensorflow.py

+    _, out_shapes = graph_util.infer_shape(g, **shape_dict)
+    return out_shapes
+
+def _stridedSlice():


Now we have strided_slice operator from nnvm. We should use it.

@srkreddy1238 The frontend is using NNVM stridedslice only. But tensorflow have additional mask attributes for stridedslice. This is handled here.

srkreddy1238 · 2018-07-08T06:07:38Z

nnvm/python/nnvm/frontend/tensorflow.py

+                     'Taxis', '_class'])(new_input, attr)
+    return _impl
+
+def _infer_out_shapes(inputs, params):


We have already _input_shapes attribute. Any challenge using it?

_infer_out_shapes is used for finding intermediate node shapes

srkreddy1238 · 2018-07-08T06:10:16Z

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

@@ -544,3 +711,6 @@ def test_forward_mobilenet():
    test_forward_inception_v1()
    test_forward_mobilenet()
    test_forward_variable()
+    test_forward_lstm()


Suggest to add a real time end to end test case for LSTM.

srkreddy1238 · 2018-07-08T06:10:45Z

tutorials/nnvm/from_tensorflow_rnn.py

@@ -0,0 +1,242 @@
+"""
+Tutorial for Tensorflow RNN Models


underline up to the text length pls.

srkreddy1238 · 2018-07-08T06:11:20Z

tutorials/nnvm/from_tensorflow_rnn.py

+sample_data_file = 'simple-examples.tgz'
+sample_url = sample_repo+sample_data_file
+
+ptb_repo = 'https://github.com/joyalbin/dmlc_store/raw/master/trained-models/tf/ptb/pb/'


use dmlc/web-data to store these model.

srkreddy1238 · 2018-07-08T06:12:19Z

tutorials/nnvm/from_tensorflow_rnn.py

+
+###############################################################################
+# Input words
+# ---------------------------------------------


under line up to the text.

Huyuwei · 2018-07-08T21:10:51Z

nnvm/python/nnvm/frontend/tensorflow.py

+
+        o = sigmoid(cs * wco + o)
+        co = tanh(cs)
+        h = co .* o


can remove these math equations and give a link to https://github.com/tensorflow/tensorflow/blob/r1.8/tensorflow/contrib/rnn/python/ops/lstm_ops.py#L41-L114

Huyuwei · 2018-07-08T21:18:13Z

nnvm/python/nnvm/frontend/tensorflow.py

+    """Recurrent network layer handlers.
+
+    Unlike normal operators, recurrent network operators have layer concept.
+    Same operators will be called multiple times (based on number of layers)


Does layer refer to steps? or multiple cells (layers) in stacked rnn? The doc here is not very clear.

@Huyuwei here, layers refers to multiple cells in RNN stack. I have rephrased the comments

Huyuwei · 2018-07-08T21:38:07Z

@joyalbin Seems current implementation only supports step=1 and users need to do unrolling manually in the case of multiple steps. What about adding a wrapper to support multiple steps?

@merrymercy may have some suggestions.

joyalbin · 2018-07-11T06:02:46Z

@Huyuwei , @srkreddy1238 I have reworked on all the review comments and updated the PR

@Huyuwei Current implementation support step=1 at a time. This implementation is similar to tensorflow. Here the model input is one 'word', LSTM input is calculated from the 'word's token. so couldnt able to remove the unrolling.
Can we merge this PR with step=1 and multiple step handling in the next PR? Could you please put more light on avoiding this unrolling to handle multiple steps?

joyalbin · 2018-07-16T03:13:27Z

@Huyuwei , @srkreddy1238 can you please help to review this PR further?

Huyuwei · 2018-07-16T14:43:21Z

nnvm/python/nnvm/frontend/tensorflow.py

+
+    Unlike normal operators, stacked rnn have cells and layer concepts.
+    Each Layer represent a cell in RNN stack. Cells in the same RNN stack
+    sequentialy process input data.


The description here is still a little confusing. Where is the recurrent part?
Can remove it or give a link to some RNN tutorial.

Huyuwei · 2018-07-16T14:53:39Z

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

+    initializer = tf.random_uniform_initializer(config.init_scale,
+                                                config.init_scale)
+    with tf.variable_scope("Model", reuse=None, initializer=initializer):
+        mtest = nnvm.testing.tf.PTBModel(is_training=False, config=config)


mtest is not used? Then testing.tf.PTBModel can be removed

Huyuwei · 2018-07-16T14:54:07Z

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

+    #TVM graph module creation
+    params, m = _get_tvm_graph_module(graph_def)
+
+


remove one blank line

Huyuwei · 2018-07-16T14:57:04Z

@joyalbin Sorry for the late response. Have added some comments.

The lstm part looks good to me. fill, gather, stridedslice operators need review from @srkreddy1238

nishi-t

trivial typo fix

nishi-t · 2018-07-17T01:53:11Z

nnvm/python/nnvm/frontend/tensorflow.py

+            Dict of operator attributes
+
+        params : dict
+            List of  pretrained weights and bias


redundant whitespace after of

nishi-t · 2018-07-17T01:56:32Z

nnvm/python/nnvm/testing/tf.py

+    word_to_id : dict
+        English word to integer id mapping
+    id_to_word : dict
+


Could you remove this blank line?

nishi-t · 2018-07-17T01:58:46Z

nnvm/python/nnvm/frontend/tensorflow.py

+            Dict of operator attributes
+
+        params : dict
+            List of  pretrained weights and bias


Could you remove redundant whitespace after 'of'.

srkreddy1238 · 2018-07-17T02:59:48Z

nnvm/python/nnvm/frontend/tensorflow.py

+                     'Taxis', '_class'])(new_input, attr)
+    return _impl
+
+def _infer_out_shapes(inputs, params):


srkreddy1238 · 2018-07-18T06:15:59Z

nnvm/python/nnvm/frontend/tensorflow.py

+        new_axis_mask = int(attr.get('new_axis_mask', 0))
+        shrink_axis_mask = int(attr.get('shrink_axis_mask', 0))
+
+        #Constant values used forming output shape.


Thanks for cleaning the above logic.
Would suggest to clean the below part as well a bit.

My understanding of all these masks as below. Please correct me if I am wrong.

begin, end: Basically to ignore a value from the inputs and use 0 for begin, max for end.
elipsismask: Only one bit will be set which is used to expand begin, end and strides to size of input dimensions by filling max range in between.
newaxis mask: add new axis in the result based on the mask bits.
shringaxis_mask: shrink the axis from result based on the mask bits.

I see this logic can be split into below logically separable blocks.
1: Handle begin and end masks on begin, end
2: Expand begin and end based on elipsis_mask
3: Apply strided_slice operation
4: Apply reshape operation with newaxis and shrink axis

Also suggest to use python API for converting bitmask into lists of integers.

srkreddy1238 · 2018-07-18T06:17:28Z

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

+    _test_stridedslice((3, 4, 3), [1, -1, 0], [4, -5, 3], [2, -1, 1], 'float32')
+    _test_stridedslice((3, 4, 3), [1, -3, 0], [2, -2, 3], [1, 1, 1], 'float32')
+    _test_stridedslice((3, 4, 3), [1, 1, 0], [4, 4, 3], [2, 1, 1], 'float32')
+    _test_stridedslice((3, 4, 3), [1, 0], [4, 3], [2, 1],


Please add another testcase with begin, end, ellipsis_mask and new/shrink axis mask with multiple axis.

joyalbin · 2018-07-24T08:35:10Z

@srkreddy1238 yes, your understanding is correct. But handling all the 5 masks based on its priority make things bit lengthy.
I have modified the code based on your comments.
Mask logic is changed as you suggested and testcases are added.

@nishi-t @Huyuwei I have handled all the review comments. Please help to approve the changes

Huyuwei · 2018-07-25T00:20:33Z

nnvm/python/nnvm/testing/tf.py

+    return ptb_raw_data(data_path, file_name)
+
+def get_workload_ptb():
+    """ Import mobilenet workload from frozen protobuf


not mobilenet

Huyuwei · 2018-07-25T02:55:00Z

nnvm/python/nnvm/testing/tf.py

+    state = session.run(state_input_name)
+    fetches = [['Model/RNN/RNN/multi_rnn_cell/cell_0/lstm_cell/LSTMBlockCell:1',
+                'Model/RNN/RNN/multi_rnn_cell/cell_0/lstm_cell/LSTMBlockCell:6',
+                'Model/RNN/RNN/multi_rnn_cell/cell_0/lstm_cell/LSTMBlockCell_1:1',


Could you add a comment here? What is fetches? What is in the content of LSTMBlockCell:6?

Huyuwei · 2018-07-25T02:55:35Z

nnvm/python/nnvm/testing/tf.py

+    return int(np.searchsorted(t, 0.5 * s))
+
+def do_tf_sample(session, data, in_states, num_samples):
+    """Sampled from the model"""


data is not used in the function.

@Huyuwei data is used at line 192, could you please correct me if I didn't get the real issue here

@joyalbin sorry, my wrong.

srkreddy1238

@joyalbin thanks, Strided_slice, Gather, Fill operators and GraphProto class modification are good to go.

@Huyuwei can confirm on the LSTM part.

tqchen · 2018-07-25T16:56:33Z

Thanks @joyalbin @Huyuwei @nishi-t @srkreddy1238 , this is merged

…ache#1389)

srkreddy1238 requested changes Jul 8, 2018

View reviewed changes

Huyuwei reviewed Jul 8, 2018

View reviewed changes

joyalbin force-pushed the lstm branch from 3098b31 to fd8dd56 Compare July 11, 2018 05:13

Huyuwei reviewed Jul 16, 2018

View reviewed changes

nishi-t suggested changes Jul 17, 2018

View reviewed changes

srkreddy1238 requested changes Jul 17, 2018

View reviewed changes

srkreddy1238 requested changes Jul 18, 2018

View reviewed changes

Albin Joy added 8 commits July 24, 2018 11:01

LSTM operator and PTB model frontend

e46410b

Removed 'input' statement from tutorial

528ae51

frontend testcase updated

bde88d3

Tensorflow frontend updated

61c630d

Gather frontend testcase is added

187ab8f

Review comment fixed

9f2fa86

Added more testcases for StridedSlice and fixed review comments

7ad8a2a

Output shape logic is separated out

0aea2ae

joyalbin force-pushed the lstm branch from 7438d9c to 0aea2ae Compare July 24, 2018 06:12

CI conflict issue solved

4a88562

Huyuwei reviewed Jul 25, 2018

View reviewed changes

srkreddy1238 approved these changes Jul 25, 2018

View reviewed changes

review comments fixed

7b5319e

Huyuwei approved these changes Jul 25, 2018

View reviewed changes

nishi-t approved these changes Jul 25, 2018

View reviewed changes

tqchen merged commit 9176753 into apache:master Jul 25, 2018

tqchen added the status: accepted label Jul 25, 2018

tqchen pushed a commit to tqchen/tvm that referenced this pull request Aug 4, 2018

[NNVM][TENSORFLOW] LSTM operator and PTB word prediction frontend (ap…

a808a98

…ache#1389)

sergei-mironov pushed a commit to sergei-mironov/tvm that referenced this pull request Aug 8, 2018

[NNVM][TENSORFLOW] LSTM operator and PTB word prediction frontend (ap…

e282ec4

…ache#1389)

		#TVM graph module creation
		params, m = _get_tvm_graph_module(graph_def)

[NNVM][TENSORFLOW] LSTM operator and PTB word prediction frontend #1389

[NNVM][TENSORFLOW] LSTM operator and PTB word prediction frontend #1389

Conversation

joyalbin commented Jul 6, 2018

joyalbin commented Jul 7, 2018

srkreddy1238 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huyuwei commented Jul 8, 2018 • edited Loading

joyalbin commented Jul 11, 2018

joyalbin commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huyuwei commented Jul 16, 2018

nishi-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishi-t Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyalbin commented Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srkreddy1238 left a comment

Choose a reason for hiding this comment

tqchen commented Jul 25, 2018

Huyuwei commented Jul 8, 2018 •

edited

Loading

nishi-t Jul 17, 2018 •

edited

Loading

joyalbin commented Jul 24, 2018 •

edited

Loading