[BugFix][Frontend] Backport vLLM #54548 sparse multimodal placeholder masks - #15861
QwertyJack wants to merge 2 commits into
Conversation
Signed-off-by: QwertyJack <7554089+QwertyJack@users.noreply.github.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a compatibility patch for Ascend-supported vLLM versions to address a bug where sparse multimodal placeholder masks were lost during serialization. By ensuring the 'is_embed' mask is preserved through the request lifecycle, the patch resolves shape mismatch issues in multimodal vision requests. The changes are applied dynamically at runtime to maintain compatibility with existing vLLM versions and include comprehensive unit tests to verify schema integrity and concurrent request handling. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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
Suggested PR Title:\n\nmarkdown\n[Platform][BugFix] Backport multimodal placeholder mask preservation\n\n\nSuggested PR Summary:\n\nmarkdown\n### What this PR does / why we need it?\nThis pull request introduces a compatibility backport (`patch_mm_placeholder_mask.py`) of `vllm-project/vllm#54548` to preserve sparse `PlaceholderRange.is_embed` masks through render/derender serialization and token-input reconstruction. It installs this patch dynamically during platform registration.\n\nFeedback on the implementation:\n1. In `_wrap_extract_features`, `engine_input` is assumed to be a dictionary, which might cause `TypeError` if it is a custom object in some vLLM versions. Additionally, `p.is_embed.tolist()` can raise an `AttributeError` if `is_embed` is already a list. A more defensive extraction is recommended.\n2. The use of `FunctionType` in `_wrap_serve_tokens` to dynamically recreate the function is fragile and can bypass the patch if the function is decorated. Using Python's `contextvars` is a much more robust and standard approach for managing per-request state in concurrent async tasks.\n3. The unit tests should be updated accordingly to reflect the `contextvars` approach.\n\n### Does this PR introduce _any_ user-facing change?\nNo, this is an internal compatibility patch to preserve multimodal placeholder masks.\n\n### How was this patch tested?\nUnit tests have been added in `tests/ut/patch/platform/test_mm_placeholder_mask.py` to verify serialization, idempotency, schema preservation, and concurrent request isolation.\n
| def _wrap_extract_features(original, placeholder_type): | ||
| @wraps(original) | ||
| def extract(engine_input): | ||
| features = original(engine_input) | ||
| if features is None: | ||
| return None | ||
| features.mm_placeholders = { | ||
| modality: [ | ||
| placeholder_type( | ||
| offset=p.offset, | ||
| length=p.length, | ||
| is_embed=None if p.is_embed is None else p.is_embed.tolist(), | ||
| ) | ||
| for p in ranges | ||
| ] | ||
| for modality, ranges in engine_input["mm_placeholders"].items() | ||
| } | ||
| return features | ||
|
|
||
| return extract |
There was a problem hiding this comment.
The current implementation of _wrap_extract_features assumes engine_input is always a dictionary and directly subscripts it with engine_input["mm_placeholders"]. However, in newer or different versions of vLLM, engine_input can be a custom object (such as MultiModalInputs or ExecutorInput), which will raise a TypeError when subscripted. Additionally, if p.is_embed is already a list rather than a PyTorch tensor, calling p.is_embed.tolist() will raise an AttributeError.
To ensure robust compatibility and defensive programming, we should safely extract mm_placeholders using both dictionary and attribute access, and handle list/tensor types for is_embed safely.
def _wrap_extract_features(original, placeholder_type):
@wraps(original)
def extract(engine_input):
features = original(engine_input)
if features is None:
return None
mm_placeholders = (
engine_input.get("mm_placeholders", {})
if isinstance(engine_input, dict)
else getattr(engine_input, "mm_placeholders", {})
)
features.mm_placeholders = {
modality: [
placeholder_type(
offset=p.offset,
length=p.length,
is_embed=(
None if p.is_embed is None
else (p.is_embed.tolist() if hasattr(p.is_embed, "tolist") else list(p.is_embed))
),
)
for p in ranges
]
for modality, ranges in mm_placeholders.items()
}
return features
return extractThere was a problem hiding this comment.
Keeping the upstream contract here: both supported references use MultiModalInput as a TypedDict and PlaceholderRange.is_embed as Tensor | None. The original extractor itself also uses dictionary access, so adding an object fallback after calling it would not support the proposed alternative. The serialization expression matches vllm-project/vllm#54548 and is covered for sparse and dense placeholders on both references.
| def _wrap_serve_tokens(original): | ||
| @wraps(original) | ||
| async def serve(self, request, raw_request=None): | ||
| if request.features is None or getattr(request, "content_parts", None): | ||
| return await original(self, request, raw_request) | ||
|
|
||
| import torch | ||
| from vllm.multimodal.inputs import PlaceholderRange | ||
|
|
||
| original_mm_input = original.__globals__["mm_input"] | ||
|
|
||
| def mm_input(**kwargs): | ||
| kwargs["mm_placeholders"] = { | ||
| modality: [ | ||
| PlaceholderRange( | ||
| offset=p.offset, | ||
| length=p.length, | ||
| is_embed=None if p.is_embed is None else torch.tensor(p.is_embed, dtype=torch.bool), | ||
| ) | ||
| for p in ranges | ||
| ] | ||
| for modality, ranges in request.features.mm_placeholders.items() | ||
| } | ||
| return original_mm_input(**kwargs) | ||
|
|
||
| # Bind only this request's input builder without mutating module globals | ||
| # across concurrent awaits or copying the version-specific async handler. | ||
| scoped = FunctionType( | ||
| original.__code__, | ||
| dict(original.__globals__, mm_input=mm_input), | ||
| original.__name__, | ||
| original.__defaults__, | ||
| original.__closure__, | ||
| ) | ||
| scoped.__kwdefaults__ = original.__kwdefaults__ | ||
| return await scoped(self, request, raw_request) | ||
|
|
||
| return serve |
There was a problem hiding this comment.
The current implementation of _wrap_serve_tokens uses FunctionType to dynamically recreate the original function with a modified globals dictionary. This approach is highly fragile and will fail if original is decorated (which is very common in web frameworks or other patches), because original.__code__ and original.__globals__ would belong to the decorator's wrapper rather than the actual function. Consequently, the mm_input override would be completely bypassed and ignored.
A much more robust, standard, and elegant way to handle per-request state in concurrent async tasks is using Python's contextvars module. We can patch mm_input in the module's globals once (idempotently) and use a ContextVar to dynamically apply the placeholder mask only when active for the current request context.
def _wrap_serve_tokens(original):
import contextvars
if not hasattr(_wrap_serve_tokens, "ctx_var"):
_wrap_serve_tokens.ctx_var = contextvars.ContextVar("mm_placeholder_mask_ctx", default=None)
ctx_var = _wrap_serve_tokens.ctx_var
globals_dict = original.__globals__
if "mm_input" in globals_dict and not getattr(globals_dict["mm_input"], "_is_patched_mask", False):
original_mm_input = globals_dict["mm_input"]
@wraps(original_mm_input)
def patched_mm_input(**kwargs):
mask_dict = ctx_var.get()
if mask_dict is not None:
import torch
from vllm.multimodal.inputs import PlaceholderRange
kwargs["mm_placeholders"] = {
modality: [
PlaceholderRange(
offset=p.offset,
length=p.length,
is_embed=None if p.is_embed is None else torch.tensor(p.is_embed, dtype=torch.bool),
)
for p in ranges
]
for modality, ranges in mask_dict.items()
}
return original_mm_input(**kwargs)
patched_mm_input._is_patched_mask = True
globals_dict["mm_input"] = patched_mm_input
@wraps(original)
async def serve(self, request, raw_request=None):
if request.features is None or getattr(request, "content_parts", None):
return await original(self, request, raw_request)
token = ctx_var.set(request.features.mm_placeholders)
try:
return await original(self, request, raw_request)
finally:
ctx_var.reset(token)
return serveThere was a problem hiding this comment.
Addressed in 06ab7d8. Removed FunctionType and directly backported the upstream rebuild_mm_placeholders call into the serving method from our supported vLLM pin. No ContextVar or per-request globals override is needed. The only release compatibility branches cover content_parts, non-streaming output kind, and session routing in v0.27.1. Tests now exercise the direct handler through mocked engine-client handoff, including concurrency, routing, streaming, validation, and exception isolation: 18 passed on the main pin; 17 passed and 1 unsupported-content-parts skip on v0.27.1.
| results = asyncio.run(run()) | ||
| assert [r["mm_placeholders"]["image"][0].get_num_embeds() for r in results] == [3, 1, 4] | ||
| assert results[0]["mm_placeholders"]["image"][0].is_embed.dtype == torch.bool | ||
| assert _upstream_handler.__globals__["mm_input"] is original_builder |
There was a problem hiding this comment.
Since we are replacing the fragile FunctionType recreation with a robust contextvars approach, the global mm_input in the module's globals will be patched once (idempotently) rather than recreated dynamically per request. Therefore, the assertion assert _upstream_handler.__globals__["mm_input"] is original_builder should be removed or updated to assert that it is the patched wrapper.
| results = asyncio.run(run()) | |
| assert [r["mm_placeholders"]["image"][0].get_num_embeds() for r in results] == [3, 1, 4] | |
| assert results[0]["mm_placeholders"]["image"][0].is_embed.dtype == torch.bool | |
| assert _upstream_handler.__globals__["mm_input"] is original_builder | |
| results = asyncio.run(run()) | |
| assert [r["mm_placeholders"]["image"][0].get_num_embeds() for r in results] == [3, 1, 4] | |
| assert results[0]["mm_placeholders"]["image"][0].is_embed.dtype == torch.bool |
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. Tip 💡 Consider Linking a Related Issue or RFCYour PR title contains the [BugFix] tag, indicating a bug fix or new feature. Linking a related issue or RFC in the PR description is strongly encouraged — it gives reviewers helpful context and speeds up the review. You can use any of these keywords:
🙏 Thanks for helping us keep the project well-organized! |
…mentation Signed-off-by: QwertyJack <7554089+QwertyJack@users.noreply.github.com>
|
/e2e tests/e2e/pull_request/four_card/test_pipeline_parallel.py |
|
/rerun Rerun (failed jobs only):
|
1 participant
What this PR does / why we need it?
Backports the sparse multimodal placeholder-mask fix from vllm-project/vllm#54548 (upstream author Hotragn, reviewed source head
60fe831acefe1778b99b5e0198581a3236d4c1c1) into an Ascend compatibility patch for supported vLLM versions that do not yet carry it.Render output currently serializes placeholder offset and length but loses
PlaceholderRange.is_embed. For sparse placeholders, reconstructing a dense range in/inference/v1/generatecan assign image embeddings to separator positions. A real DeepSeek V4 Vision request reproduced a 320-embedding versus 344-position shape mismatch through this route.The patch adds the optional JSON mask field, rebuilds nested request schemas, preserves masks during render serialization, and reconstructs CPU boolean masks for token-input requests. It also covers the duplicated feature extraction in
ServingDerender.The serving method is a direct source backport:
rebuild_mm_placeholdersfollows the upstream implementation, and its call site is applied to the supported vLLM main pinba07e4a48. The three existing v0.27.1 differences (content-parts support, non-streaming output kind, and session routing) are gated explicitly. There is noFunctionType,ContextVar, source rewriting, or request-scoped module-global override. Unrelated features introduced between the supported pin and the upstream PR base are not pulled into this backport.Installation occurs in the platform registration hook, after global/config imports and before API router construction. Versions with the native
is_embedprotocol field are left untouched. Remove this compatibility patch once all supported vLLM versions carry the upstream fix.This PR is based directly on
mainand is independent of #15783. It does not include model-runner, operator, evaluation-helper, or local numerical-stability changes.Does this PR introduce any user-facing change?
Yes. On affected vLLM versions, rendered multimodal features retain the optional
is_embedmask through JSON serialization and token-input reconstruction. Dense placeholders remain supported when the mask is absent. Text-only and content-parts requests retain the supported version's preprocessing behavior.How was this patch tested?
python -m pytest -q tests/ut/patch/platform/test_mm_placeholder_mask.py: 17 passed, 1 skipped against a clean vLLM 0.27.1 checkout (6e448d0ea9bf3d88d898b65449ca6dc2aec170ac,VLLM_VERSION=0.27.1). The skip is the content-parts test because that version has no such request field.18 tests passed against the pinned vLLM main reference
ba07e4a48fc951300d97eb506217dd530583dea3(VLLM_VERSION=0.28.0), using--noconftestto isolate this frontend suite from an unrelated missingattention.pcpimport in the full Ascend test setup.Coverage includes render/derender JSON round trips, dense/sparse masks, serialized tensors and cache hits, idempotent installation, concurrent requests, text/content-parts preprocessing, FastAPI schemas, engine-client handoff, streaming/non-streaming output kind, routing, sampling validation/defaults, and generation-error isolation. Engine calls are mocked; these are frontend unit tests.
The placeholder-builder body matches the upstream AST. The serving body was also compared with the supported pin to check that differences are limited to mask reconstruction and the explicit release compatibility branches.
A fresh CLI process using the clean vLLM 0.27.1 checkout successfully ran
vllm serve --help; the platform hook installed the direct serving backport, and the actual token-input router's OpenAPI schema retainedis_embed.Prior real-weight token-input teacher-forcing controls returned HTTP 200 after a direct local vLLM mask fix. That is evidence for the defect/fix semantics, not a real-NPU end-to-end test of this new Ascend compatibility wrapper. The wrapper itself has the frontend/handler checks above; no full-model numerical or performance sign-off is claimed.
Formatting and local hooks passed with gitleaks skipped because its downloaded binary cannot execute on this ARM64 host (
Exec format error), and actionlint skipped because no workflow files changed. Remote CI remains the merge gate.vLLM main: vllm-project/vllm@ba07e4a