Skip to content

Fixes issue: #3924 - #3929

Open
michaelsam94 wants to merge 1 commit into
ggml-org:masterfrom
michaelsam94:codex/fix-invalid-ftype-load
Open

Fixes issue: #3924#3929
michaelsam94 wants to merge 1 commit into
ggml-org:masterfrom
michaelsam94:codex/fix-invalid-ftype-load

Conversation

@michaelsam94

Copy link
Copy Markdown

Problem: invalid model ftype values reached ggml_ftype_to_ggml_type() and triggered GGML_ASSERT, aborting instead of returning a load error.

Change: validate normalized hparams.ftype in whisper_model_load() before calling ggml_ftype_to_ggml_type().

Test: added test-model-load, which writes a minimal invalid model header with ftype = 5 and expects whisper_init_from_file_with_params() to fail cleanly.

Verification I ran: git diff --check; direct compiled and ran test-model-load.

CMake/CTest note: not run because cmake was unavailable in this environment.

Problem: invalid model ftype values reached ggml_ftype_to_ggml_type() and triggered GGML_ASSERT, aborting instead of returning a load error.

Change: validate normalized hparams.ftype in whisper_model_load() before calling ggml_ftype_to_ggml_type().

Test: added test-model-load, which writes a minimal invalid model header with ftype = 5 and expects whisper_init_from_file_with_params() to fail cleanly.

Verification I ran: git diff --check; direct compiled and ran test-model-load.

CMake/CTest note: not run because cmake was unavailable in this environment.

@danbev danbev left a comment

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.

Thanks for the PR!

I think that we might need something similar in parakeet.cpp and perhaps we could add a function like ggml_ftype_is_supported to ggml in the future that both could use. This would avoid having to update this code when new types are added to ggml. But that is outside of the scope of this PR.

Comment thread tests/test-model-load.cpp
}

whisper_context_params params = whisper_context_default_params();
whisper_context * ctx = whisper_init_from_file_with_params(path, params);

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.

Perhaps we can add std::remove(path) directly after whisper_init_from_file_with_params, so that we just have one std::remove instead of one in both code paths?

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