[OpenVINO]add support for minicpmv4/4_5 #1412
Conversation
openvino-dev-samples
changed the title
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
@IlyasMoutawwakil could help to take a look ? |
|
Thanks for the fix ! let's create a tiny random model with llama as the decoder to test this 🤗 tell me you need help with that ! |
But i guess we need merge this PR first ? otherwise test case will not work |
|
@openvino-dev-samples no need to merge it now, you can simply pin that PR in setup.py so that the tests would run with it 🤗 |
Hi since minicpmv4 and minicpmv share a same model type, but different LLM. It is possible to add both of them in utils_tests.py ? |
|
@openvino-dev-samples yes, you can name it minicpmv4 in utils_tests.py |
|
Hi @openvino-dev-samples it would be faster if you made sure the minicpmv4 tests pass locally, the ci is slow and shouldn't be used as a testing mechanism, only use it for validating when local tests are already passing. |
Sorry for that, and i fully understand, but i always met connection issue in local test case, e.g
|
you can target minicpmv tests specifically to avoid this issue with pytest -k "minicpmv" |
openvino-dev-samples
changed the title
tests/openvino/utils_tests.py
Outdated
| "minicpm3": "katuni4ka/tiny-random-minicpm3", | ||
| "minicpmv": "katuni4ka/tiny-random-minicpmv-2_6", | ||
| "minicpmv4": "snake7gun/minicpm-v-4-tiny", | ||
| "minicpmv4_5": "snake7gun/tiny-minicpmv-4_5", |
There was a problem hiding this comment.
158M model size, it makes sense to try to reduce the size
|
@IlyasMoutawwakil could you help to trigger CI, thanks |
| if isinstance(behavior, str) and not isinstance(behavior, MiniCPMVConfigBehavior): | ||
| behavior = MiniCPMVConfigBehavior(behavior) | ||
|
|
||
| model_mapping = {2.6: "llama", 4.0: "qwen2", 4.5: "qwen3"} |
There was a problem hiding this comment.
should use str for versions
There was a problem hiding this comment.
may i understand why？the version in model's config is a number:
https://huggingface.co/openbmb/MiniCPM-V-4_5/blob/main/config.json#L3
There was a problem hiding this comment.
ah okay I see ! thanks for the clarification.
(it's generally a bad idea to use numbers for versions: 4.0 becomes 4 and 4.10 and 4.1 are the same version 😅)
| if isinstance(behavior, str) and not isinstance(behavior, MiniCPMVConfigBehavior): | ||
| behavior = MiniCPMVConfigBehavior(behavior) | ||
|
|
||
| model_mapping = {2.6: "llama", 4.0: "qwen2", 4.5: "qwen3"} |
There was a problem hiding this comment.
I think it is a bad idea to make decision about architecture based on model version in general.
I think you should parse class model object and use isinstance for inner objects to make decision.
There was a problem hiding this comment.
Yes, its a better approach in this case, but I dont know if we can access the modeling file at this stage.
| # skip the temporal_ids which makes the number of loop inconstant: | ||
| # https://huggingface.co/openbmb/MiniCPM-V-4_5/blob/main/resampler.py#L261 | ||
| inputs.pop("temporal_ids", None) |
There was a problem hiding this comment.
don't quite understand. To me it degrades user experience that we make users to skip this parameter after preprocessing to have matched results with the reference.
There was a problem hiding this comment.
Yes, it will impact the user experience in some cases. I think its one of the limitation for torchscipt. I will try to find a better solution handle this parameter if possible.
4 participants
Depends on PR
As LLM of minicpmv4 switched to Llama
https://huggingface.co/openbmb/MiniCPM-V-4/blob/main/modeling_minicpmv.py#L26