Flat fix #3114
Flat fix #3114
Conversation
…er.cpp into bindings_flat
|
In CI Ruby don't like my -DBINDINGS_FLAT (which also creates a GGML_BINDINGS_FLAT) The whole point of the switch is that it makes the additions / changes transparent to other systems |
|
@peardox Hi, I sent a pull request to fix Ruby problem to your branch: WhisperBooks#1 |
Fix for Ruby options Thanks - I'm clueless regartding Ruby...
| @@ -0,0 +1,60 @@ | |||
| #include "whisper.h" | |||
There was a problem hiding this comment.
Could the number of includes be reduced here to only whisper-flat.h and ggml-backend.h perhaps?
There was a problem hiding this comment.
Ahh, yeah - probably overkill there :)
I just copied what was in whisper.cpp and added to it (lazy me, sorry)
| return whisper_get_system_info_json(); | ||
| } | ||
|
|
||
| struct whisper_state * whisper_flat_get_state_from_context(struct whisper_context * ctx) { |
There was a problem hiding this comment.
Should this really be exposed? My reason for asking is that whisper_state contains C++ constructs like std::vector and my understanding is that the purpose of this is avoid them as not all languages can deal with them in an easy way.
There was a problem hiding this comment.
Addressed in next reply - also whisper state is used in other functions - not just the ones I create (I'd have to look em up)
| #ifdef BINDINGS_FLAT | ||
| WHISPER_FLAT_API void whisper_flat_backend_load_all(void); | ||
| WHISPER_FLAT_API struct whisper_activity * whisper_flat_get_activity_with_state(struct whisper_state * state); | ||
| WHISPER_FLAT_API struct whisper_state * whisper_flat_get_state_from_context(struct whisper_context * ctx); |
There was a problem hiding this comment.
Same question as above, this should really be exposed?
There was a problem hiding this comment.
They exist precisely owing to the fact that c++ std:vector aren't available in other languages
Externally whisper_state can be treated as an opaque object as long as you have its address so whisper_flat_get_state_from_context can pluck it out of a whisper_context then plug it into whisper_flat_get_activity_with_state to get timing data directly rather than via JSON
| @@ -1309,14 +1309,19 @@ static size_t aheads_masks_nbytes(struct whisper_aheads_masks & aheads_masks) { | |||
| static ggml_backend_t whisper_backend_init_gpu(const whisper_context_params & params) { | |||
| ggml_log_set(g_state.log_callback, g_state.log_callback_user_data); | |||
|
|
|||
| #ifndef BINDINGS_FLAT | |||
There was a problem hiding this comment.
Nit: Macros are mostly placed at the beginning of the line in the code base, even if the code they guard is indented. This is done in a few other places in this PR as well.
There was a problem hiding this comment.
I specifically decided to stick them at the end to clearly delineate them from everything else
Also when reading code I tend to think of things up top as more important. Well, BINDINGS_FLAT is only important to a very small subset of Whisper devs - hence make it as unimportant as possible
| @@ -70,6 +70,10 @@ option(WHISPER_ALL_WARNINGS_3RD_PARTY "whisper: enable all compiler warnings in | |||
| option(WHISPER_FATAL_WARNINGS "whisper: enable -Werror flag" OFF) | |||
| option(WHISPER_USE_SYSTEM_GGML "whisper: use system-installed GGML library" OFF) | |||
|
|
|||
| # flat bindings | |||
| option(BINDINGS_FLAT "Add extra flat definitions to Whisper + GGML" OFF) | |||
There was a problem hiding this comment.
Perhaps this should be WHISPER_BINDINGS_FLAT to be consistent with the others?
There was a problem hiding this comment.
Yeah - I had that idea after deciding GGML_BINDINGS_FLAT was a good idea
There was a problem hiding this comment.
Be updating everything in a bit - in the middle of a huge all CUDA architecture build _ SYCL test (PCs going slow)
Fixingcommit conflicts