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

AutomaticSpeechRecognitionPipeline callback_function api inconsistancy #43

Closed
j0rd1smit opened this issue Mar 21, 2023 · 6 comments
Closed

Comments

@j0rd1smit
Copy link

In the AutomaticSpeechRecognitionPipeline class the callback_function is used in two places:

  1. To return the chucks
  2. In the model generation

As a result, 1 returns a different data format than 2. This wasn't very clear to me. Does it not make sense to have a separate callback for these?

@xenova
Copy link
Owner

xenova commented Mar 21, 2023

Hi. Yes, you are correct, I originally implemented it with a TODO comment somewhere there to return a type parameter, which will help users differentiate between what is being run in the callback function.

For the most part, this is because HF's transformer library doesn't have the same functionality (probably because it would cause some inconsistencies), so, we had to make it ourselves (something which is very useful for streaming the output back).

I suppose we could change it to something like chunk_callback? Any thoughts/suggestions?

@j0rd1smit
Copy link
Author

Yeah, I really like the stream part, so keep it. I would keep calling the streaming callback callback_function to stay consistent. chunk_callback makes sense here since it is only used in the chuck callback.

Personally, my use cases are as follows:

  1. callback_function: for visualization of the newly generated tokens. This give the user feedback that the results are being generated.
  2. chunk_callback: to get the entire result for the chunk + plus the timestamps.

callback_function would then render the initial results, while chunk_callback obtains the final result and adds the timestamps. (I'm not sure if this directly works since you need all the chucks so far for timestamps if you use pipeline.tokenizer._decode_asr(chunks, ...). I think the chunk_callback only receives the newly generated chucks. However, this is fixable by keeping track of all the states yourself in the callback).

@j0rd1smit
Copy link
Author

(BTW I love how fast you are responding to issues here kudos 😀)

@xenova
Copy link
Owner

xenova commented Mar 21, 2023

(BTW I love how fast you are responding to issues here kudos 😀)

Haha yeah I'm trying my best to get all these things fixed! 😄 I'm online 24/7 ;)

Yeah, I really like the stream part, so keep it. I would keep calling the streaming callback callback_function to stay consistent. chunk_callback makes sense here since it is only used in the chuck callback.

Okay that sounds do-able. This functionality (streaming while doing merging) isn't available in the python implementation, so, there aren't any "rules" to follow per-se (other than it should make sense haha).

callback_function would then render the initial results, while chunk_callback obtains the final result and adds the timestamps. (I'm not sure if this directly works since you need all the chucks so far for timestamps if you use pipeline.tokenizer._decode_asr(chunks, ...). I think the chunk_callback only receives the newly generated chucks. However, this is fixable by keeping track of all the states yourself in the callback).

Technically, it is possible to get the timestamps while you are generating, using a combination of the callback_function and pipeline.tokenizer._decode_asr(chunks, ...), with force_full_sequences=false. This would entail keeping track of all the output tokens as you generate, and you can detect a "new chunk" if the output token you get is the <|startoftranscript|> token. Then, you can perform the merging as you generate new tokens. The force_full_sequences=false is necessary, otherwise it will through an error because the whole transcript isn't ready yet.

I can try provide an example (later possibly), but I am currently working on fixing some other bugs (see other issues).

@xenova
Copy link
Owner

xenova commented Mar 21, 2023

Updated in 6c9ea41 👍

will close the issue when I make the 1.3.2 release.

Let me know if you have any other questions or suggestions :)

@xenova
Copy link
Owner

xenova commented Mar 22, 2023

Update is now live! (https://www.npmjs.com/package/@xenova/transformers/v/1.3.2)

Closing the issue now :)

@xenova xenova closed this as completed Mar 22, 2023
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

No branches or pull requests

2 participants