Skip to content

[model] Support MiniCPM-V 4.0#39899

Closed
tc-mb wants to merge 38 commits into
huggingface:mainfrom
tc-mb:Support-MiniCPM-V-4.0
Closed

[model] Support MiniCPM-V 4.0#39899
tc-mb wants to merge 38 commits into
huggingface:mainfrom
tc-mb:Support-MiniCPM-V-4.0

Conversation

@tc-mb

@tc-mb tc-mb commented Aug 5, 2025

Copy link
Copy Markdown

This pull request supports the MiniCPM-V 4.0 model, which will be released soon.

@zucchini-nlp

Copy link
Copy Markdown
Member

Great! LMK if you need an early review or would like the PR to be merged before the release

@tc-mb

tc-mb commented Aug 6, 2025

Copy link
Copy Markdown
Author

Great! LMK if you need an early review or would like the PR to be merged before the release

Well, we released this MiniCPM-V 4 a few hours ago. We want to merge it into transformers. We'd be happy to have your help.

@zucchini-nlp

Copy link
Copy Markdown
Member

Yep, just saw, congrats on the release! Sure, I will do a preliminary review now and lmk if you need assistance with addressing comments/cleaning up the PR

@zucchini-nlp zucchini-nlp 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.

Hey, I feel like the code is pretty early-stage and doesn't fully align with transformers standards. I left comments below on what is expected to follow the mode addition standards

("megatron-bert", "MegatronBertModel"),
("mgp-str", "MgpstrForSceneTextRecognition"),
("mimi", "MimiModel"),
("minicpm_v_4", "MiniCPM_V_4Model"),

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.

we need also to add ForConditionalGeneration in image-text-to-text mapping

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

OK

Comment on lines +26 to +41
class MiniCPMVSliceConfig(PretrainedConfig):
model_type = "minicpmv"

def __init__(
self,
patch_size=14,
max_slice_nums=9,
scale_resolution=448,
**kwargs,
):
super().__init__(**kwargs)
self.patch_size = patch_size
self.max_slice_nums = max_slice_nums
self.scale_resolution = scale_resolution

@classmethod

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.

Let's not add a new config class unless it can be used to init a model class. We can move it either under MiniCPMVisionConfig or MiniCPMConfig depending on where in model arch these attr are used

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ok, now moved to MiniCPMVisionConfig

Comment on lines +58 to +65
class MiniCPM_V_4Config(LlamaConfig):
model_type = "minicpmv"
keys_to_ignore_at_inference = ["past_key_values"]

