Word-level timestamps broken for short-form audio #30325
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
Hi @sanchit-gandhi, I've added slow tests that pass. With this PR we should be able to compute word-level timestamps for short form audio using the I've noticed that word-level timestamps cannot be computed using |
There was a problem hiding this comment.
LGTM - thanks for fixing @kamilakesbi! Let's indeed do the model + processor API in a follow-up PR
|
Hey @amyeroberts! would appreciate a final review here when you have time. |
| if isinstance(segment_size, int): | ||
| generate_kwargs["num_frames"] = segment_size // self.feature_extractor.hop_length | ||
| else: | ||
| generate_kwargs["num_frames"] = segment_size[0] // self.feature_extractor.hop_length |
There was a problem hiding this comment.
Under what cases would segment_size be a iterable? And will all the values of segment_size in the iterable be the same?
There was a problem hiding this comment.
segment_size would be an iterable when using the batch_size argument in the pipeline (with bs >1), as in here: in this case, we get an iterable of size 1. If I increase the batch size, I still get an iterable with size 1.
| @@ -459,6 +460,7 @@ def preprocess(self, inputs, chunk_length_s=0, stride_length_s=None): | |||
| def _forward(self, model_inputs, return_timestamps=False, **generate_kwargs): | |||
| attention_mask = model_inputs.pop("attention_mask", None) | |||
| stride = model_inputs.pop("stride", None) | |||
| segment_size = model_inputs.pop("segment_size", None) | |||
There was a problem hiding this comment.
Are there any cases when stride is None and we want to use segment_size?
Because we're popping both from model_inputs here, technically, the user can pass this in directly as an argument when calling pipeline.forward. In which case, it would be good to have input validation on these argument
There was a problem hiding this comment.
We will get stride is None when using this pipeline:
pipe = pipeline(
task="automatic-speech-recognition",
model="openai/whisper-large-v3",
return_timestamps="word",
)
--> segment_size is the alternative to compute and add num_frames to the generate_kwargs to be passed to the generate method here.
There was a problem hiding this comment.
Sorry, I mistyped. I meant to say when stride is not None
There was a problem hiding this comment.
We want to use segment_size only when stride is None.
There was a problem hiding this comment.
OK. Let's add a quick input validation so the argument isn't just silently ignored then if stride is not None
There was a problem hiding this comment.
I've added the following lines:
if stride is not None and segment_size is not None:
raise ValueError("segment_size must be used only when stride is None")
| ], | ||
| } | ||
|
|
||
| # batch size 1: copy the audio sample since pipeline consumes it |
There was a problem hiding this comment.
👀 - as in, mutates it? It shouldn't do that....
There was a problem hiding this comment.
Not sure to know why it does that, I've simply copied the logic from another test (this one) here!
There was a problem hiding this comment.
OK, let's open an issue to address this, so the work isn't forgotten but can be done in an separate PR. We should definitely avoid side-effect like this 😬
There was a problem hiding this comment.
Ok, I'll open an issue!
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
kamilakesbi
changed the title
…kamilakesbi/transformers into whisper_large_fix_word_level_timestamps
There was a problem hiding this comment.
Thanks for fixing and iterating on this!
@kamilakesbi Do you have permissions to merge in the PR? If not, let me know and I can merge it it for you
|
Hi @amyeroberts, I don't! Could you merge it ? |
What does this PR do?
This PR aims at fixing issue #30224: word-level timestamps is currently broken in Whisper large/large-v2/large-v3/distil-large-v3. The problem comes from the fact that
num_framesisn't passed to the generate method when stride is None.I suggest simply adding a new extra argument to the output of the
preprocessmethod, calledsegment_size, which will store the input length in the case of short form audios. It will be passed to the_forwardmethod and used when stride is None to computegenerate_kwargs["num_frames"] = segment_size // self.feature_extractor.hop_length.This seems to fix the problem. Is there any particular test that should be run regarding this issue?
Who can review?
@sanchit-gandhi