[None][fix] fix Qwen-VL processor _defaults mutation at the source#14617
[None][fix] fix Qwen-VL processor _defaults mutation at the source#14617longlee0622 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR replaces a context-manager validation bypass with a targeted processor re-classing mechanism to prevent Qwen-VL ChangesQwen-VL Processor Defaults Mutation Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unittest/_torch/modeling/test_qwen_vl_processor_defaults_fix.py (1)
104-258: QA list updates look unnecessary.These additions are unit-test scoped under
tests/unittest, so I would not expect any update totests/integration/test_lists/qa/*in this PR.As per coding guidelines, "This PR’s change is unit-test scoped (under tests/unittest), so you generally do not need to update these QA lists unless adding new end-to-end functional coverage."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/modeling/test_qwen_vl_processor_defaults_fix.py` around lines 104 - 258, The PR added only unit tests under tests/unittest (e.g., functions like test_install_returns_true_for_known_processor, test_install_is_idempotent, test_defaults_not_polluted_after_call_with_output_keys) so the QA lists in tests/integration/test_lists/qa/* should not be modified; revert any changes to those QA list files and keep only the new unit test file changes, ensuring no integration QA list entries were added for this unit-test-scoped change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unittest/_torch/modeling/test_qwen_vl_processor_defaults_fix.py`:
- Around line 239-248: The test currently swallows every Exception when calling
proc._get_num_multimodal_tokens (image_sizes/video_sizes/video_grid_thw), which
masks unrelated failures; narrow the except to only the concrete exceptions
expected from the unpatched upstream stub (e.g., replace "except Exception:"
with "except (ValueError, TypeError):" or the exact exception class the upstream
stub raises) so unrelated errors still surface—update the except clause around
proc._get_num_multimodal_tokens accordingly and add a brief comment noting to
adjust the caught types if the upstream raises a different specific exception.
---
Nitpick comments:
In `@tests/unittest/_torch/modeling/test_qwen_vl_processor_defaults_fix.py`:
- Around line 104-258: The PR added only unit tests under tests/unittest (e.g.,
functions like test_install_returns_true_for_known_processor,
test_install_is_idempotent,
test_defaults_not_polluted_after_call_with_output_keys) so the QA lists in
tests/integration/test_lists/qa/* should not be modified; revert any changes to
those QA list files and keep only the new unit test file changes, ensuring no
integration QA list entries were added for this unit-test-scoped change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b93ba615-b5c7-401f-80a1-19436ad42573
📒 Files selected for processing (5)
tensorrt_llm/_torch/models/modeling_multimodal_utils.pytensorrt_llm/_torch/models/modeling_qwen2vl.pytensorrt_llm/_torch/models/modeling_qwen3vl.pytests/unittest/_torch/modeling/test_modeling_multimodal.pytests/unittest/_torch/modeling/test_qwen_vl_processor_defaults_fix.py
The previous workaround (`bypass_processor_output_validation`) patched
`validate_typed_dict` at module level to filter processor *output* keys
out of HF's per-modality `TypedDict` validation. That hid a real upstream
bug in `Qwen2/2.5/3-VL` processors: their `_get_num_multimodal_tokens`
does
`<ProcessorKwargs>._defaults.get("<modality>_kwargs", {}).update(kwargs)`
on the class-level default dict (instead of a copy). The first call that
forwards processor output keys (e.g. `video_grid_thw`) bakes them into
the per-modality default, and every subsequent processor call then
merges those keys into `output_kwargs[<modality>]` and trips
`ProcessorMixin._merge_kwargs` validation.
Fix the bug at the source: `install_qwen_vl_processor_defaults_fix()`
re-classes a loaded Qwen-VL processor instance to a thin TRT-LLM subclass
that overrides only `_get_num_multimodal_tokens` and takes a defensive
`dict(...)` copy before merging caller kwargs. No global state is
touched; the override runs entirely on instance methods and local copies,
so it is naturally thread-safe with no locks or refcounting. The
process-wide module-level patch (`bypass_processor_output_validation`)
is removed, along with its call sites in
`modeling_qwen2vl.py` / `modeling_qwen3vl.py`.
Adds a unit test module that verifies the fix installs (re-classes the
instance), is idempotent, leaves class-level `_defaults` untouched after
calls that pass output keys, and that the upstream bug still exists on
the current `transformers` (so the regression test isn't vacuous).
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Per review feedback, ``install_qwen_vl_processor_defaults_fix`` (and its helpers ``_make_safe_get_num_multimodal_tokens``, ``_QWEN_VL_KWARGS_CLASS_BY_PROCESSOR``, ``_QWEN_VL_PROCESSOR_OUTPUT_KEYS``) is Qwen-VL-specific and does not belong in the cross-model ``modeling_multimodal_utils`` surface. Move the workaround into ``modeling_qwen2vl.py`` (next to its only consumers) and re-import it from ``modeling_qwen3vl.py`` alongside the existing ``Qwen2_5_VLVisionAttention`` import. Tests follow the new import path. Behavior is unchanged. Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
e39ac28 to
4b36a02
Compare
|
/bot run |
|
PR_Github #50682 [ run ] triggered by Bot. Commit: |
|
PR_Github #50682 [ run ] completed with state |
The previous workaround (
bypass_processor_output_validation) patchedvalidate_typed_dictat module level to filter processor output keys out of HF's per-modalityTypedDictvalidation. That hid a real upstream bug inQwen2/2.5/3-VLprocessors: their_get_num_multimodal_tokensdoeson the class-level default dict (instead of a copy). The first call that forwards processor output keys (e.g.
video_grid_thw) bakes them into the per-modality default, and every subsequent processor call then merges those keys intooutput_kwargs[<modality>]and tripsProcessorMixin._merge_kwargsvalidation.Fix the bug at the source:
install_qwen_vl_processor_defaults_fix()re-classes a loaded Qwen-VL processor instance to a thin TRT-LLM subclass that overrides only_get_num_multimodal_tokensand takes a defensivedict(...)copy before merging caller kwargs. No global state is touched; the override runs entirely on instance methods and local copies, so it is naturally thread-safe with no locks or refcounting. The process-wide module-level patch (bypass_processor_output_validation) is removed, along with its call sites inmodeling_qwen2vl.py/modeling_qwen3vl.py.Adds a unit test module that verifies the fix installs (re-classes the instance), is idempotent, leaves class-level
_defaultsuntouched after calls that pass output keys, and that the upstream bug still exists on the currenttransformers(so the regression test isn't vacuous).Summary by CodeRabbit
Bug Fixes
Tests
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.