Add TFSpeech2Text #15113

gante · 2022-01-11T20:57:12Z

What does this PR do?

This PR adds a TF port of Speech2Text. A summary of the changes:

This model borrows a lot of code from TFBart, just like Speech2Text borrowed from Bart;
Tried to follow the changes in other PRs to enable smooth interoperation with other parts of transformers (e.g. auto classes), might be missing a few things 👼 ;
This seems to be the first TF model with speech as input, so I had to touch common TF code to enable correct data piping and misc operations (e.g. enable loading Conv1D PT weights into TF);
Likewise, there were a few tests in test_modelling_tf_common.py that didn't quite fit this new kind of model.

TODO:

create TF version of the weights, so we can load a model without from_pt=True

gante · 2022-01-28T20:28:46Z

Tagging @sgugger as a core dev, @patil-suraj as a core dev + original creator of our Speech2Text, and @Rocketknight1 as the TensorFlow boy. Feel free to redirect the reviews if you know of better people to review.

Some pipeline tests are failing almost surely due to the changes in generation_tf_utils.py, as some models expect (encoder_outputs, past) in the past variable and others don't -- having a look, but open to suggestions.

sgugger

Very nice addition, great work!

src/transformers/generation_tf_utils.py

sgugger · 2022-01-28T20:59:51Z

src/transformers/generation_tf_utils.py

@@ -756,6 +759,8 @@ def generate(
            )
            # expand encoder_outputs
            encoder_outputs = (tf.gather(encoder_outputs[0], expanded_batch_idxs, axis=0),)
+            if "attention_mask" in locals():  # vision models don't have this


This check is a bit weird. "attention" mask is in the keyword arguments of this function so it will show up in the locals. Maybe check if it's not set to None?

Excellent point, for some reason I didn't notice that it was in the keyword arguments (and thus the current version would fail) 👍 Checking against None now

sgugger · 2022-01-28T21:00:20Z

src/transformers/generation_tf_utils.py

+            # past drops the `encoder_outputs` during the loop and it may be needed (encoder-decoder models)
+            if len(past) > 1 and encoder_outputs is not None:
+                past = (encoder_outputs, past)


Let's double check with @patrickvonplaten for this change.

I've reworked this part to be less intrusive: the past variable is not touched, encoder_outputs is passed to prepare_inputs_for_generation a few lines below. This means that it will be ignored as part of kwargs in most models, and encoder-decoder models can now explicitly access it if they wish to do so (like TFSpeech2Text does).

This change also solves the assert comments below, in prepare_inputs_for_generation.

src/transformers/modeling_tf_pytorch_utils.py

sgugger · 2022-01-28T21:01:52Z

src/transformers/models/speech_to_text/modeling_tf_speech_to_text.py

+        self.num_heads = num_heads
+        self.dropout = tf.keras.layers.Dropout(dropout)
+        self.head_dim = embed_dim // num_heads
+        assert self.head_dim * num_heads == self.embed_dim, "embed_dim must be divisible by num_heads"


We'll need to fix this assert in the original model (no new assert in the modeling files normally, but this is a copied from).

Copied the PT check (that raises a ValueError) to both models

Well, it turns out that many models used a copy of this 😅 Changed them all 👍

src/transformers/models/speech_to_text/modeling_tf_speech_to_text.py

sgugger · 2022-01-28T21:06:09Z

tests/test_modeling_tf_common.py

+            if model_class.__name__ in ["TFSpeech2TextModel", "TFSpeech2TextForConditionalGeneration"]:
+                inputs = {
+                    "decoder_input_ids": tf.keras.Input(
+                        batch_shape=(2, max_input),
+                        name="decoder_input_ids",
+                        dtype="int32",
+                    ),
+                    "input_ids": tf.keras.Input(batch_shape=(2, 32, max_input), name="input_ids", dtype="float32"),
+                }


Is there another (more general) check we could use here?

By more general do you mean instead of hardcoding the possible names in model_class.__name__? We have TF_MODEL_FOR_SPEECH_SEQ_2_SEQ_MAPPING_NAMES, but it doesn't cover TFSpeech2TextModel :(

tests/test_modeling_tf_common.py

patrickvonplaten · 2022-01-31T13:28:24Z

src/transformers/generation_tf_utils.py

@@ -756,6 +759,8 @@ def generate(
            )
            # expand encoder_outputs
            encoder_outputs = (tf.gather(encoder_outputs[0], expanded_batch_idxs, axis=0),)
+            if attention_mask is not None:  # vision models don't have this
+                attention_mask = tf.gather(attention_mask, expanded_batch_idxs, axis=0)


Is this only needed for the new model or was it a bug to not have done this previously?

I'm a bit worried about this as it could lead to some unexpected errors in the more complicated models like TFRag. Could we maybe run the following tests here to double check that this change is ok:

RUN_SLOW=1 pytest tests/test_modeling_tf_bart.py
RUN_SLOW=1 pytest tests/test_modeling_tf_t5.py
RUN_SLOW=1 pytest tests/test_modeling_tf_rag.py (not that you need some more dependencies to be installed here)

Spot on, it failed for those models.

I've reworked these lines to behave like pytorch's _expand_inputs_for_generation(), which is used in all modalities. I've tested the changes against bart, bert, t5, vit, and s2t -- let me know what you think, @patrickvonplaten

Cool! Could you please also run the slow TFRag tests - it's a bit annoying as it's a huge and slow test, but it's the model that is most prone to break if there are changes in tf generate

I confirm that it passes TFRag tests as well (had to spin up a larger machine 😅 )

Cool! Good for me then

src/transformers/generation_tf_utils.py

src/transformers/models/bart/modeling_tf_bart.py

tests/test_modeling_tf_speech_to_text.py

patrickvonplaten

Looks good to me in general! Maybe just two final things before merging:

Quickly run some slow tests to make sure we haven't broken generate accidently for some of the more complicated models
Should we maybe leave past for now until we improve the naming of generate more globally?

…gface#15262) Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>

* Fix TF Causal LM models' returned logits * Fix expected shape in the tests Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>

fix typo

