test: add large size image transfer over tcp #4483
Conversation
biswapanda
changed the title
WalkthroughA new optional 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
🧹 Nitpick comments (1)
examples/backends/vllm/launch/agg_multimodal_epd.sh (1)
72-76: Consider simplifying the conditional check.Since
REQUEST_PLANEhas a default value of"nats"(line 22), the condition[[ -n "$REQUEST_PLANE" ]]will always evaluate to true. You can simplify this by removing the conditional wrapper.Apply this diff to simplify:
- -if [[ -n "$REQUEST_PLANE" ]]; then - export DYN_REQUEST_PLANE="$REQUEST_PLANE" - echo "Using request plane: $REQUEST_PLANE" -fi - +export DYN_REQUEST_PLANE="$REQUEST_PLANE" +echo "Using request plane: $REQUEST_PLANE" +
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/backends/vllm/launch/agg_multimodal_epd.sh(3 hunks)tests/serve/test_vllm.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: oandreeva-nv
Repo: ai-dynamo/dynamo PR: 2989
File: lib/llm/src/block_manager/distributed/transfer.rs:6-6
Timestamp: 2025-09-18T21:47:44.143Z
Learning: For PR ai-dynamo/dynamo#2989, the ConnectorTransferBatcher architectural issues will be addressed in a follow-up PR by removing the duplicate batching logic and integrating distributed transfers with the existing TransferBatcher + LocalTransferManager pipeline, rather than adding bounded concurrency primitives like Semaphore.
🧬 Code graph analysis (1)
tests/serve/test_vllm.py (1)
tests/utils/payload_builder.py (1)
chat_payload(129-156)
⏰ 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). (6)
- GitHub Check: vllm (arm64)
- GitHub Check: operator (arm64)
- GitHub Check: sglang (arm64)
- GitHub Check: operator (amd64)
- GitHub Check: trtllm (arm64)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (1)
examples/backends/vllm/launch/agg_multimodal_epd.sh (1)
22-22: LGTM! Request plane argument handling is correctly implemented.The
--request-planeargument parsing, default value, and help documentation are properly implemented and follow the existing patterns in the script.Also applies to: 35-38, 44-44
| "multimodal_agg_qwen_epd_30mb_img": VLLMConfig( | ||
| name="multimodal_agg_qwen_epd_30mb_img", | ||
| directory=vllm_dir, | ||
| script_name="agg_multimodal_epd.sh", | ||
| marks=[pytest.mark.gpu_2], | ||
| model="Qwen/Qwen2.5-VL-7B-Instruct", | ||
| delayed_start=0, | ||
| script_args=[ | ||
| "--model", | ||
| "Qwen/Qwen2.5-VL-7B-Instruct", | ||
| "--request-plane", | ||
| "tcp", | ||
| ], | ||
| timeout=360, | ||
| request_payloads=[ | ||
| chat_payload( | ||
| [ | ||
| {"type": "text", "text": "What is in this image?"}, | ||
| { | ||
| "type": "image_url", | ||
| "image_url": { | ||
| "url": "https://floatingworld.com/wp-content/uploads/2023/02/Sample-jpg-image-30mb-16.jpg" | ||
| }, | ||
| }, | ||
| ], | ||
| repeat_count=1, | ||
| expected_response=["bus"], | ||
| ) | ||
| ], | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the expected response matches the actual image content.
The test expects a response containing "bus", but the 30MB image URL (https://floatingworld.com/wp-content/uploads/2023/02/Sample-jpg-image-30mb-16.jpg) appears to be a generic sample image from floatingworld.com, not the COCO dataset bus image used in the reference test configuration multimodal_agg_qwen_epd (line 171). This mismatch could cause the test to fail.
Please verify the image content and update the expected response accordingly. You can check the image content with:
Additionally, please search the web to confirm whether the floatingworld.com sample image contains a bus:
🏁 Script executed:
#!/bin/bash
# Description: Verify the image URL is accessible and examine its metadata
# Expected: HTTP 200 response and image metadata
echo "Checking image URL accessibility..."
curl -I "https://floatingworld.com/wp-content/uploads/2023/02/Sample-jpg-image-30mb-16.jpg" 2>&1 | head -n 10
echo -e "\n\nChecking reference COCO image URL..."
curl -I "http://images.cocodataset.org/test2017/000000155781.jpg" 2>&1 | head -n 10Length of output: 1218
🌐 Web query:
What is shown in the image at https://floatingworld.com/wp-content/uploads/2023/02/Sample-jpg-image-30mb-16.jpg?
💡 Result:
The image is a colorful nebula/space-style test picture (a bright, cloud‑like “nebula” texture used as a large sample JPG). [1][2]
Sources:
[1] Floating World Gallery — Sample-jpg-image-30mb 16.
[2] Example collection of large sample/nebula test images (CSDN).
Update the expected response to match the actual nebula image content or replace the image with one containing a bus.
The verification confirms the major issue: the image at the provided URL is a colorful nebula/space-style test texture, not a bus image. The test expects ["bus"] as the response, but this will fail when the model analyzes the actual nebula image. Either update expected_response to reflect what's in the nebula image, or replace the image URL with one that actually contains a bus (such as the COCO reference image used in the multimodal_agg_qwen_epd configuration).
🤖 Prompt for AI Agents
In tests/serve/test_vllm.py around lines 180 to 209, the test uses a
nebula/space texture image but asserts expected_response=["bus"]; update the
test so the expected output matches the actual nebula image content OR replace
the image URL with a valid bus image (for example reuse the COCO bus reference
image from multimodal_agg_qwen_epd config), and ensure
repeat_count/expected_response pair is consistent with the chosen image so the
model's response assertion will pass.
| { | ||
| "type": "image_url", | ||
| "image_url": { | ||
| "url": "https://floatingworld.com/wp-content/uploads/2023/02/Sample-jpg-image-30mb-16.jpg" |
There was a problem hiding this comment.
@krishung5 - want to double check - will this be decoded in the frontend and sent over nixl or over the request plane?
There was a problem hiding this comment.
In EPD, the image will be loaded in the encode worker: https://github.com/ai-dynamo/dynamo/blob/main/components/src/dynamo/vllm/multimodal_handlers/encode_worker_handler.py#L50
it's not decoded in the frontend. I think this PR is going to add that: #3988
Overview:
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.