[Tutorial]NLP Sequence to sequence model for translation #1815

siju-samuel · 2018-10-04T10:42:30Z

Seq2Seq encoder/decoder model tutorial
Timestep support for LSTM Cells

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.

siju-samuel · 2018-10-06T02:15:19Z

@kazum @srkreddy1238 @masahi @PariksheetPinjari909 please have one round of review. thanks

siju-samuel · 2018-10-18T17:23:30Z

@yzhliu @kazum could you please have a look at this PR. TIA

kazum · 2018-10-18T21:17:26Z

@siju-samuel, sorry for my late response. I think I can take a look this weekend.

kazum · 2018-10-20T20:21:40Z

nnvm/python/nnvm/frontend/keras.py

+    # In case of RNN dense, input shape will be (1, 1, n)
+    if input_dim > 2:
+        input_shape = tuple(dim if dim else 1 for dim in _as_list(input_shape)[0])
+        if input_dim != 3 and input_shape[0] != input_shape[1] != 1:


I think this check should be

Suggested change

if input_dim != 3 and input_shape[0] != input_shape[1] != 1:

if input_dim != 3 or input_shape[0] != 1 or input_shape[1] != 1:

or

Suggested change

if input_dim != 3 and input_shape[0] != input_shape[1] != 1:

if not (input_dim == 3 and input_shape[0] == input_shape[1] == 1):

kazum · 2018-10-20T20:33:06Z

nnvm/python/nnvm/frontend/keras.py

+    in_data = _sym.squeeze(in_data, axis=0)
+    in_data = _sym.split(in_data, indices_or_sections=time_steps, axis=0)
+    for step in range(time_steps):
+        ixh1 = _sym.dense(in_data[step], kernel_wt, use_bias=False, units=units)


I think no need to use range here.

for step in in_data: ixh1 = _sym.dense(step, kernel_wt, use_bias=False, units=units)

kazum · 2018-10-20T20:39:21Z

nnvm/python/nnvm/frontend/keras.py

-                        sym = symtab.get_var(sym_name, must_contain=True)
-                    insym.append(sym)
-
+                    # In some models, sym_name may not be available in inbound_nodes


Can you share an example model where sim_name is not available in inbound_nodes?

dmlc/web-data#124
Put the contents of keras folder to your execution path
Any encoder-decoder models will have this issue, since keras treat this as 2 different networks, but its not completely independant, (decoder network input linked to encoder output)

This change ignores the encoder outputs in the decoder model, and uses zeros as an initial state instead. It looks wrong to me.

The root cause is that you are processing layers which are included in the imported model but not relevant to the current model. You can skip such layers with the below code.

if not model._node_key(keras_layer, node_idx) in model._network_nodes: continue

Note that model._network_nodes contains keys of all nodes relevant to the current model.

kazum · 2018-10-20T20:42:19Z

tutorials/nnvm/nlp/keras_s2s_translate.py

+# Base location for model related files.
+repo_base = 'https://github.com/dmlc/web-data/raw/master/keras/models/s2s_translate/'
+model_url = os.path.join(repo_base, model_file)
+data_url = os.path.join(repo_base, data_file)


The model and data files are not found in the repository yet. Can you share them?

dmlc/web-data#124

PariksheetPinjari909 · 2018-10-23T12:49:10Z

tutorials/nnvm/nlp/keras_s2s_translate.py

+# Randonly take some text and translate
+for seq_index in range(100):
+    # Take one sequence and try to decode.
+    index = random.randint(1, num_samples)


If the model is trained on num_samples, i will suggest to test the model on validation dataset.

PariksheetPinjari909 · 2018-10-23T12:51:13Z

tutorials/nnvm/nlp/keras_s2s_translate.py

+download(data_url, model_file)
+
+latent_dim = 256  # Latent dimensionality of the encoding space.
+num_samples = 10000  # Number of samples to train on.


In the script, pretrained model is used, no training is done. Can you update the comment.

siju-samuel · 2018-11-06T03:11:03Z

@kazum @PariksheetPinjari909 could you please review once again. Thanks.

kazum · 2018-11-11T19:26:51Z

nnvm/python/nnvm/frontend/keras.py

@@ -131,6 +131,14 @@ def _convert_dense(insym, keras_layer, symtab):
    if keras_layer.use_bias:
        params['use_bias'] = True
        params['bias'] = symtab.new_const(weightList[1])
+    input_shape = keras_layer.input_shape
+    input_dim = len(input_shape)
+    # In case of RNN dense, input shape will be (1, 1, n)


The current version doesn't have a check of input_shape[1] != 1, so the input shape will be (1, m, n)?

merrymercy · 2018-11-25T09:33:34Z

@kazum @PariksheetPinjari909 Could you conclude on this pr?

kazum · 2018-11-25T20:09:33Z

The current version almost looks good to me. I'll approve if my comment (#1815 (comment)) is addressed

PariksheetPinjari909 · 2018-11-26T11:47:39Z

