[WHISPER] Update modeling tests#20162
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
ydshieh
left a comment
There was a problem hiding this comment.
Thank you @ArthurZucker . LGTM, but let @sanchit-gandhi have a review too, as he is better whisper than me!
ydshieh
left a comment
There was a problem hiding this comment.
Sorry, I have a question now 🙏 !
sanchit-gandhi
left a comment
There was a problem hiding this comment.
Thanks for the fix @ArthurZucker! The slow tokenizer integration test looks good - have some comments regarding the logits test (see below).
|
I agree, it is indeed better to just use |
Co-authored-by: Sanchit Gandhi <93869735+sanchit-gandhi@users.noreply.github.com>
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
|
Ping me again once when you think it's ready :) |
|
Looks like we still need to add |
|
Yeah on it 🤗 |
Co-authored-by: Sanchit Gandhi <93869735+sanchit-gandhi@users.noreply.github.com> Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
|
Ah there's small bug with this, the kwargs is passed to |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
sanchit-gandhi
left a comment
There was a problem hiding this comment.
Thanks for iterating @ArthurZucker! Just one question about the removal of the kwargs in the feature extractor call method (see below).
| max_length=max_length if max_length else self.n_samples, | ||
| truncation=truncation, | ||
| pad_to_multiple_of=pad_to_multiple_of, | ||
| **kwargs, |
There was a problem hiding this comment.
I don't see the kwargs being used at all now. If we don't use the kwargs we can remove them from the call in L288:
Are we certain the kwargs were erroneous here?
There was a problem hiding this comment.
Yes, the add_special_tokens was passed down until here. Checked that wav2vec2 does not use kwargs for padding, plus the call is similar and kwargs are not popped out, so appears as the good solution.
There was a problem hiding this comment.
We might have to keep them for pipeline compatibility, will check that thanks 👍🏻
There was a problem hiding this comment.
Okay brill - sounds good in that case (pop from here, not in L228).
ydshieh
left a comment
There was a problem hiding this comment.
LGTM now! Thank you for your patience. Will leave @sanchit-gandhi and you to final the PR regarding the kwargs
…to fix-whisper-tok-update
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
* Update modeling tests * update tokenization test * typo * nit * fix expected attention outputs * Apply suggestions from code review Co-authored-by: Sanchit Gandhi <93869735+sanchit-gandhi@users.noreply.github.com> * Update tests from review Co-authored-by: Sanchit Gandhi <93869735+sanchit-gandhi@users.noreply.github.com> Co-authored-by: ydshieh <ydshieh@users.noreply.github.com> * remove problematics kwargs passed to the padding function Co-authored-by: Sanchit Gandhi <93869735+sanchit-gandhi@users.noreply.github.com> Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
What does this PR do?
Fixes the test after the update of the tokenizer, which added sufix and prefix tokens.