[CI] Fix PaddleOCR-VL HF test failure due to create_causal_mask API rename#37328
[CI] Fix PaddleOCR-VL HF test failure due to create_causal_mask API rename#37328hmellor merged 1 commit intovllm-project:mainfrom
Conversation
…ename Signed-off-by: Andreas Karatzas <akaratza@amd.com>
There was a problem hiding this comment.
Code Review
This pull request fixes a TypeError in the PaddleOCR-VL HF test by monkey-patching the create_causal_mask function to support a renamed parameter. The fix is correct, but the implementation of the monkey-patch has a potential issue with test isolation as it modifies a global module without reverting the change. I've added a comment with a suggestion for a more robust implementation to prevent side effects on other tests.
| def paddleocr_vl_patch_hf_runner(hf_model: HfRunner) -> HfRunner: | ||
| """Patches the HfRunner to fix create_causal_mask API mismatch. | ||
|
|
||
| The PaddleOCR-VL HF model passes `inputs_embeds` to create_causal_mask, | ||
| but transformers renamed this parameter to `input_embeds`. | ||
| """ | ||
| import sys | ||
|
|
||
| model_module = sys.modules.get(type(hf_model.model.model).__module__) | ||
| if model_module is None: | ||
| return hf_model | ||
|
|
||
| original_create_causal_mask = getattr(model_module, "create_causal_mask", None) | ||
| if original_create_causal_mask is None: | ||
| return hf_model | ||
|
|
||
| def patched_create_causal_mask(*args, **kwargs): | ||
| if "inputs_embeds" in kwargs: | ||
| kwargs["input_embeds"] = kwargs.pop("inputs_embeds") | ||
| return original_create_causal_mask(*args, **kwargs) | ||
|
|
||
| model_module.create_causal_mask = patched_create_causal_mask # type: ignore[attr-defined] | ||
| return hf_model |
There was a problem hiding this comment.
This monkey-patch modifies a module in sys.modules, which is a global state. This can break test isolation and cause side effects in other tests running in the same process. For instance, a different test might rely on the original behavior of create_causal_mask and would fail in an unexpected way.
To ensure test reliability, this patch should be reverted after the test finishes. The standard way to achieve this is by using a context manager or a pytest fixture that handles the setup and teardown of the patch.
A possible approach would be to refactor this into a context manager that can be used within the test execution logic:
from contextlib import contextmanager
@contextmanager
def patched_create_causal_mask_for_paddleocr_vl(hf_model: HfRunner):
import sys
model_module = sys.modules.get(type(hf_model.model.model).__module__)
original_create_causal_mask = getattr(model_module, "create_causal_mask", None)
if original_create_causal_mask is None:
yield
return
def patched_create_causal_mask(*args, **kwargs):
if "inputs_embeds" in kwargs:
kwargs["input_embeds"] = kwargs.pop("inputs_embeds")
return original_create_causal_mask(*args, **kwargs)
model_module.create_causal_mask = patched_create_causal_mask
try:
yield
finally:
model_module.create_causal_mask = original_create_causal_maskThis would require changes in the test runner to use the context manager, but it would make the test suite more robust.
There was a problem hiding this comment.
Global state is technically valid but not practically relevant here. The module itself is test-specific and only renames inputs_embeds.
…ename (vllm-project#37328) Signed-off-by: Andreas Karatzas <akaratza@amd.com>
…ename (vllm-project#37328) Signed-off-by: Andreas Karatzas <akaratza@amd.com>
…ename (vllm-project#37328) Signed-off-by: Andreas Karatzas <akaratza@amd.com> (cherry picked from commit eaf7c9b)
…ename (vllm-project#37328) Signed-off-by: Andreas Karatzas <akaratza@amd.com>
…ename (vllm-project#37328) Signed-off-by: Andreas Karatzas <akaratza@amd.com>
…ename (vllm-project#37328) Signed-off-by: Andreas Karatzas <akaratza@amd.com> Signed-off-by: Monishver Chandrasekaran <monishverchandrasekaran@gmail.com>
…ename (vllm-project#37328) Signed-off-by: Andreas Karatzas <akaratza@amd.com>
…ename (vllm-project#37328) Signed-off-by: Andreas Karatzas <akaratza@amd.com> Signed-off-by: Vinay Damodaran <vrdn@hey.com>
…ename (vllm-project#37328) Signed-off-by: Andreas Karatzas <akaratza@amd.com> Signed-off-by: EricccYang <yangyang4991@gmail.com>
create_causal_mask(inputs_embeds=...)but transformers renamed the parameter toinput_embeds(singular), causing aTypeErrorin the HF runnerpaddleocr_vl_patch_hf_runnerthat monkey-patchescreate_causal_maskin the model's module to accept the old parameter nameTest plan
pytest -s -v tests/models/multimodal/generation/test_common.py::test_single_image_models[paddleocr_vl-test_case34]cc @kenroche