Skip to content

[Fix] MiniCPM-V 4.6: skip legacy minicpmv conv template, fall back to…#25053

Closed
tc-mb wants to merge 1 commit into
sgl-project:mainfrom
tc-mb:fix/minicpmv4_6-chat-template
Closed

[Fix] MiniCPM-V 4.6: skip legacy minicpmv conv template, fall back to…#25053
tc-mb wants to merge 1 commit into
sgl-project:mainfrom
tc-mb:fix/minicpmv4_6-chat-template

Conversation

@tc-mb
Copy link
Copy Markdown
Contributor

@tc-mb tc-mb commented May 12, 2026

Summary

match_minicpm matches the minicpm-v substring in MiniCPM-V-4_6 paths and forces the legacy v2.6 conv template ((<image>./</image>)), but 4.6's processor and chat_template.jinja use <|image_pad|> / <|video_pad|> — so the prompt and the processor are out of sync and images are never expanded.
Returning None for model_type == "minicpmv4_6" lets the manager fall back to the HF jinja template. 2.6 / 4.0 / 4.5 / o-2.6 untouched. Verified end-to-end on MiniCPM-V-4_6 (single-image, multi-image, text-only).

… HF jinja so prompt aligns with <|image_pad|>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request modifies the match_minicpm function to return None for the minicpmv4_6 model type, allowing the system to use the Hugging Face jinja chat template. Feedback suggests that the current implementation is too specific to version 4.6 and should be generalized to cover future versions as intended by the code comments.

Comment on lines +1153 to +1160
# MiniCPM-V 4.6 (and any future versions) ship a real HF jinja chat
# template that uses <|image_pad|> / <|video_pad|> instead of the legacy
# (<image>./</image>) placeholder. Returning None here lets the
# TemplateManager fall back to the HF tokenizer/processor template,
# which keeps the prompt aligned with the multimodal processor.
model_type = get_model_type(model_path)
if model_type == "minicpmv4_6":
return None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The comment mentions that this logic should apply to "any future versions", but the implementation specifically checks only for minicpmv4_6. If future versions (e.g., minicpmv4_7) or variants like minicpmo4_6 are released with HF jinja templates, they will fall through to the regex match on line 1161 and incorrectly return the legacy template. Consider using a more generic check (e.g., checking for a version prefix or a list of types) to align with the comment's intent and ensure future-proofing.

@tc-mb
Copy link
Copy Markdown
Contributor Author

tc-mb commented May 12, 2026

#24998
A PR was found to be in progress; this PR is now stopped.

@tc-mb tc-mb closed this May 12, 2026
@tc-mb tc-mb deleted the fix/minicpmv4_6-chat-template branch May 13, 2026 04:00
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.

1 participant