-
Notifications
You must be signed in to change notification settings - Fork 285
Add C API for WhisperPipeline #2414
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
Conversation
samples/c/whisper_speech_recognition/whisper_speech_recognition.c
Outdated
Show resolved
Hide resolved
OPENVINO_GENAI_C_EXPORTS ov_status_e ov_genai_generation_config_set_num_beam_groups(ov_genai_generation_config* config, | ||
const size_t value); | ||
OPENVINO_GENAI_C_EXPORTS ov_status_e ov_genai_generation_config_set_num_beam_group(ov_genai_generation_config* config, | ||
const size_t value); | ||
|
||
/** | ||
* @brief Set the number of beams for beam search. 1 disables beam search. | ||
* @param handle A pointer to the ov_genai_generation_config instance. | ||
* @param value The number of beams for beam search. | ||
* @return ov_status_e A status code, return OK(0) if successful. | ||
*/ | ||
OPENVINO_GENAI_C_EXPORTS ov_status_e ov_genai_generation_config_set_num_beam_groups(ov_genai_generation_config* config, | ||
const size_t value); | ||
OPENVINO_GENAI_C_EXPORTS ov_status_e ov_genai_generation_config_set_num_beams(ov_genai_generation_config* config, | ||
const size_t value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it changed? Our API should be backward-compatible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused here - the previous signatures were exactly the same but the comments mention different functionality. Perhaps only the second one should change
options->sample_rate = DEFAULT_SAMPLE_RATE; | ||
|
||
for (int i = 1; i < argc; i++) { | ||
if (strcmp(argv[i], "-m") == 0 || strcmp(argv[i], "--model") == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
С++ has simpler cmd interface inviting readers to modify the implementation themselves. Is there a reason to deviate from that? Of not, align them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, initially I also had benchmarking in this file so arguments was based on the benchmark_genai_c file .
But the performance benchmarks was removed from here so no point of the complicated arguments anymore . Let me remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed: #2414 (comment)
samples/c/whisper_speech_recognition/whisper_speech_recognition.c
Outdated
Show resolved
Hide resolved
Simplified the arguments. Thanks for the review @Wovchena - ready for another go 🙏🏻 [ 98%] Built target openvino_genai_c
[100%] Building C object samples/c/whisper_speech_recognition/CMakeFiles/whisper_speech_recognition_c.dir/whisper_speech_recognition.c.o
[100%] Building C object samples/c/whisper_speech_recognition/CMakeFiles/whisper_speech_recognition_c.dir/whisper_utils.c.o
[100%] Linking C executable whisper_speech_recognition_c
[100%] Built target whisper_speech_recognition_c
> ./samples_bin/whisper_speech_recognition_c 'ov_cache0/test_models/WhisperTiny/openai/whisper-tiny' 'ov_cache0/test_data/how_are_you_doing_today.wav'
How are you doing today?
timestamps: [0.00, 2.00] text: How are you doing today?
> export SAMPLES_CPP_DIR="$(pwd)/samples_bin" && export SAMPLES_C_DIR="$(pwd)/samples_bin" && source .venv/bin/activate && python -m pytest
tests/python_tests/samples/test_whisper_speech_recognition.py -v -m whisper
========================================================================================= test session starts =========================================================================================
platform linux -- Python 3.12.3, pytest-8.4.1, pluggy-1.6.0 -- /home/brandon/openvino.genai/.venv/bin/python
cachedir: .pytest_cache
metadata: {'Python': '3.12.3', 'Platform': 'Linux-5.15.167.4-microsoft-standard-WSL2-x86_64-with-glibc2.39', 'Packages': {'pytest': '8.4.1', 'pluggy': '1.6.0'}, 'Plugins': {'langsmith': '0.4.4', 'html': '4.1.1', 'anyio': '4.9.0', 'metadata': '3.1.1'}}
rootdir: /home/brandon/openvino.genai/tests/python_tests
configfile: pytest.ini
plugins: langsmith-0.4.4, html-4.1.1, anyio-4.9.0, metadata-3.1.1
collected 1 item
tests/python_tests/samples/test_whisper_speech_recognition.py::TestWhisperSpeechRecognition::test_sample_whisper_speech_recognition[download_test_content=how_are_you_doing_today.wav-convert_model=WhisperTiny] PASSED [100%]
========================================================================================== warnings summary ===========================================================================================
../../../usr/lib/python3.12/multiprocessing/popen_fork.py:66
/usr/lib/python3.12/multiprocessing/popen_fork.py:66: DeprecationWarning: This process (pid=114380) is multi-threaded, use of fork() may lead to deadlocks in the child.
self.pid = os.fork()
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
==================================================================================== 1 passed, 1 warning in 2.36s ===================================================================================== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add README.md (I missed that earlier. I hope this is going to be the last change request)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
Added README (based off the C++ whisper and the C LLM Pipeline one) :
55a36e9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! There're CI problems, so this PR will hang unmerged for some time similar to others. Is that OK for you?
Woo! Thanks for all the reviews. Should be fine, I have what I need to continue .NET wrapper for whisper |
build_jenkins |
Strangely I don't have permission to update your branch. Can you merge master yourself? |
Weird, I wonder if it’s because it’s my fork. updated |
build_jenkins |
@Wovchena it might be easiest if I just give you write access to our fork. Seems like the timing matters quite a bit here :'( ![]() |
build_jenkins |
Following the LLM pipeline PR: 5636312#diff-38c2966d1144b82978ca2cfe7650879fb15bd6da5845a3c616e8d152b29317f1
ps: would be great to get access to NPU from WSL2 - its sooo much easier to debug compared to Powershell
intel/linux-npu-driver#56