whisper.cpp
whisper : replace `tensor->n_dims` with `ggml_n_dims(tensor)`
#1694
Merged

whisper : replace `tensor->n_dims` with `ggml_n_dims(tensor)` #1694

bobqianic
bobqianic1 year ago (edited 1 year ago)

Since tensor->n_dims is now deprecated, we've switched to using ggml_n_dims as its replacement.

Even though slaren mentioned that ggml_n_dims isn't a direct substitute for tensor::n_dims, it's not a concern in this case. This is because both ne[0] and ne[1] should exceed 1 for tensor mel and out.

Close #1690

bobqianic Update whisper-openvino-encoder.cpp
92a817fb
slaren
slaren1 year ago👍 1

I don't know if it is an issue here, but ggml_n_dims is not a drop-in replacement for tensor::n_dims because it counts the number of non-1 dimensions. So for example, a tensor created with ggml_new_tensor_2d(.., 10, 1); would have n_dims = 2, but ggml_n_dims would return 1. If this is a potential issue in this case, you can instead check for ggml_n_dims(tensor) <= 2, or the equivalent ggml_is_matrix.

bobqianic
bobqianic1 year ago

So for example, a tensor created with ggml_new_tensor_2d(.., 10, 1); would have n_dims = 2, but ggml_n_dims would return 1.

Ah, I see. But then, why did we deprecate n_dims in ggml_tensor? It seems a bit illogical to me.

https://github.com/ggerganov/whisper.cpp/blob/37a709f6558c6d9783199e2b8cbb136e1c41d346/ggml.h#L506-L544

bobqianic
bobqianic1 year ago

Please feel free to correct me if I'm mistaken. I believe a good strategy to avoid this issue is to sanitize the input during the creation of a ggml_tensor. For instance, when using ggml_new_tensor_2d, it's important to ensure that both ne[0] and ne[1] are greater than 1.

slaren
slaren1 year ago👍 1

In practice, n_dims was not very useful, and not needing to propagate the value in operations simplifies the code a bit. For this kind of checks , ggml_n_dims() with <= should achieve the same result. Forcing dimensions to be greater than 1 in the ggml_new_tensor_nd would make a lot of code more complex for no reason, applications would need to check the value of the higher dimensions and use a different function in each case. A dimension value of 1 is not an error.

Abdalnablse10
Abdalnablse101 year ago

I tried the fix, it was successfully built, but got this error when trying to test it.
abdalnablse10@x86-1:~/TestFix/whisper.cpp$ ./build/bin/main -m models/ggml-base.en-encoder-openvino.bin -f samples/jfk.wav
whisper_init_from_file_with_params_no_state: loading model from 'models/ggml-base.en-encoder-openvino.bin'
whisper_model_load: loading model
whisper_model_load: invalid model data (bad magic)
whisper_init_with_params_no_state: failed to load model
error: failed to initialize whisper context

Converting the model went like the following.
(openvino_conv_env) abdalnablse10@x86-1:~/TestFix/whisper.cpp/models$ python3 convert-whisper-to-openvino.py --model base.en
/home/abdalnablse10/whisper.cpp/models/openvino_conv_env/lib/python3.11/site-packages/whisper/model.py:166: TracerWarning: Converting a tensor to a Python boolean might cause the trace to be incorrect. We can't record the data flow of Python values, so this value will be treated as a constant in the future. This means that the trace might not generalize to other inputs!
assert x.shape[1:] == self.positional_embedding.shape, "incorrect audio shape"

and generated two files ggml-base.en-encoder-openvino.bin and ggml-base.en-encoder-openvino.xml

bobqianic
bobqianic1 year ago

abdalnablse10@x86-1:~/TestFix/whisper.cpp$ ./build/bin/main -m models/ggml-base.en-encoder-openvino.bin -f samples/jfk.wav
whisper_init_from_file_with_params_no_state: loading model from 'models/ggml-base.en-encoder-openvino.bin'
whisper_model_load: loading model
whisper_model_load: invalid model data (bad magic)
whisper_init_with_params_no_state: failed to load model
error: failed to initialize whisper context

You only need to provide the path of the standard model to the main. Ensure that both the standard model and the OPENVINO encoder model have matching names and are located in the same directory. For instance, if the standard model is named ABCD.bin, then the corresponding OPENVINO model should be named ABCD-encoder-openvino.bin

