Add TF whisper #19378

amyeroberts · 2022-10-06T11:52:37Z

What does this PR do?

Adds TF Whisper port of PyTorch implementation

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline,
Pull Request section?
Was this discussed/approved via a Github issue or the forum? Please add a link
to it if that's the case.
Did you make sure to update the documentation with your changes? Here are the
documentation guidelines, and
here are tips on formatting docstrings.
Did you write any new necessary tests?

HuggingFaceDocBuilderDev · 2022-10-06T12:05:16Z

The documentation is not available anymore as the PR was closed or merged.

gante

🔥 🔥 🔥

I have a bunch of [XLA Notes] comments -- they don't need to be addressed now. I've wrote them down as potential sources of problems for XLA :D

src/transformers/generation_tf_logits_process.py

gante · 2022-10-06T11:55:38Z

src/transformers/generation_tf_logits_process.py

+                indices=[[i, token] for i in range(scores.shape[0]) for token in self.begin_suppress_tokens],
+                updates=[-float("inf") for _ in range(scores.shape[0] * len(self.begin_suppress_tokens))],


[XLA notes] I'm suspicious about these lines, list comprehensions at call time often cause problems.

indices: A more common pattern here is to build indices with TF functions like tf.tile and tf.concat.
updates: tf.ones_like((scores.shape[0] * len(self.begin_suppress_tokens)), dtype=tf.float32) * -float("inf") would work here :)

Very interesting!

FYI post XLA fixing: this wasn't a problem 👍

gante · 2022-10-06T11:56:25Z

src/transformers/generation_tf_logits_process.py

+            indices=[[i, token] for i in range(scores.shape[0]) for token in self.suppress_tokens],
+            updates=[-float("inf") for _ in range(scores.shape[0] * len(self.suppress_tokens))],


[XLA notes] same as above

FYI post XLA fixing: this wasn't a problem 👍

gante · 2022-10-06T11:56:43Z

src/transformers/generation_tf_logits_process.py

