Changing the names and cleaning hparams_sets of the Universal Transformer based on the NIPS submission. #837

MostafaDehghani · 2018-05-25T08:38:29Z

Hey Lukasz and Ryan,

Based on the submitted paper at NIPS I changed the names used in the code to Universal Transformer (instead of the Recurrent Transformer) and also updated the names of hparams_set based on the submission and removed those that are not useful.
Cheers,

…sion

…er and Adaptive Universal Transformer)

afrozenator · 2018-05-25T22:12:07Z

Hi @MostafaDehghani, thanks for the PR. Please update such that the Travis tests pass.

MostafaDehghani · 2018-05-26T07:11:09Z

Hi @afrozenator ,

Thanks for your reply.

I believe this is because of the problem with tf.foldl in public current version of the Tensorflow which doesn't support nested/multiple tensors for input and initialization.
If you run the original code of the r_transofmrer from T2T using external Tensroflow, you'll get the same error and the error is not due to changes in this PR (I just changed names of variables).

When I was at Google, I checked in a CL for changing the foldl and foldr to support nested tensors and it should be available internally but seems it's not publicly available yet (in Tensorflow 1.8).
Can you check with @lukaszkaiser about the foldl/r CL and let him know about this error?

rsepassi · 2018-05-26T16:34:16Z

You need to update the name in the --ignore= line in the .travis.yml file. Travis must be green before merging this CL.

rsepassi · 2018-05-26T16:35:07Z

Also you can see the Travis test logs by clicking "Details" next to the check so that you're not flying blind.

stefan-it · 2018-05-26T16:49:04Z

@MostafaDehghani Is there a kind of preprint available of that paper? I'm really interested in (I made some experiments with the r_transformer model and it turns out that that it converges ~ 20.000 steps earlier than the original transformer model (single model, 1 GPU)

…ansformer_test.py until tf.foldl is updated in tensorflow 1.9

MostafaDehghani · 2018-05-26T17:27:45Z

Thanks @rsepassi. Solved the problem.

MostafaDehghani · 2018-05-26T17:39:19Z

@stefan-it, glad that you tried it :) We submitted the paper to NIPS and we're going to submit a preprint to arXiv soon (next week or so). In the meantime, let me know if you have any question.

afrozenator · 2018-05-27T08:26:30Z

Thanks for fixing that @MostafaDehghani and @rsepassi for figuring it out. Ryan was showing me around the GitHub workflow :)

@MostafaDehghani - I was trying to automatically sync this internally, and if I'm not mistaken you haven't signed the CLA yet? -- you just have to do this once.

Head over to https://cla.developers.google.com/ to see your current agreements on file or to sign a new one.

If you have already signed a CLA then maybe my automatic import is broken, let me know -- it would be useful to me -- I'll then sync in your changes the painful way :)

MostafaDehghani · 2018-05-27T08:33:27Z

Hi @afrozenator, Great!

I have signed the Google Individual CLA on Nov 16, 2017 and I can see it in https://cla.developers.google.com/. So this might be a problem on your side.
Thanks for taking care of this and let me know if I need to do anything.

afrozenator · 2018-05-29T20:13:52Z

.travis.yml

@@ -54,14 +54,13 @@ script:
  #   * visualization_test
  #   * model_rl_experiment_test
  # algorithmic_math_test: flaky
-  # r_transformer_test: requires new feature in tf.foldl (rm with TF 1.9)
-  - pytest


Hi @MostafaDehghani -- Can you put this "- pytest" line back ... I've worked out how to import this automatically internally.

Thanks again for your patience as I continue to work on the tooling!

afrozenator · 2018-05-29T22:01:34Z

Hi @MostafaDehghani - One last thing -- Travis seems to be unhappy because of the pylint errors in the newly added files, can you fix those?

Copy/pasting for convenience:

C0303:  4 : Trailing whitespace [trailing-whitespace]
C0303:  5 : Trailing whitespace [trailing-whitespace]
C0303:  6 : Trailing whitespace [trailing-whitespace]
C0330: 64 : Wrong hanging indentation (add 2 spaces).
      universal_transformer_util.universal_transformer_encoder(
      ^ | [bad-continuation]
C0330: 65 : Wrong hanging indentation (add 1 space).
         encoder_input,
         ^| [bad-continuation]
C0330: 66 : Wrong hanging indentation (add 1 space).
         self_attention_bias,
         ^| [bad-continuation]
C0330: 67 : Wrong hanging indentation (add 1 space).
         hparams,
         ^| [bad-continuation]
C0330: 68 : Wrong hanging indentation (add 1 space).
         nonpadding=transformer.features_to_nonpadding(features, "inputs"),
         ^| [bad-continuation]
C0330: 69 : Wrong hanging indentation (add 1 space).
         save_weights_to=self.attention_weights))
         ^| [bad-continuation]
