Skip to content

Fix MinicpmV model converter and clip to avoid using hardcode. #14750

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gryffindor-rr
Copy link

Make sure to read the contributing guidelines before submitting a PR
Tested by llama-mtmd-cli with multiple minicpmv models.

@github-actions github-actions bot added examples python python script changes labels Jul 18, 2025
@ngxson ngxson self-requested a review July 18, 2025 09:22
Copy link
Collaborator

@ngxson ngxson left a comment

Choose a reason for hiding this comment

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

Please create PR from a non-master branch next time, otherwise I cannot push my corrections directly to this PR

@gryffindor-rr
Copy link
Author

Please create PR from a non-master branch next time, otherwise I cannot push my corrections directly to this PR

sure. what branch will you suggest to use? shall I create a new one say 'minicpmv'?

@gryffindor-rr gryffindor-rr requested a review from ngxson July 28, 2025 05:51
@@ -44,6 +44,8 @@
#define KEY_WIN_ATTN_PATTERN "clip.vision.n_wa_pattern"
#define KEY_ATTN_WINDOW_SIZE "clip.vision.window_size"
#define KEY_MINICPMV_VERSION "clip.minicpmv_version"
#define KEY_MINICPMV_QUERY_NUM "clip.minicpmv_query_num"
#define KEY_MINICPMV_PROJ_DIM "clip.minicpmv_projection_dim"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#define KEY_MINICPMV_PROJ_DIM "clip.minicpmv_projection_dim"

@@ -201,6 +201,8 @@ struct clip_hparams {
// legacy
bool has_llava_projector = false;
int minicpmv_version = 0;
int32_t minicpmv_query_num = 0; // MiniCPM-V query number
int32_t minicpmv_projection_dim = 0; // MiniCPM-V projection dimension
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
int32_t minicpmv_projection_dim = 0; // MiniCPM-V projection dimension

@@ -2110,7 +2105,18 @@ struct clip_model_loader {
get_u32(KEY_PATCH_SIZE, hparams.patch_size);
get_u32(KEY_IMAGE_CROP_RESOLUTION, hparams.image_crop_resolution, false);
get_i32(KEY_MINICPMV_VERSION, hparams.minicpmv_version, false); // legacy

get_u32(KEY_MINICPMV_QUERY_NUM, hparams.minicpmv_query_num, false);
get_u32(KEY_MINICPMV_PROJ_DIM, hparams.minicpmv_projection_dim, false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
get_u32(KEY_MINICPMV_PROJ_DIM, hparams.minicpmv_projection_dim, false);

# Read config.json to get actual model configuration
config_path = os.path.join(dir_model, "config.json")
model_config = {}
if os.path.exists(config_path):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if os.path.exists(config_path):
if os.path.isfile(config_path):

Comment on lines +682 to +684
resampler_emb_dim = model_config.get("hidden_size", 0) if model_config else 0
fout.add_uint32("clip.minicpmv_query_num", query_num)
fout.add_uint32("clip.minicpmv_projection_dim", resampler_emb_dim)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
resampler_emb_dim = model_config.get("hidden_size", 0) if model_config else 0
fout.add_uint32("clip.minicpmv_query_num", query_num)
fout.add_uint32("clip.minicpmv_projection_dim", resampler_emb_dim)
fout.add_uint32("clip.minicpmv_query_num", query_num)

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.

3 participants