Fix handling of encode_video output in vllm.py so each frame’s Base64#753
Fix handling of encode_video output in vllm.py so each frame’s Base64#753LiamLian0727 wants to merge 2 commits into
Conversation
WalkthroughThe Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lmms_eval/models/vllm.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
lmms_eval/models/vllm.py (1)
lmms_eval/models/qwen2_5_vl.py (1)
flatten(173-178)
🔇 Additional comments (2)
lmms_eval/models/vllm.py (2)
150-157: Approve the improved flatten logic.The updated
flattenmethod correctly handles mixed content (both iterable and non-iterable elements), which addresses the core issue described in the PR objectives. The original implementation assumed all elements were iterable, which would fail when processing single base64 strings alongside lists of base64 strings fromencode_video().
208-208: Approve the fix for handling encode_video output.The change to use
self.flatten(imgs)correctly addresses the bug whereencode_video()returns a list of Base64-encoded frames but the calling code treated it as a single string. This ensures all base64 strings are properly flattened before creating the image URLs, preventing invalid URL formats likedata:image/png;base64,['AAA','BBB',...].
| if isinstance(i, (list, tuple)): | ||
| new_list.extend(i) | ||
| else: | ||
| new_list.append(j) |
There was a problem hiding this comment.
Critical bug: Fix undefined variable reference.
The flatten method has a critical bug on line 156 where it references j but j is not defined in the current scope. This will cause a NameError at runtime.
Apply this diff to fix the undefined variable:
def flatten(self, input):
new_list = []
for i in input:
if isinstance(i, (list, tuple)):
new_list.extend(i)
else:
- new_list.append(j)
+ new_list.append(i)
return new_list📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isinstance(i, (list, tuple)): | |
| new_list.extend(i) | |
| else: | |
| new_list.append(j) | |
| def flatten(self, input): | |
| new_list = [] | |
| for i in input: | |
| if isinstance(i, (list, tuple)): | |
| new_list.extend(i) | |
| else: | |
| new_list.append(i) | |
| return new_list |
🧰 Tools
🪛 Ruff (0.11.9)
156-156: Undefined name j
(F821)
🤖 Prompt for AI Agents
In lmms_eval/models/vllm.py around lines 153 to 156, the variable `j` is used
but not defined, causing a NameError. Replace the reference to `j` with `i` in
the else clause to correctly append the current element being iterated over.
1 participant
Description:
This PR addresses a bug in the VLLM model integration whereby encode_video() returns a List[str] of Base64‐encoded frames, but the caller treats it as a single string. As a result, the model receives invalid URLs like data:image/png;base64,['AAA','BBB',…].
Summary by CodeRabbit