C0330:112 : Wrong hanging indentation (add 2 spaces).
      universal_transformer_util.universal_transformer_decoder(
      ^ | [bad-continuation]
C0330:113 : Wrong hanging indentation (add 2 spaces).
        decoder_input,
        ^ | [bad-continuation]
C0330:114 : Wrong hanging indentation (add 2 spaces).
        encoder_output,
        ^ | [bad-continuation]
C0330:115 : Wrong hanging indentation (add 2 spaces).
        decoder_self_attention_bias,
        ^ | [bad-continuation]
C0330:116 : Wrong hanging indentation (add 2 spaces).
        encoder_decoder_attention_bias,
        ^ | [bad-continuation]
C0330:117 : Wrong hanging indentation (add 2 spaces).
        hparams,
        ^ | [bad-continuation]
C0330:118 : Wrong hanging indentation (add 2 spaces).
        nonpadding=nonpadding,
        ^ | [bad-continuation]
C0330:119 : Wrong hanging indentation (add 2 spaces).
        save_weights_to=self.attention_weights))
        ^ | [bad-continuation]
C0301:246 : Line too long (85/80) [line-too-long]
C0330:275 : Wrong hanging indentation (add 2 spaces).
      universal_transformer_util.universal_transformer_encoder(
      ^ | [bad-continuation]
C0330:276 : Wrong hanging indentation (add 1 space).
         encoder_input,
         ^| [bad-continuation]
C0330:277 : Wrong hanging indentation (add 1 space).
         self_attention_bias,
         ^| [bad-continuation]
C0330:278 : Wrong hanging indentation (add 1 space).
         hparams,
         ^| [bad-continuation]
C0330:279 : Wrong hanging indentation (add 1 space).
         nonpadding=transformer.features_to_nonpadding(features, "inputs"),
         ^| [bad-continuation]
C0330:280 : Wrong hanging indentation (add 1 space).
         save_weights_to=self.attention_weights))
         ^| [bad-continuation]

…nsor

…ensor2tensor" This reverts commit d016ab3, reversing changes made to 41434d4.

MostafaDehghani · 2018-05-30T08:05:50Z

Hi @afrozenator, it should be ready to go! let me know if there's any other problem.

This reverts commit d885757, reversing changes made to 42981f3.

afrozenator · 2018-05-30T17:10:55Z

Hi @MostafaDehghani -- Thanks for fixing the linting errors. Travis still seems to be complaining, I've restarted it, I'm not sure if it is related to your PR. Let me investigate. Lets get this in today.

Thanks!

afrozenator · 2018-05-30T17:38:45Z

Hi @MostafaDehghani -- I guess your commits are on top of an older version of T2T -- Could you 'sync to head'? (I don't even know what it is called in git parlance).

Meanwhile I'm running presubmit on my internal patch of this PR.

Thanks!

rsepassi · 2018-05-30T18:14:52Z

to update, should be able to run on your branch:

git fetch upstream # assuming the tensor2tensor remote is called "upstream"
git rebase upstream/master

…sion

…er and Adaptive Universal Transformer)

…ansformer_test.py until tf.foldl is updated in tensorflow 1.9

…ensor2tensor" This reverts commit d016ab3, reversing changes made to 41434d4.

This reverts commit d885757, reversing changes made to 42981f3.

afrozenator

This seems to be 'reverting' a bunch of changes instead of applying those in this PR -- @MostafaDehghani can you check?

For example: Head ISSUE_TEMPLATE - https://raw.githubusercontent.com/tensorflow/tensor2tensor/master/ISSUE_TEMPLATE.md vs yours https://github.com/tensorflow/tensor2tensor/pull/837/files#diff-0b6c1416b1293274c4e2d11fb49e1795

afrozenator · 2018-05-30T18:35:01Z

A good sanity check is that the number of files changed should be the 5 that you changed

MostafaDehghani · 2018-05-30T18:37:39Z

@afrozenator yep.. sorry for that!
I'm getting confused... let me figure out what's wrong!

This reverts commit 008ff4c, reversing changes made to a787228.

afrozenator · 2018-05-30T18:59:36Z

No problem @MostafaDehghani -- just let me know when you are satisfied

…hghani/tensor2tensor"" This reverts commit 06eb36d.

This reverts commit a787228.

MostafaDehghani · 2018-05-30T19:26:07Z

@afrozenator fixed the "reverts" I've done mistakenly.
When do git rebase upstream/master I get a lot of conflicts to fix manually, including all the changes I made. Do you have any idea what is the problem?

afrozenator · 2018-05-30T21:14:00Z

@MostafaDehghani : @rsepassi is the expert on git here :) I'll let him comment.

Travis seems to still have some issues, because the parameters on encode and decode were changed.

This should be simple to fix, right? IIUC you are just deleting the arguments, it maybe possible to leave them as is and then add del losses (in encode) and similarly in decode to make lint happy?

Thanks

$ if [[ "$TRAVIS_PYTHON_VERSION" == "2.7" ]] && [[ "$TF_VERSION" == "$TF_LATEST"  ]]; then pylint -j 2 tensor2tensor; fi
Using config file /home/travis/build/tensorflow/tensor2tensor/pylintrc
************* Module tensor2tensor.models.research.universal_transformer
W0221: 42 UniversalTransformer.encode: Parameters differ from overridden 'encode' method [arguments-differ]
W0221: 86 UniversalTransformer.decode: Parameters differ from overridden 'decode' method [arguments-differ]
W0221:261 UniversalTransformerEncoder.encode: Parameters differ from overridden 'encode' method [arguments-differ]

afrozenator · 2018-05-31T14:18:44Z

Awesome @MostafaDehghani -- merging now.

PiperOrigin-RevId: 198719318

MostafaDehghani · 2018-05-31T15:53:37Z

Great...Thanks a lot @afrozenator :)

afrozenator · 2018-05-31T18:27:37Z

Thank you @MostafaDehghani for the PR and the persistence :)

Many regards

…rmer based on the NIPS submission. (tensorflow#837) * Changing the names and cleaning hparams_sets based on the NIPS submission * Adding some ranges for haparams_set (for both the Universal Transformer and Adaptive Universal Transformer) * Updating name of r_transofmer in .travis.yml --to ignore universal_transformer_test.py until tf.foldl is updated in tensorflow 1.9 * Resolving the .travis.yml conflict. * Update .travis.yml * fixing lint errors * Revert "Merge branch 'master' of https://github.com/MostafaDehghani/tensor2tensor" This reverts commit d016ab3, reversing changes made to 41434d4. * Revert "Merge branch 'master' into master" This reverts commit d885757, reversing changes made to 42981f3. * Changing the names and cleaning hparams_sets based on the NIPS submission * Adding some ranges for haparams_set (for both the Universal Transformer and Adaptive Universal Transformer) * Updating name of r_transofmer in .travis.yml --to ignore universal_transformer_test.py until tf.foldl is updated in tensorflow 1.9 * fixing lint errors * Revert "Merge branch 'master' of https://github.com/MostafaDehghani/tensor2tensor" This reverts commit d016ab3, reversing changes made to 41434d4. * Revert "Merge branch 'master' into master" This reverts commit d885757, reversing changes made to 42981f3. * Revert "merge text_encoder" This reverts commit 008ff4c, reversing changes made to a787228. * Revert "Revert "Merge branch 'master' of https://github.com/MostafaDehghani/tensor2tensor"" This reverts commit 06eb36d. * Revert "Revert "Merge branch 'master' into master"" This reverts commit a787228. * fixing arguments of encode and decode to match the overridden methods

PiperOrigin-RevId: 198719318

MostafaDehghani added 2 commits May 24, 2018 15:16

Changing the names and cleaning hparams_sets based on the NIPS submis…

ea20260

…sion

Adding some ranges for haparams_set (for both the Universal Transform…

01f5f68

…er and Adaptive Universal Transformer)

MostafaDehghani added 2 commits May 26, 2018 19:09

Updating name of r_transofmer in .travis.yml --to ignore universal_tr…

7e99430

…ansformer_test.py until tf.foldl is updated in tensorflow 1.9

Resolving the .travis.yml conflict.

c5cf25e

Merge branch 'master' into master

d9b2806

afrozenator added the cla: yes PR author has signed CLA label May 27, 2018

MostafaDehghani mentioned this pull request May 29, 2018

*bug* t2t-decoder: ValueError with r_transformer model #767

Closed

afrozenator reviewed May 29, 2018

View reviewed changes

Update .travis.yml

35b6640

MostafaDehghani added 3 commits May 30, 2018 09:12

fixing lint errors

41434d4

Merge branch 'master' of https://github.com/MostafaDehghani/tensor2te…

d016ab3

…nsor

Revert "Merge branch 'master' of https://github.com/MostafaDehghani/t…

42981f3

…ensor2tensor" This reverts commit d016ab3, reversing changes made to 41434d4.

MostafaDehghani added 2 commits May 30, 2018 16:30

Merge branch 'master' into master

d885757

Revert "Merge branch 'master' into master"

b065b99

This reverts commit d885757, reversing changes made to 42981f3.

MostafaDehghani added 2 commits May 30, 2018 20:17

Changing the names and cleaning hparams_sets based on the NIPS submis…

4dce5be

…sion

Adding some ranges for haparams_set (for both the Universal Transform…

959afc8

…er and Adaptive Universal Transformer)

MostafaDehghani added 5 commits May 30, 2018 20:20

Updating name of r_transofmer in .travis.yml --to ignore universal_tr…

aa1070f

…ansformer_test.py until tf.foldl is updated in tensorflow 1.9

fixing lint errors

9d19e3c

Revert "Merge branch 'master' of https://github.com/MostafaDehghani/t…

06eb36d

…ensor2tensor" This reverts commit d016ab3, reversing changes made to 41434d4.

Revert "Merge branch 'master' into master"

a787228

This reverts commit d885757, reversing changes made to 42981f3.

merge text_encoder

008ff4c

afrozenator suggested changes May 30, 2018

View reviewed changes

Revert "merge text_encoder"

442f53c

This reverts commit 008ff4c, reversing changes made to a787228.

MostafaDehghani added 2 commits May 30, 2018 21:13

Revert "Revert "Merge branch 'master' of https://github.com/MostafaDe…

6b21125

…hghani/tensor2tensor"" This reverts commit 06eb36d.

Revert "Revert "Merge branch 'master' into master""

f741f94

This reverts commit a787228.

fixing arguments of encode and decode to match the overridden methods

a47d41a

afrozenator merged commit 44defb5 into tensorflow:master May 31, 2018

tensorflow-copybara pushed a commit that referenced this pull request May 31, 2018

internal merge of PR #837

95485e4

PiperOrigin-RevId: 198719318

whr94621 pushed a commit to whr94621/tensor2tensor that referenced this pull request Jun 12, 2018

internal merge of PR tensorflow#837

f8d4992

PiperOrigin-RevId: 198719318

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing the names and cleaning hparams_sets of the Universal Transformer based on the NIPS submission. #837

Changing the names and cleaning hparams_sets of the Universal Transformer based on the NIPS submission. #837

MostafaDehghani commented May 25, 2018

afrozenator commented May 25, 2018

MostafaDehghani commented May 26, 2018

rsepassi commented May 26, 2018

rsepassi commented May 26, 2018

stefan-it commented May 26, 2018

MostafaDehghani commented May 26, 2018

MostafaDehghani commented May 26, 2018

afrozenator commented May 27, 2018

MostafaDehghani commented May 27, 2018

afrozenator May 29, 2018

MostafaDehghani May 29, 2018

afrozenator commented May 29, 2018

MostafaDehghani commented May 30, 2018

afrozenator commented May 30, 2018

afrozenator commented May 30, 2018

rsepassi commented May 30, 2018

afrozenator left a comment

afrozenator commented May 30, 2018

MostafaDehghani commented May 30, 2018

afrozenator commented May 30, 2018

MostafaDehghani commented May 30, 2018

afrozenator commented May 30, 2018

afrozenator commented May 31, 2018

MostafaDehghani commented May 31, 2018

afrozenator commented May 31, 2018

Changing the names and cleaning hparams_sets of the Universal Transformer based on the NIPS submission. #837

Changing the names and cleaning hparams_sets of the Universal Transformer based on the NIPS submission. #837

Conversation

MostafaDehghani commented May 25, 2018

afrozenator commented May 25, 2018

MostafaDehghani commented May 26, 2018

rsepassi commented May 26, 2018

rsepassi commented May 26, 2018

stefan-it commented May 26, 2018

MostafaDehghani commented May 26, 2018

MostafaDehghani commented May 26, 2018

afrozenator commented May 27, 2018

MostafaDehghani commented May 27, 2018

afrozenator May 29, 2018

Choose a reason for hiding this comment

MostafaDehghani May 29, 2018

Choose a reason for hiding this comment

afrozenator commented May 29, 2018

MostafaDehghani commented May 30, 2018

afrozenator commented May 30, 2018

afrozenator commented May 30, 2018

rsepassi commented May 30, 2018

afrozenator left a comment

Choose a reason for hiding this comment

afrozenator commented May 30, 2018

MostafaDehghani commented May 30, 2018

afrozenator commented May 30, 2018

MostafaDehghani commented May 30, 2018

afrozenator commented May 30, 2018

afrozenator commented May 31, 2018

MostafaDehghani commented May 31, 2018

afrozenator commented May 31, 2018