[Bugfix][Model Runner V2] Restore multimodal draft capability detection - #50417
Conversation
NickLucche
left a comment
There was a problem hiding this comment.
can we do supports_multimodal here or similar reflection mechanisms? @DarkLight1337
| try: | ||
| dummy_input_ids = torch.tensor([[1]], device=self.device) | ||
| self.model.embed_input_ids( | ||
| dummy_input_ids, | ||
| multimodal_embeddings=None, | ||
| is_multimodal=None, | ||
| ) | ||
| except (NotImplementedError, AttributeError, TypeError): | ||
| logger.warning( | ||
| "Draft model does not support multimodal inputs, " | ||
| "falling back to text-only mode" | ||
| ) | ||
| self.supports_mm_inputs = False |
There was a problem hiding this comment.
can we spare the dummy forward here (trying to centralize everything into profiling run), and instead rely on self.model class inspection here ?
|
Thanks for the fix @TQCB. Like Nick suggested, I think it would be better to use vllm/vllm/lora/model_manager.py Lines 158 to 163 in 8700f86 embed_input_ids.
|
@TheEpicDolphin This would indeed be better, but
Neither of these are the case for most draft models. There is a hacky solution which would be: signature(self.model.embed_input_ids).bind(
None,
multimodal_embeddings=None,
is_multimodal=None,
)but I would prefer having a more explicit contract, rather than relying on this ad hoc check. Ideally there would be an advertized |
Yeah I prefer this approach as well |
|
Hello @NickLucche @TheEpicDolphin @DarkLight1337 , I've updated the PR to use a
These are the models I found merged external mm embeddings and don't already inherit |
|
Updated to use a protocol instead |
njhill
left a comment
There was a problem hiding this comment.
Thanks @TQCB! Looks good to me overall, just couple of minor comments.
Could we make the same changes here
vllm/vllm/v1/worker/gpu/spec_decode/multi_module_mtp/speculator.py
Lines 42 to 60 in dd11df0
There was a problem hiding this comment.
This could be moved to load_model to avoid allocating unnecessarily
In fact would it now make sense to move all of the self.support_mm_inputs logic into load_model?
Assisted-by: Codex Signed-off-by: Raphael Rialland <raphael.rialland@mistral.ai>
Assisted-by: Codex Signed-off-by: Raphael Rialland <raphael.rialland@mistral.ai>
Assisted-by: Codex Signed-off-by: Raphael Rialland <raphael.rialland@mistral.ai>
Assisted-by: Codex Signed-off-by: Raphael Rialland <raphael.rialland@mistral.ai>
Assisted-by: Codex Signed-off-by: Raphael Rialland <raphael.rialland@mistral.ai>
Assisted-by: Codex Signed-off-by: Raphael Rialland <raphael.rialland@mistral.ai>
Assisted-by: Codex Signed-off-by: Raphael Rialland <raphael.rialland@mistral.ai>
Assisted-by: Codex Signed-off-by: Raphael Rialland <raphael.rialland@mistral.ai>
9a5dc44 to
3d13da1
Compare
|
Thanks for the comments @njhill . I've moved the support check to the parent, and now all the children just check the flag and allocate the buffers they need based on the result. This feels pretty clean to me, LMK what you think! |
njhill
left a comment
There was a problem hiding this comment.
Thanks @TQCB LGTM!
cc @TheEpicDolphin @benchislett re the final structure
Purpose
Separate the target model ability to produce multimodal embeddings from the drafter ability to consume them.
Changes
SupportsMultiModalEmbeddingsprotocol and typed capability helper.SupportsMultiModalinherit the protocol.DraftModelSpeculator.load_model().Eagle3LlamaForCausalLMunmarked.Validation
Model evals were not run; no model implementation or model math changed.
Duplicate check
No open PR implements this capability contract. #36097 propagates multimodal embeddings but does not provide explicit drafter capability detection.
AI assistance
Prepared with assistance from OpenAI Codex; the human submitter reviewed the changes.