[Core] Use individual MM items in P0/P1 cache and model runner #22570
Conversation
Signed-off-by: DarkLight1337 <[email protected]>
DarkLight1337
requested review from
ywang96,
WoosukKwon,
robertgshaw2-redhat and
njhill
as code owners
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
mergify
bot
added
multi-modality
![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 is a significant refactoring of how multimodal inputs are handled, moving from MultiModalKwargs per request to a list of MultiModalKwargsItem. This change is aimed at improving the design for caching and processing of multimodal data. The changes are extensive, touching many files in the core engine, workers, and tests. The tests have been updated to reflect the new logic, which is a positive sign. However, I've identified a critical issue in the new MultiModalKwargsItem.__init__ method that can lead to runtime errors with empty inputs. Additionally, there's a potential data loss bug in gpu_model_runner.py when handling raw multimodal inputs with mixed modalities, which could silently drop data. These issues should be addressed to ensure the correctness of the new implementation.
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
| if len(batch) > 0 and is_list_of(batch, torch.Tensor, check="all"): | ||
| if len(batch) == 1: | ||
| # An optimization when `batch` contains only one tensor: | ||
| # - produce exactly same result as `torch.concat(batch)` | ||
| # - will achieve zero-copy if the tensor is contiguous | ||
| return batch[0].contiguous() | ||
|
|
||
| def _expect_same_shape(tensor: torch.Tensor): | ||
| return tensor.shape[:self.dim] + tensor.shape[self.dim + 1:] | ||
| dim = self.dim + (self.dim < 0) * len(batch[0].shape) |
There was a problem hiding this comment.
The extra self.dim < 0 check allows negative dim to be passed to this field
| @@ -51,6 +53,13 @@ def __post_init__(self): | |||
| def num_tokens(self) -> int: | |||
| return self.num_prompt_tokens + len(self.output_token_ids) | |||
|
|
|||
| # Temporary back-compatibility for plugins that define model runner | |||
There was a problem hiding this comment.
Signed-off-by: DarkLight1337 <[email protected]>
DarkLight1337
added
the
ready
Signed-off-by: DarkLight1337 <[email protected]>
|
Added ready label just to check CI, please don't merge yet as this is pending discussion with @ywang96 @WoosukKwon |
It is still used by |
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.Purpose
Follow-up to #22457, in preparation for moving processing cache from P0 to P1.
Key changes:
MultiModalKwargsItemcan now contain empty data.MultiModalKwargsItem, and modifiesMultiModalKwargsItemin place.EngineCoreRequest,Request,NewRequestData,CachedRequestStatenow usemm_kwargs: list[MultiModalKwargsItem]instead ofmm_inputs: list[MultiModalKwargs]. (cc @wangxiyuan please update vllm/vllm-ascend accordingly after this PR)merge_and_sort_multimodal_metadata -> argsort_mm_positionsandgroup_mm_inputs_by_modality -> group_mm_kwargs_by_modalitywith new semantics to enhance code reuse.pin_memoryargument for mergingMultiModalFieldElems (unused for now, see comment insidegroup_mm_kwargs_by_modality)Test Plan
Test Result
(Optional) Documentation Update