Conversation
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@ajc/image-batchRecommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@ajc/image-batch |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
WalkthroughTwo new test functions were added: one unit test validating that multiple image URLs are correctly formatted in a ChatEndpoint payload, and one integration test verifying image batch sizing behavior and payload structure validation. No existing logic was modified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
![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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/endpoints/test_openai_chat_completions.py(1 hunks)tests/integration/test_multimodal.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T03:16:02.685Z
Learnt from: ajcasagrande
Repo: ai-dynamo/aiperf PR: 389
File: src/aiperf/endpoints/openai_chat.py:41-46
Timestamp: 2025-10-23T03:16:02.685Z
Learning: In the aiperf project, the ChatEndpoint at src/aiperf/endpoints/openai_chat.py supports video inputs (supports_videos=True) through custom extensions, even though the standard OpenAI /v1/chat/completions API does not natively support raw video inputs.
Applied to files:
tests/endpoints/test_openai_chat_completions.py
🧬 Code graph analysis (2)
tests/endpoints/test_openai_chat_completions.py (3)
tests/conftest.py (1)
sample_conversations(317-347)src/aiperf/endpoints/openai_chat.py (2)
ChatEndpoint(27-217)format_payload(48-77)src/aiperf/common/models/record_models.py (1)
RequestInfo(755-808)
tests/integration/test_multimodal.py (3)
tests/integration/conftest.py (3)
AIPerfCLI(47-118)aiperf_mock_server(159-236)run(56-90)tests/integration/models.py (3)
AIPerfMockServer(30-41)request_count(159-163)has_input_images(237-239)tests/aiperf_mock_server/models.py (1)
inputs(81-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build (macos-latest, 3.12)
- GitHub Check: build (macos-latest, 3.11)
- GitHub Check: integration-tests (macos-latest, 3.12)
- GitHub Check: build (macos-latest, 3.10)
- GitHub Check: build (ubuntu-latest, 3.10)
- GitHub Check: integration-tests (macos-latest, 3.11)
- GitHub Check: integration-tests (ubuntu-latest, 3.11)
- GitHub Check: integration-tests (ubuntu-latest, 3.10)
- GitHub Check: integration-tests (macos-latest, 3.10)
- GitHub Check: integration-tests (ubuntu-latest, 3.12)
🔇 Additional comments (1)
tests/endpoints/test_openai_chat_completions.py (1)
195-243: LGTM! Well-structured unit test for image batching.The test correctly validates that multiple image URLs within a single
Imageobject'scontentslist are properly formatted as separateimage_urlentries in the payload. The test structure is clear, and the assertions accurately verify the expected OpenAI Chat Completions format.
| for message in messages: | ||
| content = message.get("content", []) | ||
| if isinstance(content, list): | ||
| # Count image_url entries in the content array | ||
| image_count = sum( | ||
| 1 | ||
| for item in content | ||
| if isinstance(item, dict) | ||
| and item.get("type") == "image_url" | ||
| ) | ||
| # Each turn should have exactly batch_size images | ||
| assert image_count == batch_size, ( | ||
| f"Expected {batch_size} images per turn, got {image_count}" | ||
| ) |
There was a problem hiding this comment.
Only assert image count for messages that contain images.
The current logic asserts image_count == batch_size for every message where content is a list. However, not all messages necessarily contain images (e.g., assistant responses or messages with only audio/video). This could cause false failures.
Apply this diff to check only messages with images:
for message in messages:
content = message.get("content", [])
if isinstance(content, list):
# Count image_url entries in the content array
image_count = sum(
1
for item in content
if isinstance(item, dict)
and item.get("type") == "image_url"
)
- # Each turn should have exactly batch_size images
- assert image_count == batch_size, (
- f"Expected {batch_size} images per turn, got {image_count}"
- )
+ # Each turn with images should have exactly batch_size images
+ if image_count > 0:
+ assert image_count == batch_size, (
+ f"Expected {batch_size} images per turn, got {image_count}"
+ )📝 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.
| for message in messages: | |
| content = message.get("content", []) | |
| if isinstance(content, list): | |
| # Count image_url entries in the content array | |
| image_count = sum( | |
| 1 | |
| for item in content | |
| if isinstance(item, dict) | |
| and item.get("type") == "image_url" | |
| ) | |
| # Each turn should have exactly batch_size images | |
| assert image_count == batch_size, ( | |
| f"Expected {batch_size} images per turn, got {image_count}" | |
| ) | |
| for message in messages: | |
| content = message.get("content", []) | |
| if isinstance(content, list): | |
| # Count image_url entries in the content array | |
| image_count = sum( | |
| 1 | |
| for item in content | |
| if isinstance(item, dict) | |
| and item.get("type") == "image_url" | |
| ) | |
| # Each turn with images should have exactly batch_size images | |
| if image_count > 0: | |
| assert image_count == batch_size, ( | |
| f"Expected {batch_size} images per turn, got {image_count}" | |
| ) |
🤖 Prompt for AI Agents
In tests/integration/test_multimodal.py around lines 111 to 124, the test
currently asserts image_count == batch_size for every message whose content is a
list, which fails for turns that don't contain images; change the logic to only
perform the equality assertion when the message actually contains image entries
(e.g., check if any item in content has type "image_url" or simply if
image_count > 0) and skip the assertion for messages with zero image entries so
only image-containing messages are validated against batch_size.
1 participant
Summary by CodeRabbit