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

Added whisper state + default state on the whisper_context #523

Merged

Conversation

sandrohanea
Copy link
Contributor

@sandrohanea sandrohanea commented Feb 21, 2023

In order to reuse memory and run multiple transformations on a single model, added the whisper_state (which is keeping the state of the current transformation).

In order to not be a breaking change and keep the usage of whisper_context to minimum, I added a default_state on the context and the current methods (e.g. whisper_full) will use that context in order to run the transformation.

Ofc, these methods are not thread safe, but additional methdos are created where the state can be externally provided (e.g. whisper_full_with_state )

For the callbacks, the state is also provided so bindings need to be updated for this, but the other methods are still valid.

@sandrohanea
Copy link
Contributor Author

Hey @ggerganov,
This PR is an alternative to #494 where the existing functionaliy is kept the same (by using a default_state) and additional capability (to run multiple transformations on the same context) is provided with newer functions.

Let me know what you think about this simpler approach.

If you'll like it, we can create some follow-up tasks to update emscripten + various samples to not create so many contexts anymore and save some memory. :)

Copy link
Owner

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

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

Nice - well done!

We need to update the callbacks in the examples.

If you'll like it, we can create some follow-up tasks to update emscripten + various samples to not create so many contexts anymore and save some memory. :)

Hmm, where do we create many contexts?

whisper.cpp Outdated Show resolved Hide resolved
whisper.cpp Outdated Show resolved Hide resolved
whisper.cpp Outdated Show resolved Hide resolved
whisper.cpp Outdated Show resolved Hide resolved
whisper.cpp Outdated Show resolved Hide resolved
whisper.cpp Outdated Show resolved Hide resolved
whisper.cpp Show resolved Hide resolved
whisper.cpp Outdated Show resolved Hide resolved
whisper.cpp Outdated Show resolved Hide resolved
whisper.h Outdated Show resolved Hide resolved
@sandrohanea
Copy link
Contributor Author

Nice - well done!

We need to update the callbacks in the examples.

If you'll like it, we can create some follow-up tasks to update emscripten + various samples to not create so many contexts anymore and save some memory. :)

Hmm, where do we create many contexts?

Here for example: https://github.com/ggerganov/whisper.cpp/blob/master/examples/stream.wasm/emscripten.cpp#L16

But there are other samples which are creating multiple contexts like that.

@ggerganov
Copy link
Owner

@sandrohanea
Ah, this looks like it creates multiple contexts, but in reality it is just one.
Initially I was thinking the WASM lib should support multiple contexts, so I created this global map. And then it propagated in various examples. But it is actually not needed. I should remove it and replace it with a single instance.

@ggerganov ggerganov merged commit 59fdcd1 into ggerganov:master Mar 5, 2023
@sandrohanea sandrohanea deleted the feature/introduced-whisper-state branch March 18, 2023 08:40
anandijain pushed a commit to anandijain/whisper.cpp that referenced this pull request Apr 28, 2023
…gerganov#523)

* Added whisper state + default state on the whisper_context

* Fixed some examples and bindings

* Fixed whisper_n_len (which was used in some binding) and added whisper_n_len_from_state

* Fixed comments

* whisper : reuse kv_cache_free() and fix compiler warnings

* whisper : clean-up the API comments

---------

Co-authored-by: Sandro Hanea <sandrohanea@microsoft.com>
Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
jacobwu-b pushed a commit to jacobwu-b/Transcriptify-by-whisper.cpp that referenced this pull request Oct 24, 2023
…gerganov#523)

* Added whisper state + default state on the whisper_context

* Fixed some examples and bindings

* Fixed whisper_n_len (which was used in some binding) and added whisper_n_len_from_state

* Fixed comments

* whisper : reuse kv_cache_free() and fix compiler warnings

* whisper : clean-up the API comments

---------

Co-authored-by: Sandro Hanea <sandrohanea@microsoft.com>
Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
jacobwu-b pushed a commit to jacobwu-b/Transcriptify-by-whisper.cpp that referenced this pull request Oct 24, 2023
…gerganov#523)

* Added whisper state + default state on the whisper_context

* Fixed some examples and bindings

* Fixed whisper_n_len (which was used in some binding) and added whisper_n_len_from_state

* Fixed comments

* whisper : reuse kv_cache_free() and fix compiler warnings

* whisper : clean-up the API comments

---------

Co-authored-by: Sandro Hanea <sandrohanea@microsoft.com>
Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
landtanin pushed a commit to landtanin/whisper.cpp that referenced this pull request Dec 16, 2023
…gerganov#523)

* Added whisper state + default state on the whisper_context

* Fixed some examples and bindings

* Fixed whisper_n_len (which was used in some binding) and added whisper_n_len_from_state

* Fixed comments

* whisper : reuse kv_cache_free() and fix compiler warnings

* whisper : clean-up the API comments

---------

Co-authored-by: Sandro Hanea <sandrohanea@microsoft.com>
Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
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.

None yet

2 participants