fix(llm): correctly pass multimodal_projector to LlamaCppModel - #5350
fix(llm): correctly pass multimodal_projector to LlamaCppModel#5350Ricardo-M-L wants to merge 2 commits into
Conversation
When all config attributes (max_sequence_length, seq_length, max_position_embeddings) are None, the function incorrectly returned 2048 even though max() with all None values would raise ValueError. Now uses a generator expression with default parameter to properly handle the case where no values are available. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
LlamaCppModel.__init__ expects `llamacpp_model_config` dict as 4th positional argument, but was receiving the general `kwargs` dict. This caused multimodal_projector (and other llamacpp-specific params) to not be properly passed when launching GGUF models via CLI. Extract llamacpp-specific params (multimodal_projector, n_ctx, etc.) and pass them correctly to LlamaCppModel constructor. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces changes to extract LlamaCpp-specific configurations from kwargs when instantiating LlamaCppModel in core.py, and updates get_context_length in utils.py to handle missing sequence length attributes by defaulting them to None and using a fallback value of 2048. A critical bug was identified in the LlamaCppModel check where type(llm_cls).name evaluates to "type" instead of the class name, which would prevent the LlamaCpp-specific configuration block from executing. Using llm_cls.name directly resolves this issue.
| if peft_model is not None: | ||
|
|
||
| # Extract llamacpp-specific config from kwargs for LlamaCppModel | ||
| is_llamacpp = "LlamaCppModel" in type(llm_cls).__name__ |
There was a problem hiding this comment.
Since llm_cls is a class (e.g., XllamaCppModel), type(llm_cls) evaluates to type, and type(llm_cls).__name__ is always "type". Consequently, the condition "LlamaCppModel" in type(llm_cls).__name__ will always evaluate to False, and the LlamaCpp-specific configuration extraction will never execute. You should check llm_cls.__name__ directly instead.
| is_llamacpp = "LlamaCppModel" in type(llm_cls).__name__ | |
| is_llamacpp = "LlamaCppModel" in llm_cls.__name__ |
Ricardo-M-L
changed the title
|
there is conflict, and pls address gemini comment first |
3 participants
This PR addresses: fix(llm): correctly pass multimodal_projector to LlamaCppModel