feat: add TQMediaConnector plugin for TransferQueue image data support - #38355
feat: add TQMediaConnector plugin for TransferQueue image data support#38355RobotGF wants to merge 10 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new TQMediaConnector to support fetching media from TransferQueue using the tq:// URL scheme. The implementation includes a lazy-initialized client and logic to handle both synchronous and asynchronous loading. Feedback highlights security concerns regarding the use of pickle for deserializing environment variables, potential performance overhead from creating new event loops in the synchronous path, and unnecessary serialization/deserialization overhead when converting images to PNG bytes before passing them to media_io.
|
[verl] related PR: verl-project/verl#5780 |
Register a "tq" MediaConnector into vLLM's MEDIA_CONNECTOR_REGISTRY that supports tq://<partition_id>/<global_index> URL scheme. This enables vLLM to fetch multi-modal image data directly from TransferQueue storage, avoiding serialising large PIL Images over Ray RPC. - New file: vllm/multimodal/media/tq_connector.py - TQ client lazily initialised from env vars (VERL_TQ_CONTROLLER_INFO) - Falls back to parent MediaConnector for http/data/file URLs - Optional dependency: gracefully skipped when transfer_queue not installed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
44e96d7 to
c9edcd0
Compare
…review issues Sync vLLM's TQMediaConnector with verl's updated tq_multimodal.py which switched from per-image TQ entries to batched storage (pixel_flat/shapes/ offsets fields under a single batch_key). Update URL format from tq://<partition>/<global_index> to tq://<partition>/<batch_key>/<index>. Also fix review issues: - Use existing event loop in _load_from_tq_sync to avoid conflicts - Add .detach().cpu() before .numpy() for GPU tensor safety - Add try/except around storage manager init with proper logging - Replace SimpleNamespace with frozen dataclass for TQ config - Add field validation and index bounds checking - Move stdlib imports (base64, pickle, BytesIO) to file top level - Conditionally export TQMediaConnector in __all__ only when available Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t tests Address security review: pickle.loads on env vars risks arbitrary code execution. Use JSON as the primary deserialization format, with a pickle fallback (plus warning log) for backward compatibility with older verl versions. Also add comprehensive unit tests for TQMediaConnector covering: - URL parsing (valid, invalid, old 2-segment format) - Tensor-to-numpy conversion (CPU, grad, plain ndarray) - Async/sync image loading with mocked TQ client - Batched image storage round-trip (RGB, RGBA, grayscale) - Index out-of-range and missing field error handling - URL routing (tq:// vs data:/http: fallback) - Client init caching and failure recovery - JSON-first / pickle-fallback deserialization Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
P0 - Eliminate PNG encode/decode round-trip: For ImageMediaIO (the common case), construct MediaWithBytes directly from Image.fromarray + cheap BMP bytes instead of the costly PNG compress → PNG decompress cycle. Saves ~100-250ms per image. Pipeline: TQ slice → numpy → Image.fromarray → mode convert → BMP. Total: 2 memcpy (down from 4+ with PNG). P1 - Add per-instance batch cache: Multiple tq:// URLs sharing the same batch_key (typical for a single multi-image request) now trigger only one TQ round-trip instead of N. Cache is naturally request-scoped since vLLM creates one connector per request. P2 - Fix sync path deadlock risk: Replace run_coroutine_threadsafe (deadlocks when called from the same thread as the running loop) with a dedicated daemon thread that creates its own event loop. Also: simplify _parse_tq_url to use string split instead of full urllib3 RFC 3986 parsing; extract _extract_image_from_batch helper; add tests for batch cache and sync-from-async-context scenarios. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tighten the invalid base64 assertion and apply Ruff formatting so the TQ connector tests pass the repo's pre-commit checks. Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: RobotGF <1083760447@qq.com>
6d9bfee to
b66f30f
Compare
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def _init_tq_client(): |
There was a problem hiding this comment.
The initialization of TQ is much easier now: Ascend/TransferQueue#26. Is it possible to switch to the new API? Which can also be helpful to reduce the dependency of these env vars:
_ENV_TQ_CONTROLLER_INFO = "VERL_TQ_CONTROLLER_INFO"
_ENV_TQ_STORAGE_UNIT_INFOS = "VERL_TQ_STORAGE_UNIT_INFOS"
_ENV_TQ_STORAGE_BACKEND = "VERL_TQ_STORAGE_BACKEND"
| client = _init_tq_client() | ||
| metadata = await client.async_kv_retrieve_meta( | ||
| keys=[batch_key], | ||
| partition_id=partition_id, | ||
| create=False, | ||
| ) | ||
| td = await client.async_get_data(metadata) |
There was a problem hiding this comment.
We can directly using the high-level KV API instead, which encapsulates these processes
There was a problem hiding this comment.
BTW, what is the actual data format during tq.put/get? We have implemented zerocopy serialization for tensor and numpy array.
There was a problem hiding this comment.
BTW, what is the actual data format during tq.put/get? We have implemented zerocopy serialization for tensor and numpy array.
|
This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
This pull request has been automatically closed due to inactivity. Please feel free to reopen if you intend to continue working on it. Thank you! |
2 participants
Register a "tq" MediaConnector into vLLM's MEDIA_CONNECTOR_REGISTRY that supports tq://<partition_id>/<global_index> URL scheme. This enables vLLM to fetch multi-modal image data directly from TransferQueue storage, avoiding serialising large PIL Images over Ray RPC.
Purpose
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.