+    other tokens to `-inf` so that they are sampled at their corresponding index."""
+
+    def __init__(self, force_token_map):
+        self.force_token_map = dict(force_token_map)


[XLA notes] I'm also skeptical that dict .get() works with XLA. We might want to convert this to a flat tensor, with negative tokens in the empty positions.

I'm almost certain that anything like get() or a dictionary lookup will be done once when the function is traced, with any arguments treated as constants, rather than in each loop iteration with the arguments treated as variables.

FYI post XLA fixing: this was indeed a problem 😱

src/transformers/generation_tf_utils.py

tests/models/whisper/test_modeling_tf_whisper.py

ArthurZucker

LGTM other than maybe XLA and the shared embedding

ArthurZucker · 2022-10-06T13:19:33Z

src/transformers/generation_tf_logits_process.py

+                indices=[[i, token] for i in range(scores.shape[0]) for token in self.begin_suppress_tokens],
+                updates=[-float("inf") for _ in range(scores.shape[0] * len(self.begin_suppress_tokens))],


Very interesting!

src/transformers/models/whisper/modeling_tf_whisper.py

ArthurZucker · 2022-10-06T13:38:32Z

src/transformers/models/whisper/modeling_tf_whisper.py

+            return_dict=return_dict,
+        )
+        # Decoder and encoder embeddings are tied
+        lm_logits = tf.matmul(outputs[0], self.model.get_input_embeddings().weights, transpose_b=True)


Here would just remind my previous comment about TFSharedEmbedding that has a linear mode

It looks like the TFSharedEmbedding layer is flagged for being deleted cc @gante

I've tidied up the call a little bit though. Let me know what you think.

tests/models/whisper/test_modeling_tf_whisper.py

Rocketknight1

I focused on the core model code and comparing it to the PT implementation, since it seemed like @gante was handling generation and XLA compatibility. In the one place where I thought I'd found an error (missing padding in the Conv1Ds) that was being handled in the call(). I added a suggestion for a comment there, but other than that the core model code LGTM!

src/transformers/models/whisper/modeling_tf_whisper.py

gante

Correction to the forced ids logits processor test :)

tests/generation/test_generation_tf_logits_process.py

src/transformers/models/whisper/modeling_tf_whisper.py

sgugger

Thanks a lot for adding the port so quickly 💪

src/transformers/models/whisper/modeling_whisper.py

patrickvonplaten

Very nice!

patrickvonplaten

Super nice!

patrickvonplaten · 2022-10-10T09:33:26Z

src/transformers/models/whisper/modeling_tf_whisper.py

+    def decoder(self):
+        return self.model.decoder
+
+    def encoder(self):


not really relevant for this PR but why is there both a encoder and a get_encoder function?

Good question. Tbh, I was just copying this to match the PT model and didn't think about it. Looking at other models e.g. bart it seems to be a common pattern in the codebase.

src/transformers/models/whisper/modeling_tf_whisper.py

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

Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>

Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com>

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

…eration

amyeroberts requested review from Rocketknight1, gante, ArthurZucker and patrickvonplaten October 6, 2022 11:54

gante reviewed Oct 6, 2022

View reviewed changes

ArthurZucker reviewed Oct 6, 2022

View reviewed changes

Rocketknight1 approved these changes Oct 6, 2022

View reviewed changes

src/transformers/models/whisper/modeling_tf_whisper.py Outdated Show resolved Hide resolved

src/transformers/models/whisper/modeling_tf_whisper.py Outdated Show resolved Hide resolved

gante reviewed Oct 6, 2022

View reviewed changes

tests/generation/test_generation_tf_logits_process.py Outdated Show resolved Hide resolved

amyeroberts commented Oct 6, 2022

View reviewed changes

src/transformers/models/whisper/modeling_tf_whisper.py Outdated Show resolved Hide resolved

gante approved these changes Oct 7, 2022

View reviewed changes

sgugger approved these changes Oct 7, 2022

View reviewed changes

src/transformers/models/whisper/modeling_whisper.py Outdated Show resolved Hide resolved

patrickvonplaten approved these changes Oct 7, 2022

View reviewed changes

patrickvonplaten approved these changes Oct 10, 2022

View reviewed changes

amyeroberts force-pushed the add-tf-whisper-rebase branch from 5fa46be to f320909 Compare October 10, 2022 10:57

ArthurZucker added 15 commits October 10, 2022 13:22

simplify loop

261b3b5

add featur extractor

435effd

add model

da4e95d

start conversion

01a2874

add dropout

b06e6ed

initial commit of test files

0671707

copnversion for all models

cab8901

update processor for correct padding

ec83a22

update feature extraction

c146800

update integration test logits match

669fc79

fmnt: off for the logits

5e54293

on the fly mel bank

d3235f2

small nit

1dc1035

update test

387dd80

update tokenizer

d21b751

amyeroberts and others added 22 commits October 10, 2022 13:22

Update src/transformers/models/whisper/modeling_tf_whisper.py

ea21254

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

Remove changes to adjust_logits_during_generation function

ded5c07

Update src/transformers/models/whisper/modeling_tf_whisper.py

b078c5d

Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>

Tidy up imports that don't require TF

fb54e32

Update tests - skip and no more skip

7bfaa9d

Update tests/generation/test_generation_tf_logits_process.py

df852e6

Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com>

Update src/transformers/models/whisper/modeling_tf_whisper.py

7fb03dc

Update src/transformers/models/whisper/modeling_tf_whisper.py

a8b8f31

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

Add training flags

125fe2c

Add (skipped) XLA generation tests

d3d9fb9

Add embedding correctness test

c18592b

Add constant ids for generation tests

1dc0760

Make logits finding a bit tidier

7e67099

Remove unused args

e7e2615

xla generation enabled

3a876e2

Don't skip XLA tests anymore

0867561

Fix tests - add position ids to expected signature and update rag gen…

eb1107f

…eration

Undo method reorder

f6b01a6

Remove added whitespace

408259d

Remove copy-paste gradient checkopint ref

9db7004

Remove

fbe6366

Trigger CI - (issue with refs when pulling)

53e4627

amyeroberts force-pushed the add-tf-whisper-rebase branch from 46728e3 to 53e4627 Compare October 10, 2022 12:22

amyeroberts merged commit e3f028f into huggingface:main Oct 10, 2022

amyeroberts deleted the add-tf-whisper-rebase branch October 10, 2022 13:48

ArthurZucker mentioned this pull request Oct 11, 2022

Fix whisper for pipeline #19482

Merged

sanchit-gandhi mentioned this pull request Nov 7, 2022

[README] Add section on 🤗 Transformers openai/whisper#468

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TF whisper #19378

Add TF whisper #19378

amyeroberts commented Oct 6, 2022

HuggingFaceDocBuilderDev commented Oct 6, 2022 •

edited

Loading

gante left a comment •

edited

Loading

gante Oct 6, 2022

ArthurZucker Oct 6, 2022

gante Oct 6, 2022

gante Oct 6, 2022

gante Oct 6, 2022

gante Oct 6, 2022

Rocketknight1 Oct 6, 2022

gante Oct 6, 2022

ArthurZucker left a comment

ArthurZucker Oct 6, 2022

ArthurZucker Oct 6, 2022

amyeroberts Oct 6, 2022

Rocketknight1 left a comment

gante left a comment

sgugger left a comment

patrickvonplaten left a comment

patrickvonplaten left a comment

patrickvonplaten Oct 10, 2022

amyeroberts Oct 10, 2022

		indices=[[i, token] for i in range(scores.shape[0]) for token in self.begin_suppress_tokens],
		updates=[-float("inf") for _ in range(scores.shape[0] * len(self.begin_suppress_tokens))],

Add TF whisper #19378

Add TF whisper #19378

Conversation

amyeroberts commented Oct 6, 2022

What does this PR do?

Before submitting

HuggingFaceDocBuilderDev commented Oct 6, 2022 • edited Loading

gante left a comment • 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

ArthurZucker 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

Rocketknight1 left a comment

Choose a reason for hiding this comment

gante left a comment

Choose a reason for hiding this comment

sgugger left a comment

Choose a reason for hiding this comment

patrickvonplaten left a comment

Choose a reason for hiding this comment

patrickvonplaten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuggingFaceDocBuilderDev commented Oct 6, 2022 •

edited

Loading

gante left a comment •

edited

Loading