[MM] Simplify _apply_hf_processor_main - #53275
Conversation
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
|
@claude review |
|
Documentation preview: https://vllm--53275.org.readthedocs.build/en/53275/ |
mergify
Bot
added
documentation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this revision and found no new bugs. The two issues flagged in my previous review — the missing text argument in Dots3Note's HF processor call, and the dropped Chameleon sep-token for text-only prompts — both appear fixed in the current diff. Because this is a large refactor of the core multimodal HF-processor dispatch touching 83 files across nearly every model in the repo, a human look is still worthwhile given the scale and the number of model-specific overrides involved.
What was reviewed:
- Confirmed Dots3Note's
_apply_hf_processor_mainnow supplies dummy text before calling the custom processor, matching the pattern used by other custom-processor models. - Confirmed Chameleon's sep-token append now happens unconditionally, before the early-return for text-only prompts.
- Investigated candidate issue: a dead
input_idsfield left inllava_onevision.py's processed outputs — ruled out as non-harmful. - Investigated candidate issue: Qwen3-Omni audio padding using the wrong feature extractor when
mm_processor_kwargsoverrides it — ruled out.
Extended reasoning...
Overview
This PR is a large refactor of vLLM's multimodal HF-processor dispatch: it inlines _call_hf_processor, _apply_hf_processor_text_mm, _apply_hf_processor_tokens_only, and _apply_hf_processor_mm_only into a single _apply_hf_processor_main, removes is_update_applied from several return signatures in favor of a class attribute, and bumps the minimum transformers version to 5.10.4 so HF processors can be called with multimodal data alone (no text). Nearly every multimodal model file in the repo (83 files total) is touched to migrate to the new override pattern, each with model-specific logic for whether dummy text needs to be supplied.
Security risks
None identified. This is a refactor of internal processing dispatch logic with no new external inputs, auth, or serialization surfaces.
Level of scrutiny
This warrants a high level of scrutiny: it changes a core code path (vllm/multimodal/processing/processor.py) that every multimodal model routes through, and the correctness of each of the 70+ per-model overrides depends on subtle behavioral details (whether a given HF processor requires text, whether extra tokens like sep-tokens need unconditional handling, etc.) that are easy to get wrong per-model, as evidenced by the two bugs already caught and fixed in this PR's review history. Given the very large surface area, even with no findings this round, exhaustive per-model verification is hard to guarantee from static review alone.
Other factors
Both previously-reported bugs are fixed in the current diff, indicating the author is responsive to review feedback. No outstanding unresolved comments remain in the thread. The PR still lacks a filled-in test plan/test results section per the PR description checklist, and given the scope of behavioral changes across so many models, human review and ideally broader test/eval coverage (e.g. running the existing multimodal processing test suite across the affected models) would add meaningful confidence beyond what static review can provide.
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
|
@claude review |
There was a problem hiding this comment.
Beyond the two inline findings, I also checked NemotronParse's HF processor call path after its _call_hf_processor override was removed — its processor accepts multi-modal-only calls without a required text argument (unlike the Dots3Note/MiniCPMV cases), so it is not affected by the same missing-text issue.
Extended reasoning...
Checked whether NemotronParseMultiModalProcessor, which had its custom _call_hf_processor (text-vs-mm branching) deleted in this PR with no replacement _get_hf_processor_text override, would hit the same 'HF processor called without required text' crash seen in Dots3Note. NemotronParse does not override get_hf_processor, and its underlying processor supports being called with only image data (no text key) under the new transformers>=5.10.4 requirement this PR adds, so the default _apply_hf_processor_main path (text=None) works correctly here; this is a different situation from Dots3NoteProcessor.call, which has a required positional text argument.
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
|
@claude review |
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
|
@claude review |
|
/ci run |
|
✅ Triggered Buildkite CI #85069 for commit |
There was a problem hiding this comment.
Automated review ran on the latest revision and found no bugs. Given the scale of this refactor (touches the HF-processor call path for ~80 model processors), a human look is still worthwhile before merging.
What was reviewed: the new _apply_hf_processor_main/_get_hf_processor_text/_postprocess_hf_mm_data/_postprocess_prompt hooks and their per-model overrides; the NemotronParse mm-only call path (flagged as possibly missing a required text argument) — ruled out, its _apply_hf_processor_main no longer requires a custom override since the base class now handles the text-less HF call correctly for its processor.
Extended reasoning...
Overview
This PR is a large follow-up refactor (87 files) that flattens _call_hf_processor, _apply_hf_processor_text_mm, _apply_hf_processor_tokens_only, and _apply_hf_processor_mm_only into a single _apply_hf_processor_main hook, introduces _get_hf_processor_text/_postprocess_hf_mm_data/_postprocess_prompt as narrower override points, removes is_update_applied from several return signatures in favor of a hf_processor_applies_updates class attribute, and bumps the minimum transformers version to 5.10.4. Nearly every multimodal model processor in the repo is touched to adapt to the new API.
Security risks
None identified — this is an internal refactor of the HF-processor invocation pipeline with no changes to authentication, input trust boundaries, or serialization paths.
Level of scrutiny
This warrants a high level of scrutiny: it changes a core abstraction (BaseMultiModalProcessor) used by essentially every multimodal model in vLLM, and earlier commits in this PR's history had genuine regressions (missing overrides causing crashes, a dropped sep-token post-process step, a stale test signature, a broken super() call) that were caught by prior automated review passes and have since been fixed in the current revision. Given the breadth of the change and its history of subtle breakage, per-model correctness is very hard to fully verify without running each model's test suite.
Other factors
All previously flagged issues in this PR's timeline (dots3_note missing text arg, Chameleon sep-token loss, AudioFlamingo3 test signature mismatch, MiniCPMV super()._call_hf_processor breakage, stale error-message references) appear resolved in the current diff. CI was triggered on the latest commit. No bugs were found in this pass, but the size and criticality of the change is reason enough to keep a human in the loop.
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
|
/ci run |
|
@claude review |
|
✅ Triggered Buildkite CI #85084 for commit |
DarkLight1337
changed the title
There was a problem hiding this comment.
This automated review found no bugs in the current diff. Given the scope of this change (87 files, a core refactor of how every multimodal model's HF processor is invoked), a human review is still warranted before merge.
What was reviewed: the new _apply_hf_processor_main/_get_hf_processor_text/_preprocess_hf_mm_data/_postprocess_hf_mm_data/_postprocess_prompt hooks in vllm/multimodal/processing/processor.py and their per-model overrides across the diff; the removal of is_update_applied from the processor return type and its replacement with the hf_processor_applies_updates class flag; and the Qwen3-Omni audio pre-padding hop_length usage, which was checked and is not affected by kwargs handling.
Extended reasoning...
Overview
This PR refactors vLLM's core multimodal HF-processor invocation pipeline: it inlines _call_hf_processor, _apply_hf_processor_text_mm, _apply_hf_processor_tokens_only, and _apply_hf_processor_mm_only into a single _apply_hf_processor_main, introduces new override points (_get_hf_processor_text, _preprocess_hf_mm_data, _postprocess_hf_mm_data, _postprocess_prompt), removes is_update_applied from several return signatures in favor of a class-level hf_processor_applies_updates flag, and bumps the minimum transformers version. It touches 87 files, including per-model processor overrides for essentially every multimodal model vLLM supports (LLaVA family, Qwen-VL/Omni family, InternVL, MiniCPM-V/O, Molmo, Gemma, Whisper/audio models, and many out-of-tree model directories under vllm/models/).
Security risks
None identified. This is an internal refactor of processing/tokenization plumbing; it does not touch auth, network-facing surfaces, or untrusted-input parsing beyond what already existed (multimodal data is already treated as trusted input from the request).
Level of scrutiny
High. This change touches a shared, critical code path (BaseMultiModalProcessor) used by nearly every multimodal model in the repository, with correctness depending on each of ~40 per-model override sites being updated consistently with the new hook contracts. Earlier rounds of automated review on this same PR surfaced several real, high-severity issues in that category (a crash-on-every-request bug in Dots3Note, an AttributeError crash in MiniCPMV image/video requests, a broken test relying on the old _call_hf_processor signature, and a silent behavior regression in Chameleon's sep-token handling for text-only prompts) — all of which were subsequently fixed by the author in later commits. That history, combined with the sheer surface area of the change, supports treating this as a change that benefits from a human pass even though this run's automated pass found nothing.
Other factors
The author has been responsive to each round of automated feedback, and the diff now reflects fixes for every previously reported issue (verified by re-reading the current diff for those specific files). This run's bug hunting system also investigated and ruled out one additional candidate (Qwen3-Omni's hop_length-based audio pre-padding). Given the size and criticality of the change, and per project guidance to defer on large-scale refactors of critical paths, I recommend a human review pass focused on the doc/design-doc consistency (docs/design/mm_processing.md, docs/contributing/model/multimodal.md) and spot-checking a sample of the ~40 per-model overrides for hook-contract consistency.
DarkLight1337
added
the
ready-run-all-tests
|
/ci run |
|
✅ Triggered Buildkite CI #85153 for commit |
2 participants
Purpose
Follow-up to #53093
_call_hf_processor,_apply_hf_processor_text_mm,_apply_hf_processor_tokens_only, and_apply_hf_processor_mm_onlyinto the existing_apply_hf_processor_main, reducing code indirection. Nevertheless to avoid duplicate code, we introduce_get_hf_processor_text,_preprocess_hf_mm_dataand_postprocess_hf_mm_datawhich are still much easier to understand than the previous code.promptargument from_apply_hf_processor_main. Models that modifiedpromptshould override_postprocess_promptinstead.is_update_appliedfrom the return type of_apply_hf_processor_main,_apply_hf_processor. and_cached_apply_hf_processor. Models that setis_update_applied=Trueshould overideBaseMultiModalProcessor.hf_processor_applies_updatesinstead.call_hf_processor_mm_onlycalls with callingctx.call_hf_processorwithout text.Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.