Skip to content

Conversation

@ngxson
Copy link
Collaborator

@ngxson ngxson commented Nov 9, 2025

Fix #17125

Tests are OK:

[audio]  OK:   llama-mtmd-cli ggml-org/ultravox-v0_5-llama-3_2-1b-GGUF:Q8_0
[audio]  OK:   llama-mtmd-cli ggml-org/Qwen2.5-Omni-3B-GGUF:Q4_K_M
[audio]  OK:   llama-mtmd-cli ggml-org/Voxtral-Mini-3B-2507-GGUF:Q4_K_M

Copy link
Collaborator

@CISC CISC left a comment

Choose a reason for hiding this comment

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

Can't you just have default values in clip_hparams though?

@ngxson
Copy link
Collaborator Author

ngxson commented Nov 9, 2025

Can't you just have default values in clip_hparams though?

Either way is possible, but I think it's better to avoid communicating patch_size should have a default value. It's technically should not have a default value. And in the case of audio model, it's more like a dummy value.

@CISC
Copy link
Collaborator

CISC commented Nov 9, 2025

Can't you just have default values in clip_hparams though?

Either way is possible, but I think it's better to avoid communicating patch_size should have a default value. It's technically should not have a default value. And in the case of audio model, it's more like a dummy value.

What about at least initializing it to 0 so it'll crash if one forgets for a new modality?

@henk717
Copy link

henk717 commented Nov 9, 2025

Initializing to 0 breaks the current audio model implementation. It was found because my system initialized it as 0 already resulting in audio models failing to launch at random moments.

@CISC
Copy link
Collaborator

CISC commented Nov 9, 2025

Initializing to 0 breaks the current audio model implementation. It was found because my system initialized it as 0 already resulting in audio models failing to launch at random moments.

I meant in clip_hparams (while still setting it to 1 for audio).

@ngxson
Copy link
Collaborator Author

ngxson commented Nov 9, 2025

What about at least initializing it to 0 so it'll crash if one forgets for a new modality?

yes and even better, I think there should be default value for other hparams too (added in 4ac2c20)

This should prevent forgetting to initialize one of the params in general.

@LostRuins
Copy link
Collaborator

also there are multiple GGML_ASSERT scattered throughout the code that can trip if patch size is an unexpected value even if unused. but this approach should work.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Qwen Omni divide by zero error (with solution)

4 participants