Fixes #68 - memory allocation in whisper_full_params #72
Conversation
+ update tests to run on mac with coreml We can't dynamically allocate memory for language in whisper_full_params because there is no way to free it when the params are destructed. To avoid the need we are converting language value to something in g_lang as it is statically allocated.
The `whisper_full_params` structure uses `const char*` to manage strings, with memory allocation handled externally in the codebase. However, when a string is passed from Python, we need to ensure its lifetime is tied to the `whisper_full_params` instance. Achieving this without introducing excessive boilerplate code can be challenging. The cleanest solution I’ve found is to create a derived class from `struct whisper_full_param` that stores copies of the strings as std::string members. This approach allows us to manage memory effectively, keeping the Python-provided strings alive for the duration that the parameters class is in use. An alternative approach was to manipulate the __dict__ attribute and retain the Python string on the Python side of the class. However, this method seemed too unconventional and could be fragile, as modifying or removing values from __dict__ might lead to segmentation faults.
|
Thanks, @PiotrCzapla, for drawing my attention to the memory leaks. I just reviewed your previous PR and this one. Something like this. if (self.language != nullptr) {
free((void*)self.language);
}
self.language = strdup(new_c);this way we will have a copy to the |
|
The previous PR was crashing during transcription. The language field is being set in whisper_full_with_state and then we were trying to free static address. I've addressed this by going thorough the language table for language. But I don't have a similar way of addressing this for initial_prompt and suppress_regex. The fields aren't currently modified during inference as far as I was able to check but we don't know if that won't change in the future. The current PR way more robust in this regard. I think it is worth it, there is quite a few changed lines but the boiler code is short only 30 lines long (the addition of the wrapper class), the rest are modification to expose the wrapper class to python instead of the original struct. Besides I wanted to find a way to do it safely without writing lots of code as an exercise :), in ruby bindings this is just a one-liner, so I hoped for some similar way in pybind11, but it isn't so easy unfortunately. |
|
Yes I see .. the language field is also initialized to "en" when using |
|
I’m learning a lot as well! :) The closure syntax is new to me, but if I’m interpreting it correctly, it looks like the string_wrapper is being created once along with the closure, which would mean it’s shared between instances of the class, right?
If that’s the case, then the following test case shouldn’t work:
def test_initial_prompt_param_twice(self):
a = pw.whisper_full_params()
a.initial_prompt = "A" + " test"
b = pw.whisper_full_params()
b.initial_prompt = "B" + " test" # Here, 'a' will likely have a dangling pointer to freed memory.
self.assertEqual("A test", a.initial_prompt)
self.assertEqual("B test", b.initial_prompt)
I can’t test it atm. But If I read it right then we can end up with all sort of unintended crashes when people start to allocate multiple model instances.
… On 13 Oct 2024, at 00:06, Abdeladim S. ***@***.***> wrote:
Yes I see .. the language field is also initialized to "en" when using whisper_full_default_params, calling free will raise an error.
I was thinking why not just use a shared_ptr instead ?!
Take a look at this branch <https://github.com/abdeladim-s/pywhispercpp/tree/fix_memeory_allocation>, I borrowed some of your test cases and tried to implement it (it was a good exercise indeed ;)), It seems to be working well in my tests.
Let me know what do you think ?
—
Reply to this email directly, view it on GitHub <#72 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACTBVEIRDDSS6ZWOVI4BP3Z3GMNRAVCNFSM6AAAAABPXOBGLSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBYG4YTENRWHE>.
You are receiving this because you were mentioned.
|
|
True, it's shared between instances .. Good catch :) In that case, I think we can just go with your approach, it's better. Thanks a lot for the contribution. |
|
You’re welcome! Thanks for creating the binding and exploring these ideas with me. I initially chose it because of the COREML instructions in the README, and it really paid off. My encoding time dropped from 1.1 seconds to 0.7 seconds on large-v3-turbo, which gives me almost instant dictation speeds (about a 2-second delay after dictating for 30 seconds). That’s a game changer, especially since Whisper is so much better than macOS’s built-in dictation, and it’s easily tripled my words per minute when using ChatGPT or Claude. |
|
'Glad you found the binding useful! :) If you encounter any other issues or have any suggestions for improvements, please don't hesitate to let me know. |
The
whisper_full_paramsstructure usesconst char*to manage strings, with memory allocation handled externally in the codebase. However, when a string is passed from Python, we need to ensure its lifetime is tied to thewhisper_full_paramsinstance. Achieving this without introducing excessive boilerplate code can be challenging.The cleanest solution I’ve found is to create a derived class from
struct whisper_full_paramthat stores copies of the strings as std::string members. This approach allows us to manage memory effectively, keeping the Python-provided strings alive for the duration that the parameters class is in use.’ve also exposed a new field, suppress_regex, which wasn’t previously available. I hope that’s okay. The code has been compiled against the latest version of whisper.cpp (commit ede1718f6d45aa3f7ad4a1e169dfbc9d51570c4e). However, I haven’t updated the version number in this repository.