Skip to content

Added a bool fold_lowercase to whisper_context_params#2005

Open
ulatekh wants to merge 5 commits intoggml-org:masterfrom
ulatekh:fold-lowercase
Open

Added a bool fold_lowercase to whisper_context_params#2005
ulatekh wants to merge 5 commits intoggml-org:masterfrom
ulatekh:fold-lowercase

Conversation

@ulatekh
Copy link
Copy Markdown
Contributor

@ulatekh ulatekh commented Mar 27, 2024

If true, it folds language-model tokens to lowercase. By default, it's false.
This is intended to make grammar matching more predictable, e.g. no need to account for case in the grammar.

ulatekh added 2 commits March 27, 2024 14:15
If true, it folds language-model tokens to lowercase.
By default, it's false.
This is intended to make grammar matching more predictable,
e.g. no need to account for case in the grammar.
bool print_energy = false;
bool no_timestamps = true;
bool use_gpu = true;
bool model_fold_lc = false;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change name to vocab_lc

Comment thread whisper.h Outdated
Comment thread whisper.cpp
Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
Comment thread examples/main/main.cpp
Comment on lines 209 to +210
fprintf(stderr, " -nt, --no-timestamps [%-7s] do not print timestamps\n", params.no_timestamps ? "true" : "false");
fprintf(stderr, " --model-fold-lc [%-7s] fold all model tokens to lowercase\n", params.model_fold_lc ? "true" : "false");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fprintf(stderr, " -nt, --no-timestamps [%-7s] do not print timestamps\n", params.no_timestamps ? "true" : "false");
fprintf(stderr, " --model-fold-lc [%-7s] fold all model tokens to lowercase\n", params.model_fold_lc ? "true" : "false");
fprintf(stderr, " -nt, --no-timestamps [%-7s] do not print timestamps\n", params.no_timestamps ? "true" : "false");
fprintf(stderr, " --vocab-lc [%-7s] fold all vocab tokens to lowercase\n", params.vocab_lc ? "true" : "false");

@ulatekh
Copy link
Copy Markdown
Contributor Author

ulatekh commented Apr 11, 2024

I have no idea what's wrong with the Java bindings. I loaded them all into Visual Studio Code and fixed all the errors it reported (which didn't seem related to my changes), but still the Java-related tests fail. FYI, I haven't programmed in Java in over 10 years.

@ggerganov
Copy link
Copy Markdown
Member

I'm also not good with Java, but I think we are probably observing an issue similar to this one: ggml-org/llama.cpp#1902 (comment)

In short, even though the two structs whisper_context_params (C) and WhisperContextParams (Java) have the same members, there are likely different paddings between the members which is causing UB in the following code:

https://github.com/ggerganov/whisper.cpp/blob/8f253ef3af1c62c04316ba4afa7145fc4d701a8c/bindings/java/src/main/java/io/github/ggerganov/whispercpp/WhisperCpp.java#L70-L81

The proper solution is to order the members in decreasing size (i.e. keep the bools at the end of the struct). Or maybe avoid bool and simply replace them with int - this seems much easier change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants