Skip to content

[bugfix] Lazy import cv2#26869

Merged
ProExpertProg merged 1 commit intovllm-project:mainfrom
angelayi:angelayi/cv2_import
Oct 15, 2025
Merged

[bugfix] Lazy import cv2#26869
ProExpertProg merged 1 commit intovllm-project:mainfrom
angelayi:angelayi/cv2_import

Conversation

@angelayi
Copy link
Copy Markdown
Contributor

@angelayi angelayi commented Oct 15, 2025

Purpose

Fixes pytest -sv tests/standalone_tests/lazy_imports.py failure which I saw in this run

Test Plan

pytest -sv tests/standalone_tests/lazy_imports.py

Test Result

test passes

cc @ProExpertProg

Signed-off-by: angelayi <yiangela7@gmail.com>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly implements lazy loading for the cv2 module to fix a test failure. The current approach of moving the import into each function that uses it is valid. However, I've suggested using the LazyLoader utility, which is a more idiomatic and maintainable pattern within the vLLM codebase for handling lazy dependencies. This centralizes the lazy-loading logic and avoids repetitive import statements in functions. My review includes a concrete suggestion to adopt this pattern.

from functools import lru_cache
from typing import Any, ClassVar, Literal

import cv2
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Instead of moving the cv2 import inside each function, consider using the LazyLoader utility from vllm.utils. This is a cleaner and more idiomatic approach for lazy loading in this project, as seen with other modules. It avoids repeating import statements and keeps the function bodies cleaner, while still achieving the goal of lazy loading.

Suggested change
import cv2
from vllm.utils import LazyLoader
cv2 = LazyLoader("cv2", globals(), "cv2")

Comment on lines +82 to +83
import cv2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

With the proposed LazyLoader at the module level, this local import is no longer necessary and can be removed.

@ProExpertProg ProExpertProg enabled auto-merge (squash) October 15, 2025 03:37
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 15, 2025
@ProExpertProg ProExpertProg merged commit efdef57 into vllm-project:main Oct 15, 2025
48 checks passed
bbartels pushed a commit to bbartels/vllm that referenced this pull request Oct 16, 2025
Signed-off-by: angelayi <yiangela7@gmail.com>
Signed-off-by: bbartels <benjamin@bartels.dev>
albertoperdomo2 pushed a commit to albertoperdomo2/vllm that referenced this pull request Oct 16, 2025
Signed-off-by: angelayi <yiangela7@gmail.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
lywa1998 pushed a commit to lywa1998/vllm that referenced this pull request Oct 20, 2025
Signed-off-by: angelayi <yiangela7@gmail.com>
alhridoy pushed a commit to alhridoy/vllm that referenced this pull request Oct 24, 2025
Signed-off-by: angelayi <yiangela7@gmail.com>
0xrushi pushed a commit to 0xrushi/vllm that referenced this pull request Oct 26, 2025
Signed-off-by: angelayi <yiangela7@gmail.com>
Signed-off-by: 0xrushi <6279035+0xrushi@users.noreply.github.com>
0xrushi pushed a commit to 0xrushi/vllm that referenced this pull request Oct 26, 2025
Signed-off-by: angelayi <yiangela7@gmail.com>
Signed-off-by: 0xrushi <6279035+0xrushi@users.noreply.github.com>
rtourgeman pushed a commit to rtourgeman/vllm that referenced this pull request Nov 10, 2025
Signed-off-by: angelayi <yiangela7@gmail.com>
devpatelio pushed a commit to SumanthRH/vllm that referenced this pull request Nov 29, 2025
Signed-off-by: angelayi <yiangela7@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants