Repository navigation
[Bugfix] Handle numpy array outputs when generate image - #1680
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08bb4cf9b7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
08bb4cf to
7ea1e60
Compare
|
why use /v1/images/generations as the endpoint? |
|
Sorry, I checked the example and it shows that this API is used. Is it not usable? |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Review
Rating: 6/10 | Verdict:
Summary
Bugfix addresses valid issue (Wan2.2 returning numpy arrays), but implementation has critical edge cases and missing tests. Code logic is unclear and needs refactoring.
Issues
-
Critical: IndexError on empty list - Line 1424 checks
images[0]without verifying the list is non-empty. Will crash ifimagesis empty. -
Confusing logic - Line 1425
images = images[0]is unclear. Is this handling a nested array case? Needs comment explaining the expected structure. -
Missing tests - No unit tests for the new
_normalize_imagefunction or the modified_extract_images_from_result. -
Incomplete PR description - Checklist items not filled, no test results provided.
-
Edge cases not handled:
- Empty images list
- Mixed types (some PIL, some numpy)
- Unsupported dtype (e.g., bool, complex)
Highlights
- ✅ Addresses real issue (Wan2.2 compatibility)
- ✅ Handles multiple normalization cases (float [0,1] and [-1,1])
- ✅ Properly handles ndim > 3 case
Recommendation
Fix edge cases and add tests before merge.
Reviewed by OpenClaw with vllm-omni-skills 🦐
| elif hasattr(request_output, "images") and request_output.images: | ||
| images = request_output.images | ||
| return images | ||
| if isinstance(images[0], np.ndarray): |
There was a problem hiding this comment.
This will raise IndexError if images is empty. Add a guard: if images and isinstance(images[0], np.ndarray):
| images = request_output.images | ||
| return images | ||
| if isinstance(images[0], np.ndarray): | ||
| images = images[0] |
There was a problem hiding this comment.
This logic is confusing. If images[0] is a numpy array, you replace the entire images list with images[0]. Is this handling a nested array structure like [np_array_of_frames]? Add a comment explaining the expected input format.
There was a problem hiding this comment.
yes, when generate more one image this array structure is np_array_of_frames. like:
images = np.array([
[[...], [...], ...], # one png (256, 256, 3)
[[...], [...], ...], # two png (256, 256, 3)
[[...], [...], ...], # three png (256, 256, 3)
])
# shape: (3, 256, 256, 3)
| setattr(object, key, val) | ||
|
|
||
|
|
||
| def _normalize_image(image: Any) -> Any: |
There was a problem hiding this comment.
The function handles floating dtypes but doesn't check for unsupported types (e.g., bool, complex). Consider adding: if not np.issubdtype(image.dtype, np.integer) and not np.issubdtype(image.dtype, np.floating): raise ValueError(f"Unsupported dtype: {image.dtype}")
| def _normalize_image(image: Any) -> Any: | ||
| """Normalize a single image output to a PIL-compatible format.""" | ||
| if isinstance(image, np.ndarray): | ||
| while image.ndim > 3: |
There was a problem hiding this comment.
The normalization assumes float range is either [-1, 1] or [0, 1]. What about other ranges? Consider adding a debug log or warning when rescaling from [-1, 1] range to help diagnose issues.
| if isinstance(images[0], np.ndarray): | ||
| images = images[0] | ||
| return [_normalize_image(img) for img in images] | ||
|
|
There was a problem hiding this comment.
Missing unit test for _normalize_image and _extract_images_from_result. At minimum, add tests for: (1) numpy array with float [0,1], (2) numpy array with float [-1,1], (3) numpy array with int, (4) already PIL Image, (5) empty list.
|
@lishunyang12 PTAL. Thanks! |
|
Will take a look. |
lishunyang12
left a comment
There was a problem hiding this comment.
The idea is right but there are two bugs in the implementation.
Signed-off-by: rongfu.leng <lenronfu@gmail.com>
Signed-off-by: rongfu.leng <lenronfu@gmail.com>
ae9af4a to
fa13f7c
Compare
Signed-off-by: rongfu.leng <lenronfu@gmail.com>
gcanlin
left a comment
There was a problem hiding this comment.
Could you upload the generated images?
…#1680) Signed-off-by: rongfu.leng <lenronfu@gmail.com>
…#1680) Signed-off-by: rongfu.leng <lenronfu@gmail.com>
…#1680) Signed-off-by: rongfu.leng <lenronfu@gmail.com>
…#1680) Signed-off-by: rongfu.leng <lenronfu@gmail.com>
…#1680) Signed-off-by: rongfu.leng <lenronfu@gmail.com>
…#1680) Signed-off-by: rongfu.leng <lenronfu@gmail.com>
…#1680) Signed-off-by: rongfu.leng <lenronfu@gmail.com>
PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED.
Purpose
Fixes: #1660
Wan2.2 video pipelines return numpy arrays (output_type="np"), but image_api_utils.py assumes PIL Images and calls img.save().
We
_extract_images_from_resultthis method to add a numpy-to-PIL conversion.Serve:
Test:
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model. Please runmkdocs serveto sync the documentation editions to./docs.BEFORE SUBMITTING, PLEASE READ https://github.com/vllm-project/vllm-omni/blob/main/CONTRIBUTING.md (anything written below this line will be removed by GitHub Actions)