Universal Transformer appears to be buggy and not converging correctly

[rllin-fathom]

Summary

Universal Transformer appears to be buggy and not converging correctly:

• Universal transformer does not converge on multi_nli as of the latest tensor2tensor master (9729521). See below for reproduction

• UT does converge on multi_nli as of August 3 2018 commit 5fff1ca (we didn’t run this fully out, but it was making meaningful progress, unlike below, so we terminated it and considered it successful).

• To confirm this was not simply an odd issue with multi_nli, we tried UT with a number of other problems (exact repo not shown below), including ‘lambada_rc’ and ‘stanford_nli’ (run at commit ca628e4 from around October 16 2018) All of these failed to converge.

Environment information

Docker image based off nvidia/cuda:9.0-devel-ubuntu16.04

Tf version: tensorflow-gpu=1.11.0

T2t version: Tensor2tensor master at commit 9729521 on Oct 30 2018.

We also saw this failed behavior on tf-nightly-gpu==1.13.0.dev20181022

Reproduce

Problem: multi_nli
Model: universal_transformer
Hparams_set: universal_transformer_tiny

python3 /usr/src/t2t/tensor2tensor/bin/t2t-trainer
--data_dir="DATA_DIR"
--eval_early_stopping_steps="10000"
--eval_steps="10000"
--generate_data="True"
--hparams=""
--hparams_set="universal_transformer_tiny"
--iterations_per_loop="2000"
--keep_checkpoint_max="80"
--local_eval_frequency="2000"
--model="universal_transformer"
--output_dir="OUTPUT_DIR"
--problem="multi_nli"
--t2t_usr_dir="T2T_USR_DIR"
--tmp_dir="T2T_TMP_DIR"

Run was stopped after 50000 steps due to lack of convergence as loss fluctuates between 1.098 and 1.099.

INFO:tensorflow:Saving dict for global step 50000: global_step = 50000, loss = 1.0991247, metrics-multi_nli/targets/accuracy = 0.31821653, metrics-multi_nli/targets/accuracy_per_sequence = 0.31821653, metrics-multi_nli/targets/accuracy_top5 = 1.0, metrics-multi_nli/targets/approx_bleu_score = 0.7479816, metrics-multi_nli/targets/neg_log_perplexity = -1.099124, metrics-multi_nli/targets/rouge_2_fscore = 0.0, metrics-multi_nli/targets/rouge_L_fscore = 0.31869644

[afrozenator]

Paging @MostafaDehghani :)

[rllin-fathom]

digging through the diff between 5fff1ca (roughly August 3 2018 our last working merge from upstream) and 9729521 (master as of yesterday)

5fff1ca...tensorflow:9729521bc3cd4952c42dcfda53699e14bee7b409

we just ran 9729521 with

tensor2tensor/tensor2tensor/models/research/universal_transformer.py

Line 399 in 9729521

hparams.add_hparam("transformer_ffn_type", "fc")

switched back to sepconv
and

tensor2tensor/tensor2tensor/models/research/universal_transformer.py

Line 419 in 9729521

hparams.add_hparam("use_memory_as_final_state", True)

back to False and we are seeing convergence (accuracy 0.49 at step 2000).

going to do runs to narrow which one of those changes is responsible.

[rllin-fathom]

switching only

tensor2tensor/tensor2tensor/models/research/universal_transformer.py

Line 419 in 9729521

hparams.add_hparam("use_memory_as_final_state", True)

back to False gets me a run that is converging.

fix here #1192

[afrozenator]

It seems to have been changed in #1036 by @MostafaDehghani

[MostafaDehghani]

Oh! Yes, this is my fault! I'm really really sorry!
I actually fixed this in a PR (9bec469#diff-750434e84a49123af08c4226eb4962f7R419), but I closed that PR because of another problem! Anywyas, I'm sending a new PR to fix this!

[MostafaDehghani]

Fixed it in #1194.

[cbockman]

@afrozenator @MostafaDehghani does this actually fix this issue? Apologies if I'm not following something here:

#1:
Behavior before your PR:

use_memory_as_final_state=True
universal_transformer_layer returns (output, extra_output)
universal_transformer_encoder returns common_layers.layer_preprocess(output, hparams), extra_output

#2:
Behavior after your PR:
use_memory_as_final_state=False
universal_transformer_layer returns (output, extra_output)
universal_transformer_encoder returns common_layers.layer_preprocess(output, hparams), extra_output

I.e., it doesn't seem like this changes anything?

#3:
Behavior with #1192:
use_memory_as_final_state=False
universal_transformer_layer returns (extra_output, extra_output)
universal_transformer_encoder returns common_layers.layer_preprocess(extra_output, hparams), extra_output

We can verify this manually with a training run, but not clear to me how these PRs could help.

[MostafaDehghani]

Thanks @cbockman. mmmm....Isn't it like this:

Behavior before #1194:
use_memory_as_final_state=True
universal_transformer_layer returns (extra_output, extra_output)
Which leads to:
universal_transformer_encoder returns common_layers.layer_preprocess(extra_output, hparams), extra_output
universal_transformer_decoder returns common_layers.layer_preprocess(extra_output, hparams), extra_output

Behavior after #1194:
use_memory_as_final_state=False
universal_transformer_layer returns (output, extra_output)
Which leads to:
universal_transformer_encoder returns common_layers.layer_preprocess(output, hparams),
extra_output
universal_transformer_decoder returns common_layers.layer_preprocess(output, hparams), extra_output

?

Also I'm not sure about what you've said about Behavior with #1192.

p.s.
Note that removing the two lines in the universal_transformer_encoder don't change anything, as it was checking for the use_memory_as_last_state not the use_memory_as_final_state! use_memory_as_last_state were not defined, so if hparams.get("use_memory_as_last_state", False): was alwayse false :)

[cbockman]

EDIT: scratch that!

I misunderstood what .get was doing with hparams (https://www.tensorflow.org/api_docs/python/tf/contrib/training/HParams#get).

We're running a training run to verify, but I believe this issue can be closed, thanks for the quick cycle everyone.