-
Notifications
You must be signed in to change notification settings - Fork 4.3k
bindings.ruby : fix test failures in test_whisper #2955
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
This commit updates the parallel tests to use 2 processors instead of the number of processors on the system. It also comments out the setting of the log callback to an empty lambda as this causes a segfault when enabled. The motivation for the change to the number of processors is that if one has a large number of processors, for example I have 16 on the machine I used to test this, this would cause the following warning to be printed: ```console whisper_full_with_state: input is too short - 680 ms < 1000 ms. consider padding the input audio with silence ``` This is logged from: ```c++ int whisper_full_with_state( struct whisper_context * ctx, struct whisper_state * state, struct whisper_full_params params, const float * samples, int n_samples) { ... if (seek_end < seek_start + 100) { WHISPER_LOG_WARN("%s: input is too short - %d ms < 1000 ms. consider padding the input audio with silence\n", __func__, (seek_end - seek_start)*10); return 0; } ``` This will return early and there will be segment callbacks to be invoked which in turn will cause the tests to fail.
This commit fixes the following warnings in the Ruby tests: ```console /whisper/bindings/ruby/tests/test_segment.rb:52: warning: ambiguity between regexp and two divisions: wrap regexp in parentheses or add a space after `/' operator ``` And also adds a '_' prefix to some unused variables to avoid warnings.
@danbev |
No problem, I'd be happy to take a look at this 👍 @KitaitiMakoto I've reverted the change now but I'm no longer getting segfaults when running the tests like I did earlier. today. One thing that changed is that I rebased my branch to include the latest ggml sync to master, to make sure things still worked. But with those latest changes in ggml I can't reproduce the segfaults and more. |
The commit reverts the commenting out of the Whisper.log_set call in the test_whisper.rb tests. I'm no longer getting segfaults when running the tests with this which was the case earlier. One theory could be that I rebased this to include the latest ggml sync to master to make sure things still worked. With the latest changes in ggml, I can't reproduce the segfaults.
@danbev @ggerganov |
@KitaitiMakoto Feel free to merge now or in the future. Just make sure to use "Squash and merge" mode. |
Okay, I will. |
This commit updates the parallel tests to use 2 processors instead of the number of processors on the system. It also comments out the setting of the log callback to an empty lambda as this causes a segfault when enabled.
The motivation for the change to the number of processors is that if one has a large number of processors, for example I have 16 on the machine I used to test this, this would cause the following warning to be printed:
whisper_full_with_state: input is too short - 680 ms < 1000 ms. consider padding the input audio with silence
This is logged from:
This will return early and there will be no segment callbacks to be invoked which in turn will cause the tests to fail.