/home/abdalnablse10/whisper.cpp/models/openvino_conv_env/lib/python3.11/site-packages/whisper/model.py:166: TracerWarning: Converting a tensor to a Python boolean might cause the trace to be incorrect. We can't record the data flow of Python values, so this value will be treated as a constant in the future. This means that the trace might not generalize to other inputs!
assert x.shape[1:] == self.positional_embedding.shape, "incorrect audio shape"

This is just a warning. Ignore it.

Abdalnablse10
Abdalnablse101 year ago

./build/bin/main -m ./models/ggml-base.en.bin -f ./samples/jfk.wav
whisper_init_from_file_with_params_no_state: loading model from './models/ggml-base.en.bin'
whisper_model_load: loading model
whisper_model_load: n_vocab = 51864
whisper_model_load: n_audio_ctx = 1500
whisper_model_load: n_audio_state = 512
whisper_model_load: n_audio_head = 8
whisper_model_load: n_audio_layer = 6
whisper_model_load: n_text_ctx = 448
whisper_model_load: n_text_state = 512
whisper_model_load: n_text_head = 8
whisper_model_load: n_text_layer = 6
whisper_model_load: n_mels = 80
whisper_model_load: ftype = 1
whisper_model_load: qntvr = 0
whisper_model_load: type = 2 (base)
whisper_model_load: adding 1607 extra tokens
whisper_model_load: n_langs = 99
Illegal instruction

I starting to believe I'm doing something dumb, does intel atom z8500 support openvino?

bobqianic
bobqianic1 year ago

I starting to believe I'm doing something dumb, does intel atom z8500 support openvino?

Although OpenVINO supports the Z8500, the issue likely arises because the Z8500 lacks AVX support, yet OpenVINO still utilizes it, leading to illegal instructions. Try adding the flags -DWHISPER_NO_AVX and -DWHISPER_NO_AVX2 when you're building with CMake. This might help with the issue.

image

Abdalnablse10
Abdalnablse101 year ago

I starting to believe I'm doing something dumb, does intel atom z8500 support openvino?

Although OpenVINO supports the Z8500, the issue likely arises because the Z8500 lacks AVX support, yet OpenVINO still utilizes it, leading to illegal instructions. Try adding the flags -DWHISPER_NO_AVX and -DWHISPER_NO_AVX2 when you're building with CMake. This might help with the issue.

image

cmake -B build -DWHISPER_OPENVINO=1 -DWHISPER_NO_AVX=1 -DWHISPER_NO_AVX2=1

cmake --build build -j --config Release

./build/bin/main -m ./models/ggml-base.en.bin -f ./samples/jfk.wav whisper_init_from_file_with_params_no_state: loading model from './models/ggml-base.en.bin'
whisper_model_load: loading model
whisper_model_load: n_vocab = 51864
whisper_model_load: n_audio_ctx = 1500
whisper_model_load: n_audio_state = 512
whisper_model_load: n_audio_head = 8
whisper_model_load: n_audio_layer = 6
whisper_model_load: n_text_ctx = 448
whisper_model_load: n_text_state = 512
whisper_model_load: n_text_head = 8
whisper_model_load: n_text_layer = 6
whisper_model_load: n_mels = 80
whisper_model_load: ftype = 1
whisper_model_load: qntvr = 0
whisper_model_load: type = 2 (base)
whisper_model_load: adding 1607 extra tokens
whisper_model_load: n_langs = 99
Illegal instruction

Looks like it didn't work.

bobqianic
bobqianic1 year ago (edited 1 year ago)

Looks like it didn't work.

Alright, please go to the OpenVINO repository and create an issue. I thought this issue was resolved in 2020, but it appears there's a regression.

openvinotoolkit/openvino#5782
openvinotoolkit/openvino#387

ggerganov
ggerganov approved these changes on 2023-12-29
ggerganov1 year ago

Cannot test OpenVINO build, but I think the usage of ggml_n_dims here is correct

ggerganov ggerganov merged f5f485f8 into master 1 year ago
bobqianic bobqianic deleted the fix-openvino branch 1 year ago

Login to write a write a comment.

Login via GitHub

Reviewers
Assignees
No one assigned
Labels
Milestone