default_vision_config = {
"hidden_size": 1152,
"image_size": 980,
"intermediate_size": 4304,

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.

few comments here:

  1. We can't use inheritance outside "modularXXX.py" files
  2. The config has to be structured and contain a config.text_config, config.vision_config inside. So we need 3 config classes, where the text and vision configs can be just re-used from existing models. For ex, see LlavaConfig

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for the suggestion. Now I regenerate configuration_xx.py and modeling_xx.py using the utils/modular_model_converter.py. This should hopefully be more aligned with the requirements.

Comment on lines +19 to +24
import numpy as np
import PIL
import PIL.Image
import PIL.ImageSequence
import torch
from PIL import Image

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.

redundant imports

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ok

Comment on lines +26 to +34
from transformers import AutoImageProcessor
from transformers.image_processing_utils import BaseImageProcessor, BatchFeature
from transformers.image_transforms import to_channel_dimension_format
from transformers.image_utils import (
ChannelDimension,
ImageInput,
infer_channel_dimension_format,
is_batched,
is_torch_tensor,

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.

let's use relative import here

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ok

Comment on lines +24 to +30
from transformers.image_processing_utils import BatchFeature
from transformers.image_utils import ImageInput
from transformers.processing_utils import ProcessorMixin, ProcessingKwargs, Unpack, ImagesKwargs
from transformers.tokenization_utils_base import (
PaddingStrategy,
PreTokenizedInput,
TextInput,

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.

relative imports

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ok, all imports now fixed

Comment on lines +16 to +21
from transformers import LlamaTokenizerFast


class MiniCPM_V_4TokenizerFast(LlamaTokenizerFast):
def __init__(self, **kwargs):
super().__init__(**kwargs)

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.

we can just use LlamaTokenizer, no need to create a new tokenizer class. New special tokens can be added with https://huggingface.co/docs/transformers/en/main_classes/tokenizer#multimodal-tokenizer

Comment thread tests/models/minicpm_v_4/test.py Outdated
@@ -0,0 +1,45 @@
from transformers import AutoModelForCausalLM, AutoModel, AutoTokenizer

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.

to delete the file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ok


if is_torch_available():
import torch

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.

needs to have tests from common ModelTesterMixin and GenerationTesterMixin

variance_scaling_(tensor, mode="fan_in", distribution="normal")


class MiniCPMVisionEmbeddings(nn.Module):

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.

can do MiniCPMVisionEmbeddings(SiglipVisionEmbeddings) and outline only the differences. Ig in this case the diff is in position ids

Comment on lines +137 to +157
img_cnt = []
for pixel_values in pixel_values_list:
img_cnt.append(len(pixel_values))
all_pixel_values.extend([i.flatten(end_dim=1).permute(1, 0) for i in pixel_values])

# exist image
if all_pixel_values:
tgt_sizes = [tgt_size for tgt_size in tgt_sizes if isinstance(tgt_size, torch.Tensor)]
tgt_sizes = torch.vstack(tgt_sizes).type(torch.int32)

max_patches = torch.max(tgt_sizes[:, 0] * tgt_sizes[:, 1])

all_pixel_values = torch.nn.utils.rnn.pad_sequence(all_pixel_values, batch_first=True,
padding_value=0.0)
B, L, _ = all_pixel_values.shape
all_pixel_values = all_pixel_values.permute(0, 2, 1).reshape(B, 3, -1, L)

patch_attn_mask = torch.zeros((B, 1, max_patches), dtype=torch.bool, device=device)
for i in range(B):
patch_attn_mask[i, 0, :tgt_sizes[i][0] * tgt_sizes[i][1]] = True

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.

In continuation of discussion on MiniCPMBatchFeature:

I see we are padding pixels in any case when running the model, so i'd say we move do padding logic to image processors. That way we can use the default BatchFeature and it will be aligned with other UHD-like models

Comment on lines +160 to +169
if B > vision_batch_size:
hs = []
for i in range(0, B, vision_batch_size):
start_idx = i
end_idx = i + vision_batch_size
tmp_hs = self.vpm(all_pixel_values[start_idx:end_idx], patch_attention_mask=patch_attn_mask[start_idx:end_idx], tgt_sizes=tgt_sizes[start_idx:end_idx]).last_hidden_state
hs.append(tmp_hs)
vision_embedding = torch.cat(hs, dim=0)
else:
vision_embedding = self.vpm(all_pixel_values, patch_attention_mask=patch_attn_mask, tgt_sizes=tgt_sizes).last_hidden_state

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.

We don't need to do batched run here, and instead just forward all pixels at once. Batching is handled either by Trainer or user's own inference code

Comment on lines +180 to +191
else: # no image
if self.training:
dummy_image = torch.zeros(
(1, 3, 224, 224),
device=device, dtype=dtype
)
tgt_sizes = torch.Tensor([[(224 // self.config.patch_size), math.ceil(224 / self.config.patch_size)]]).type(torch.int32)
dummy_feature = self.resampler(self.vpm(dummy_image).last_hidden_state, tgt_sizes)
else:
dummy_feature = []
for _ in range(len(pixel_values_list)):
vision_hidden_states.append(dummy_feature)

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.

to delete, can't return random tensors if no images are present. The get_image_features methods can't be called without pixels

Comment on lines +196 to +199
if hasattr(self.llm.config, 'scale_emb'):
vllm_embedding = self.llm.model.embed_tokens(data['input_ids']) * self.llm.config.scale_emb
else:
vllm_embedding = self.llm.model.embed_tokens(data['input_ids'])

@zucchini-nlp zucchini-nlp Aug 19, 2025

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.

the config can always have scaling. If model doesn't need it, it will be set to 1.0 in config.json. Also we embed text ids in model's forward while self.get_image_features is reserved only to obtain processed image embeddings. Take a look at LLaVa model format

Comment on lines +215 to +216
cur_vllm_emb.scatter_(0, image_indices.view(-1, 1).repeat(1, cur_vllm_emb.shape[-1]),
cur_vs_hs.view(-1, cur_vs_hs.shape[-1]))

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.

Let's use out-of-place op here, and ig we could just get mask and masked_scatter once instead of iterating over each sample in batch

@github-actions

github-actions Bot commented Sep 1, 2025

Copy link
Copy Markdown
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: auto, minicpm_v_4

@vasqu

vasqu commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

What's the current state @tc-mb? 🤗

@tc-mb

tc-mb commented Jun 15, 2026

Copy link
Copy Markdown
Author

What's the current state @tc-mb? 🤗

This is too old; I'll get a new one when I have the chance.

@tc-mb tc-mb closed this Jun 15, 2026
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.

4 participants