…nizerBase` `__init__` (huggingface#15454) * replace assert with exception for `padding_side` arg in `PreTrainedTokenizerBase` `__init__` * add test * fix kwargs * reformat test * format * format * fix typo to render the documentation

…st version is available (huggingface#15319) * add new test * update test * remove `tokenizer_file` from `additional_files_names` in `tokenization_utils_base.py` * add `tokenizer_file` for the fast only tokenizer * change global variables layoutxml * remove `"tokenizer_file"` from DPR tokenizer's Global variables * remove `tokenizer_file` from herbert slow tokenizer init * `"tokenizer_file"` from LED tokenizer's Global variables * remove `tokenizer_file` from mbart slow tokenizer init * remove `tokenizer_file` from slow tokenizer template * adapt to versioning * adapt the `test_tokenizer_mismatch_warning` test * clean test * clarify `VOCAB_FILES_NAMES` in tokenization_utils_fast.py * Revert "remove `tokenizer_file` from mbart slow tokenizer init" This reverts commit 0dbb723. * Revert "`"tokenizer_file"` from LED tokenizer's Global variables" This reverts commit 5a3f879. * Revert "remove `tokenizer_file` from herbert slow tokenizer init" This reverts commit f5e1000. * Revert "remove `"tokenizer_file"` from DPR tokenizer's Global variables" This reverts commit da08953. * set `tokenizer_file` in super `__init__` of mbart

Rocketknight1 · 2022-02-01T17:33:48Z

Cloned your branch and did some experimentation and LGTM! I noticed one issue - model.save() seems to encounter problems, but I'm not totally sure of why, or whether it's limited to this model or not. save_pretrained and save_weights worked correctly, and saving in SavedModel format has always been a bit shaky for us, so this isn't critical.

patrickvonplaten · 2022-02-02T22:36:14Z

src/transformers/generation_tf_utils.py

@@ -1131,8 +1119,9 @@ def _generate_beam_search(
        done = [False for _ in range(batch_size)]

        while cur_len < max_length:
+            # import pdb; pdb.set_trace()


can we delete this?

smh not using breakpoint() in the year 2022

patrickvonplaten · 2022-02-02T22:37:48Z

src/transformers/generation_tf_utils.py

+            shape=(-1,),
+        )
+        # prepares text-based inputs
+        if len(shape_list(input_ids)) == 2:


Slightly worried here about the TF vision tests. Can you check this one as well:

RUN_SLOW=1 pytest tests/test_modeling_tf_vision_encoder_decoder.py

Can confirm that they pass 👍

patrickvonplaten

One final vision tests as mentioned before and then it's good for me as well

gante · 2022-02-04T16:52:47Z

Pending the results of automated tests, this should be the last planned commit. We already have facebook/s2t-small-librispeech-asr as a TF model, and I will upload the TF version of the others today.

@patrickvonplaten -- One important final change that I'd like to ask for a double-check is the removal of the positional embeddings weights from the nn.Parameter() wrapper, in the PT model. It is a constant that was not being saved nor loaded, and was causing issues in the pt_tf tests (the TF model had no such variable, and technically it is not a parameter).

To check the changes, I've run:

RUN_SLOW=1 pytest tests/test_modeling_tf_vision_encoder_decoder.py
RUN_SLOW=1 pytest tests/test_modeling_tf_bart.py
RUN_SLOW=1 pytest tests/test_modeling_tf_t5.py
RUN_SLOW=1 pytest tests/test_modeling_tf_rag.py
RUN_SLOW=1 pytest tests/test_modeling_speech_to_text_2.py
RUN_PT_TF_CROSS_TESTS=1 RUN_SLOW=1 pytest tests/test_modeling_speech_to_text.py
RUN_PT_TF_CROSS_TESTS=1 RUN_SLOW=1 pytest tests/test_modeling_tf_speech_to_text.py

EDIT: TF models uploaded.

patrickvonplaten · 2022-02-07T16:59:55Z

src/transformers/models/speech_to_text/modeling_speech_to_text.py

@@ -153,9 +153,9 @@ def make_weights(self, num_embeddings: int, embedding_dim: int, padding_idx: Opt
            # in forward put the weights on the correct dtype and device of the param
            emb_weights = emb_weights.to(dtype=self.weights.dtype, device=self.weights.device)

-        self.weights = nn.Parameter(emb_weights)
+        self.weights = emb_weights


@patil-suraj - is this change ok? Why exactly do we have to do this here actually?

I'm not sure we can just save a tensor like this in PyTorch. When doing model.to(device) does this tensor correctly move to the device as well?

I don't really understand why this would cause an issue in TF<=>PT exactly - what was the issue? BTW, we always wrap the static sinusoidal encodings in a nn.Parameter() or nn.Embedding(...), E.g. Vit's positional embeddings, DistilBERT's position embeddings. Why should we do it differently here?

But maybe it's totally fine. What do you think @LysandreJik @sgugger ?

what was the issue?

This is the only model that I could find that may change the size of the embedding layer in the forward call (here). The TF version for other embeddings allocates these named model parameters in build(), which is expected to happen before the first call(). Since nn.Parameter() is mostly a wrapper to add the variable to the model's list of parameters (AFAIK), then dropping it means we don't need a named parameter in TF, which means we can simplify the code and avoid solving the resizing issue.

Mmm, but dropping it also removes it from the checkpoint, no? This might prevent proper loading/saving AFAICT.

Facebook checkpoints do not have it and they load correctly in TF and PT regardless of this change. I'm assuming that, for the same reason, we exclude errors when loading a checkpoint in PT and this variable doesn't exist, as it is in _keys_to_ignore_on_load_missing (here).

The issue that prompted me to do this change was uncovered in the equivalence tests, where we try to load in TF a PT model that does not come from a checkpoint (here). We also have this var in _keys_to_ignore_on_save, but the test attempts to load a PT model in TF without saving it first -- i.e., having the var in nn.Parameter() breaks the test because it is not excluded, contrarily to the checkpoints.

Technically, someone who fine-tuned (or just saved and reshared) a Speech2Text model will have this weight in the checkpoint. I don't know how it's used exactly, so I don't know if it's breaking to ignore it (we would have to put it in the _keys_to_ignore_on_load_unexpected).

(sorted on Slack -- conclusion: reverting this change since it is needed for the to method)

gante · 2022-02-07T22:25:47Z

@patrickvonplaten reverted the previous change and added the embedding weights as named variables in TF

patrickvonplaten

Thanks for working on it!

* Add wrapper classes * convert inner layers to tf * Add TF Encoder and Decoder layers * TFSpeech2Text models * Loadable model * TF model with same outputs as PT model * test skeleton * correct tests and run the fixup * correct attention expansion * TFSpeech2Text pask_key_values with TF format

gante added the WIP Label your PR/Issue with WIP for some long outstanding Issues/PRs that are work in progress label Jan 13, 2022

gante force-pushed the add_tf_s2t branch from 1c8eca7 to ff8b2d3 Compare January 18, 2022 19:24

gante marked this pull request as ready for review January 28, 2022 20:18

gante added TensorFlow Anything TensorFlow and removed WIP Label your PR/Issue with WIP for some long outstanding Issues/PRs that are work in progress labels Jan 28, 2022

gante requested review from sgugger, Rocketknight1 and patil-suraj January 28, 2022 20:28

gante changed the title ~~Add TF Speech2Text~~ Add TFSpeech2Text Jan 28, 2022

sgugger approved these changes Jan 28, 2022

View reviewed changes

patrickvonplaten reviewed Jan 31, 2022

View reviewed changes

src/transformers/generation_tf_utils.py Outdated Show resolved Hide resolved

patrickvonplaten reviewed Jan 31, 2022

View reviewed changes

src/transformers/models/bart/modeling_tf_bart.py Show resolved Hide resolved

patrickvonplaten reviewed Jan 31, 2022

View reviewed changes

tests/test_modeling_tf_speech_to_text.py Outdated Show resolved Hide resolved

patrickvonplaten reviewed Jan 31, 2022

View reviewed changes

tests/test_modeling_tf_speech_to_text.py Show resolved Hide resolved

patrickvonplaten reviewed Jan 31, 2022

View reviewed changes

gante added 13 commits February 1, 2022 16:57

set the goal

09be826

Add wrapper classes

0ce166c

(tmp commit)

7e7db8b

convert inner layers to tf

2967e72

Add TF Encoder and Decoder layers

b0227b3

TFSpeech2Text models

de33fcb

Loadable model

3fddf4f

TF model with same outputs as PT model

a5eee1d

test skeleton

e52f07e

correct tests and run the fixup

6f39b0a

revert some lines that shouldn't have been touched

7071bf4

address PR comments

82dd2a2

run make fixup

2dc345f

patil-suraj and others added 10 commits February 1, 2022 16:57

skip test for XGLM (huggingface#15445)

485d271

[generate] fix synced_gpus default (huggingface#15446)

2ab8652

remove "inputs" in tf common test script (no longer required) (huggin…

de5e723

…gface#15262) Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>

Fix TF Causal LM models' returned logits (huggingface#15256)

49a257e

* Fix TF Causal LM models' returned logits * Fix expected shape in the tests Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>

fix from_vision_text_pretrained doc example (huggingface#15453)

f1ea43c

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>

[M2M100, XGLM] fix positional emb resize (huggingface#15444)

76ec1d0

Update README.md (huggingface#15462)

259a8a6

fix typo

delete commented line

7728c92

gante force-pushed the add_tf_s2t branch from da921ca to 7728c92 Compare February 1, 2022 16:57

gante added 2 commits February 1, 2022 17:10

revert previous messy commit

758761e

Merge remote-tracking branch 'origin/master' into add_tf_s2t

28c2eba

Rocketknight1 approved these changes Feb 1, 2022

View reviewed changes

Fix pipeline tests; correct input variable name

b278fe9

patrickvonplaten reviewed Feb 2, 2022

View reviewed changes

Fix dummy input sizes; Remove constant from nn.Parameters

442bd65

patrickvonplaten reviewed Feb 7, 2022

View reviewed changes

named embedding weights

992c7b9

run make fixup

48d56b2

patrickvonplaten approved these changes Feb 8, 2022

View reviewed changes

gante merged commit 8406fa6 into huggingface:master Feb 8, 2022

gante deleted the add_tf_s2t branch February 8, 2022 16:27

Add TFSpeech2Text #15113

Add TFSpeech2Text #15113

Conversation

gante commented Jan 11, 2022 • edited Loading

What does this PR do?

gante commented Jan 28, 2022

sgugger 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

gante Jan 31, 2022 • 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

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

gante Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickvonplaten left a comment

Choose a reason for hiding this comment

Rocketknight1 commented Feb 1, 2022

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

patrickvonplaten left a comment

Choose a reason for hiding this comment

gante commented Feb 4, 2022 • 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

Choose a reason for hiding this comment

gante Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

sgugger Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gante commented Feb 7, 2022

patrickvonplaten left a comment

Choose a reason for hiding this comment

gante commented Jan 11, 2022 •

edited

Loading

gante Jan 31, 2022 •

edited

Loading

gante Feb 1, 2022 •

edited

Loading

gante commented Feb 4, 2022 •

edited

Loading

gante Feb 7, 2022 •

edited

Loading

sgugger Feb 7, 2022 •

edited

Loading