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

* simplify loop * add featur extractor * add model * start conversion * add dropout * initial commit of test files * copnversion for all models * update processor for correct padding * update feature extraction * update integration test logits match * fmnt: off for the logits * on the fly mel bank * small nit * update test * update tokenizer * nit feature extraction * update * update tokenizer test * adds logit processor and update tokenizer to get supress tokens * style * clean convert * revert to original modeling tf utils * Update * update * nit * clean convert file * update tests and nits * quality * slow generation test * ffn_dim to allow customization * update readme * add to toctreee * start fixing integration tests * update tests and code * fix feature extractor * fix config tests common * update code to fix tests * fix feature exctractor * nit feature extraction * update test for new feature extractor * style * add absrtact * large logits wioth custom decoder input ids * wraap around is otrch available * fix feature extractor * correct logits for whisper small.en * nit * fix encoder_attentino_mask * some fixes * remove unnecessary inputs * nits * add normalizer file * update etst tokenization * fix attention mask not defined * fix generate * remove uncoder attention mask useless * update test modeling whisper * update condfig to add second non supress tokens * nits on feature exrtactor * nit for test tokenizers * update etsts * update tests * update tokenization test * fixup * invalidated hf token. Clean convert openai to whisper * fix logit tests * fixup * Add model to README * Fix doc tests * clean merge * revert toc_tree changes * remove useless LogitProcessor * Update whisper .mdx * update config file doc * update configuration docstring * update test tokenization * update test tokenization * update tokenization whisper Added copied from where needed * update feature extraction * nit test name * style * quality * remove get suppress tokens and update non_speech tokens global variables * Update src/transformers/models/whisper/feature_extraction_whisper.py Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> * clean modeling whisper and test Removed the attention mask arguments that are deprecated * fix large test * Add multilingual audio test, and translate test * style * fix larg multilingual test * nits * add copied from for attention layer * remove attention masks in doc * add english normalizer * Update docs/source/en/model_doc/whisper.mdx Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> * update tokenization test * remove copied from in whisper attention : no bias in k_proj only * wrap around dependencies in english normalizer * style * correct import generation logits * for now, wrap feature extractor with torch * remove torch depencies for feature extraction and style * Update src/transformers/models/whisper/convert_openai_whisper_to_tfms.py Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com> * Update src/transformers/models/whisper/configuration_whisper.py Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com> * Update docs/source/en/model_doc/whisper.mdx Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com> * fixup * nit * update logitds * style * nit * nits and fix final tests * add `is_more_itertools_available` to utils * quality * add begin supress tokens, supress tokens to generate args and config * clean supressTokensLogitProcessor in generation logits * Nit naming * add supressTokensAtBegin * udpate tests, supress tokens to None or correct values * nit and style * update RAG to fit test and generate_logit * add copy pasted statment on english normalizer * add arguments to config_common_kwargs * Update src/transformers/generation_utils.py Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com> * Update src/transformers/generation_logits_process.py Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com> * revert changes based on reviews * update doc and nits * Update src/transformers/models/whisper/configuration_whisper.py Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com> * more nits * last nits * update test configuration common * add BART name in decoder attention mask documentation * Update src/transformers/models/whisper/modeling_whisper.py Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com> * style * nit * nit * add english.json file to git * nits on documentation * nit * nits * last styling * add main toctree file * remove sentence piece dependency * clean init file * fix tokenizer that has no dependencies on sentencepiece * update whisper init file, nit * remove english.json file * add get decoder prompt id * All weights loading * Remove hanging pdb * Fixup and tidy up * Use same copied from as PT model * Remove whitespace changes * Remove torch references * Tie embeddings * Remove logits processor input to generate * Update logit values * revert changes and add forced logit processor * nit * clean normalizer * remove protected * Add logit processors and update generation code & tests * Some tidy up * Update docstring * update * update based on review * Update src/transformers/models/whisper/configuration_whisper.py Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> * Update src/transformers/models/whisper/configuration_whisper.py Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> * Update to reflect changes on the PT model branch * Tidy up * Remove extra whitespace * Fix test - make input ids small enough we can append * Include upstream changes on main * PR comments - add batch tests, remove comments & defaults * Fix model output imports * Update src/transformers/models/whisper/modeling_tf_whisper.py Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com> * Update src/transformers/generation_tf_logits_process.py Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com> * Update src/transformers/models/whisper/modeling_tf_whisper.py Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com> * Update src/transformers/models/whisper/modeling_tf_whisper.py Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com> * Update tests/models/whisper/test_modeling_tf_whisper.py Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com> * Update src/transformers/models/whisper/modeling_tf_whisper.py Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com> * Update src/transformers/models/whisper/modeling_tf_whisper.py Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com> * Update docstring example * Update src/transformers/models/whisper/modeling_tf_whisper.py Co-authored-by: Matt <Rocketknight1@users.noreply.github.com> * Remove changes to adjust_logits_during_generation function * Update src/transformers/models/whisper/modeling_tf_whisper.py Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com> * Tidy up imports that don't require TF * Update tests - skip and no more skip * Update tests/generation/test_generation_tf_logits_process.py Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com> * Update src/transformers/models/whisper/modeling_tf_whisper.py * Update src/transformers/models/whisper/modeling_tf_whisper.py Co-authored-by: Matt <Rocketknight1@users.noreply.github.com> * Add training flags * Add (skipped) XLA generation tests * Add embedding correctness test * Add constant ids for generation tests * Make logits finding a bit tidier * Remove unused args * xla generation enabled * Don't skip XLA tests anymore * Fix tests - add position ids to expected signature and update rag generation * Undo method reorder * Remove added whitespace * Remove copy-paste gradient checkopint ref * Remove * Trigger CI - (issue with refs when pulling) Co-authored-by: Arthur Zucker <arthur.zucker@gmail.com> Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> Co-authored-by: NielsRogge <niels.rogge1@gmail.com> Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com> Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com> Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com> Co-authored-by: Matt <Rocketknight1@users.noreply.github.com> Co-authored-by: Joao Gante <joao@huggingface.co>

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

gante left a comment •

edited

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

gante left a comment • edited

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

gante left a comment •

edited