Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some TFWhisperModelIntegrationTests #24428

Merged
merged 14 commits into from
Jun 23, 2023
Merged

Fix some TFWhisperModelIntegrationTests #24428

merged 14 commits into from
Jun 23, 2023

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Jun 22, 2023

What does this PR do?

(Probably since the introduction of GenerationConfig), some TF Whisper Integration tests fail with error

ValueError: The following `model_kwargs` are not used by the model: ['language', 'task'] (note: typos in the generate arguments will also show up in this list)

From my understanding, we should pass some arguments via generation_kwargs.

Note that WhisperForConditionalGeneartion has its custom generate but TFWhisperForConditionalGeneartion doesn't.

⚠️ When I try to apply the same changes to PyTorch Whisper test methods, it fails as the output is different.

We are somehow in a trouble of the inconsistency between PT/TF.

(Not sure if we should overwrite generate in TFWhisperForConditionalGeneartion.)

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing!

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 22, 2023

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

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM- thanks for fixing!

Regarding the PyTorch tests failing with the same inputs - how do they fail?

@ydshieh
Copy link
Collaborator Author

ydshieh commented Jun 22, 2023

LGTM- thanks for fixing!

Regarding the PyTorch tests failing with the same inputs - how do they fail?

It gives different outputs

@ydshieh
Copy link
Collaborator Author

ydshieh commented Jun 22, 2023

I am very sorry, I was doing something stupid and made wrong claims. The generation_kwargs is an argument to the __init__ of GenerationConfig instead of the generate method.

(Some of) The previous failing tests pass as the 2 problematic arguments to generate are not passed, but this is wrong logic.

I will look in more depth.

@ydshieh ydshieh marked this pull request as draft June 22, 2023 22:05
@ydshieh ydshieh force-pushed the fix_tf_whisper_tests branch from 7b28857 to f968f55 Compare June 23, 2023 05:28
@ydshieh
Copy link
Collaborator Author

ydshieh commented Jun 23, 2023

Well, I finally decided to overwrite generate for TFWhisperForConditionalGeneartion.

@ydshieh ydshieh requested a review from amyeroberts June 23, 2023 06:48
@ydshieh ydshieh marked this pull request as ready for review June 23, 2023 08:57
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Needs testing for prompt_ids otherwise lgtm! Also we don't need part of the legacy things for timestamps since it was never supported in TF (just in case)

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the generate logic and fixing this!

+1 on @ArthurZucker's comment about adding the tests

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
@ydshieh ydshieh merged commit 2898fd3 into main Jun 23, 2023
@ydshieh ydshieh deleted the fix_tf_whisper_tests branch June 23, 2023 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants