Add whisper pipeline - Initial commit #789
Conversation
src/cpp/include/openvino/genai/whisper_speech_recognition_pipeline.hpp
Outdated
Show resolved
Hide resolved
src/cpp/include/openvino/genai/whisper_speech_recognition_pipeline.hpp
Outdated
Show resolved
Hide resolved
| throw std::runtime_error("Failed to read WAV file " + wav_file_path); | ||
| } | ||
|
|
||
| ov::genai::WhisperSpeechRecognitionPipeline pipeline{model_path}; |
There was a problem hiding this comment.
We checked with @yatarkan integrated GPU on his machine, it works fine. Trying to check on discrete one.
There was a problem hiding this comment.
Let's check separately
samples/cpp/whisper_speech_recognition/whisper_speech_recognition.cpp
Outdated
Show resolved
Hide resolved
samples/cpp/whisper_speech_recognition/whisper_speech_recognition.cpp
Outdated
Show resolved
Hide resolved
src/cpp/include/openvino/genai/whisper_speech_recognition_pipeline.hpp
Outdated
Show resolved
Hide resolved
andrei-kochin
changed the title
| m_impl->m_generation_config.validate(); | ||
| } | ||
|
|
||
| ov::genai::WhisperPipeline::~WhisperPipeline() = default; |
There was a problem hiding this comment.
do we need it at all? I think you can omit dtor definition
There was a problem hiding this comment.
I've tried to remove it but I got compilation error. As I understood use of std::unique_ptr<Impl> class member requires class to have a destructor: https://stackoverflow.com/questions/34072862/why-is-error-invalid-application-of-sizeof-to-an-incomplete-type-using-uniqu
I guess there is a way to implement it without need of WhisperPipeline explicit destructor, please let me know if I need to investigate.
Error:
[build] /usr/include/c++/11/bits/unique_ptr.h: In instantiation of ‘void std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = ov::genai::WhisperPipeline::Impl]’:
[build] /usr/include/c++/11/bits/unique_ptr.h:361:17: required from ‘std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = ov::genai::WhisperPipeline::Impl; _Dp = std::default_delete<ov::genai::WhisperPipeline::Impl>]’
[build] /opt/home/suvorova/projects/openvino/openvino.genai/src/cpp/include/openvino/genai/whisper_pipeline.hpp:20:30: required from hereThere was a problem hiding this comment.
maybe we can use shared_ptr then?
| @@ -11,7 +11,10 @@ | |||
| #include <openvino/runtime/auto/properties.hpp> | |||
| #include "../cpp/src/tokenizers_path.hpp" | |||
|
|
|||
| #include "./utils.hpp" | |||
There was a problem hiding this comment.
| #include "./utils.hpp" | |
| #include "utils.hpp" |
just add target_include dirs to current folder in cmake
There was a problem hiding this comment.
Naively adding target_include_dirs didn't work. It compiles successfully, but python code couldn't find symbol in utils.cpp. I'll add todo to address that.
pybind11_add_module(py_generate_pipeline py_generate_pipeline.cpp)
target_include_directories(py_generate_pipeline PUBLIC "${CMAKE_CURRENT_SOURCE_DIR}/")
| run: | | ||
| . "${{ env.OV_INSTALL_DIR }}/setupvars.ps1" | ||
| python -m pip install . --verbose | ||
| python -m pytest ./tests/python_tests/test_whisper_generate_api.py |
There was a problem hiding this comment.
can we test only via either wheel or C++ archive?
There was a problem hiding this comment.
I followed existing pattern here. I guess we can run just some single test to ensure both (wheel and C++ archive) approaches work and not the whole test suite. @Wovchena what's your thoughts on this?
This is work in progress PR. Todos:
assets/whisper/mel_filters_data.binon initializationdr_wav.hwithFetchContentov::genai::TokenizerRemoteTensorfor GPUCurrent limitations:
Tickets: CVS-147994, CVS-146010, CVS-152522