I just ran the model for 10 test samples from 10000 to 10050. In the result the output sequence doesn't match with the sequence provided in dataset. @siju-samuel can you have a look at this.

siju-samuel · 2018-11-27T05:18:28Z

It wont match exactly same as with the actual translation as shown in the text file. This is a only a porting of keras s2s to tvm. so this will match exactly same with keras output. with tvm, accuracy wont improve further.

merrymercy · 2018-11-28T06:08:35Z

Should merge dmlc/web-data#124 @tqchen

tqchen · 2018-11-28T18:21:48Z

@merrymercy dmlc/web-data#124 is merged

* [Tutorial]NLP Sequence to sequence model for translation * Review comments * Review comments updated

siju-samuel mentioned this pull request Oct 4, 2018

Keras seq2seq model weights and training architecture dmlc/web-data#124

Merged

tqchen added the status: need review label Oct 6, 2018

kazum requested changes Oct 20, 2018

View reviewed changes

yzhliu added the status: need update need update based on feedbacks label Oct 23, 2018

PariksheetPinjari909 suggested changes Oct 23, 2018

View reviewed changes

siju-samuel force-pushed the seq2seq branch 4 times, most recently from 3732425 to 807fd96 Compare November 2, 2018 08:29

siju-samuel force-pushed the seq2seq branch from 1882340 to fa94049 Compare November 6, 2018 03:05

kazum requested changes Nov 11, 2018

View reviewed changes

siju-samuel added 3 commits November 26, 2018 09:40

[Tutorial]NLP Sequence to sequence model for translation

d41c084

Review comments

a87069c

Review comments updated

0b6957f

siju-samuel force-pushed the seq2seq branch from fa94049 to 0b6957f Compare November 26, 2018 04:13

PariksheetPinjari909 approved these changes Nov 28, 2018

View reviewed changes

kazum approved these changes Nov 28, 2018

View reviewed changes

merrymercy approved these changes Nov 28, 2018

View reviewed changes

merrymercy merged commit acea3cc into apache:master Nov 29, 2018

tqchen added status: accepted and removed status: need review status: need update need update based on feedbacks labels Nov 29, 2018

FrozenGene pushed a commit to FrozenGene/tvm that referenced this pull request Dec 27, 2018

[Tutorial]NLP Sequence to sequence model for translation (apache#1815)

69b1b63

* [Tutorial]NLP Sequence to sequence model for translation * Review comments * Review comments updated

ZihengJiang mentioned this pull request Feb 1, 2019

TVM 0.5 Release Note #2448

Closed

wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019

[Tutorial]NLP Sequence to sequence model for translation (apache#1815)

f053465

* [Tutorial]NLP Sequence to sequence model for translation * Review comments * Review comments updated

wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019

[Tutorial]NLP Sequence to sequence model for translation (apache#1815)

6077a87

* [Tutorial]NLP Sequence to sequence model for translation * Review comments * Review comments updated

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tutorial]NLP Sequence to sequence model for translation #1815

[Tutorial]NLP Sequence to sequence model for translation #1815

siju-samuel commented Oct 4, 2018 •

edited

Loading

siju-samuel commented Oct 6, 2018 •

edited

Loading

siju-samuel commented Oct 18, 2018

kazum commented Oct 18, 2018

kazum Oct 20, 2018

kazum Oct 20, 2018

kazum Oct 20, 2018

siju-samuel Oct 22, 2018

kazum Oct 27, 2018

kazum Oct 20, 2018

siju-samuel Oct 22, 2018

PariksheetPinjari909 Oct 23, 2018

PariksheetPinjari909 Oct 23, 2018

siju-samuel commented Nov 6, 2018

kazum Nov 11, 2018

merrymercy commented Nov 25, 2018

kazum commented Nov 25, 2018

PariksheetPinjari909 commented Nov 26, 2018

siju-samuel commented Nov 27, 2018

merrymercy commented Nov 28, 2018

tqchen commented Nov 28, 2018

	if input_dim != 3 and input_shape[0] != input_shape[1] != 1:
	if input_dim != 3 or input_shape[0] != 1 or input_shape[1] != 1:

	if input_dim != 3 and input_shape[0] != input_shape[1] != 1:
	if not (input_dim == 3 and input_shape[0] == input_shape[1] == 1):

[Tutorial]NLP Sequence to sequence model for translation #1815

[Tutorial]NLP Sequence to sequence model for translation #1815

Conversation

siju-samuel commented Oct 4, 2018 • edited Loading

siju-samuel commented Oct 6, 2018 • edited Loading

siju-samuel commented Oct 18, 2018

kazum commented Oct 18, 2018

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

siju-samuel commented Nov 6, 2018

Choose a reason for hiding this comment

merrymercy commented Nov 25, 2018

kazum commented Nov 25, 2018

PariksheetPinjari909 commented Nov 26, 2018

siju-samuel commented Nov 27, 2018

merrymercy commented Nov 28, 2018

tqchen commented Nov 28, 2018

siju-samuel commented Oct 4, 2018 •

edited

Loading

siju-samuel commented Oct 6, 2018 •

edited

Loading