Skip to content

clip : fix confused naming ffn_up and ffn_down #13290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 5, 2025

Conversation

ngxson
Copy link
Collaborator

@ngxson ngxson commented May 3, 2025

The old clip.cpp code reverse the ffn_up and ffn_down naming by mistake, which make it extremely messy when migrating the conversion script to convert_hf_to_gguf.py

To save myself from some headaches, so I decided to fix it once and for all 😤😤

The new rule is:

  • New model always have fc1 == ffn_up and fc2 == ffn_down
  • For old models, if the reversed ffn naming is detected, we reverse it back

This PR also contain name changing to make it more align with llama.cpp style:

  • n_intermediate --> n_embd
  • hidden_size --> n_ff

Small note: GGUF converted from the old qwen surgery script has n_ff = 0, hopefully this will not be messy in the future


Tested by converting fresh new GGUF and run it with llama-mtmd-cli locally:

  • Gemma 3 4B
  • SmolVLM2 2.2B
  • Qwen 2.5 3B

Tested with existing GGUF on the internet:

OK:   llama-mtmd-cli ggml-org/SmolVLM-500M-Instruct-GGUF:Q8_0
OK:   llama-mtmd-cli ggml-org/SmolVLM2-2.2B-Instruct-GGUF:Q4_K_M
OK:   llama-mtmd-cli ggml-org/SmolVLM2-500M-Video-Instruct-GGUF:Q8_0
OK:   llama-mtmd-cli ggml-org/gemma-3-4b-it-GGUF:Q4_K_M
OK:   llama-mtmd-cli guinmoon/MobileVLM-3B-GGUF:Q4_K_M
OK:   llama-mtmd-cli THUDM/glm-edge-v-5b-gguf:Q4_K_M
OK:   llama-mtmd-cli second-state/Llava-v1.5-7B-GGUF:Q2_K
OK:   llama-mtmd-cli cjpais/llava-1.6-mistral-7b-gguf:Q3_K
OK:   llama-mtmd-cli ibm-research/granite-vision-3.2-2b-GGUF:Q4_K_M
OK:   llama-mtmd-cli second-state/MiniCPM-Llama3-V-2_5-GGUF:Q2_K
OK:   llama-mtmd-cli openbmb/MiniCPM-V-2_6-gguf:Q2_K
OK:   llama-mtmd-cli openbmb/MiniCPM-o-2_6-gguf:Q4_0
OK:   llama-mtmd-cli bartowski/Qwen2-VL-2B-Instruct-GGUF:Q4_K_M
OK:   llama-mtmd-cli ggml-org/Qwen2.5-VL-3B-Instruct-GGUF:Q4_K_M
OK:   llama-mtmd-cli ggml-org/pixtral-12b-GGUF:Q4_K_M
OK:   llama-mtmd-cli ggml-org/Mistral-Small-3.1-24B-Instruct-2503-GGUF
OK:   llama-mtmd-cli ggml-org/Qwen2-VL-2B-Instruct-GGUF:Q4_K_M
OK:   llama-mtmd-cli ggml-org/Qwen2-VL-7B-Instruct-GGUF:Q4_K_M
OK:   llama-mtmd-cli ggml-org/Qwen2.5-VL-3B-Instruct-GGUF:Q4_K_M
OK:   llama-mtmd-cli ggml-org/Qwen2.5-VL-7B-Instruct-GGUF:Q4_K_M

@ngxson ngxson requested a review from ggerganov May 3, 2025 21:37
@github-actions github-actions bot added examples python python script changes labels May 3, 2025
@ngxson
Copy link
Collaborator Author

ngxson commented May 5, 2025

Hi @slaren @ggerganov , sorry for pinging but I realized that some models current does not work correctly without this fix.

If you have time, could you please review this PR? Thanks!

@ngxson ngxson merged commit 5215b91 into ggml-org:master May 5, 2025
57 of 58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples python python script changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants