Repository navigation
[Core] Cleanup Omni connector runtime and transfer backends - #5096
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
| vllm_config: Any, | ||
| model_config: Any, | ||
| kv_transfer_manager: OmniKVTransferManager | None = None, | ||
| **_ignored_kwargs: Any, |
There was a problem hiding this comment.
Both callers dropped vllm_config, so **_ignored_kwargs now silently swallows typo'd kwargs. Drop it so bad calls fail loudly.
There was a problem hiding this comment.
do not use 'any' type
|
|
||
| sync_ms = self._sync_receive_buffer_if_needed() | ||
| except Exception as e: | ||
| self._metrics["timeouts"] += 1 |
There was a problem hiding this comment.
This except also wraps _sync_receive_buffer_if_needed(), so a CUDA sync failure gets miscounted as a timeout and logged as RDMA Get error. Bump errors for the non-transfer path instead.
| return self._get_by_key(get_key) | ||
| result = self._get_by_key(get_key) | ||
| if result is not None: | ||
| self._metrics["gets"] += 1 |
There was a problem hiding this comment.
gets is bumped in 4 separate branches, easy to drift. Increment it once at a single exit instead.
hsliuustc0106
left a comment
There was a problem hiding this comment.
avoid using any type hint for input args
| vllm_config: Any, | ||
| model_config: Any, | ||
| kv_transfer_manager: OmniKVTransferManager | None = None, | ||
| **_ignored_kwargs: Any, |
There was a problem hiding this comment.
do not use 'any' type
|
|
||
| _t2 = _time_mod.perf_counter() | ||
| _alloc_ms = (_t2 - _t1) * 1000 | ||
| alloc_ms = (_t2 - _t1) * 1000 |
There was a problem hiding this comment.
shall we add a logger level for time profiling? cc @ZacheryAU
| zmq_addr = f"tcp://{src_host}:{src_port}" | ||
| req_socket = self._get_req_socket(zmq_addr, timeout_ms=_total_timeout_ms) | ||
|
|
||
| base_timeout_ms = 30000 |
There was a problem hiding this comment.
avoid magic number inside funtions
There was a problem hiding this comment.
Pull request overview
This PR cleans up the Omni connector runtime APIs and connector backends by removing legacy/unused compatibility paths, refactoring the Mooncake transfer connector into smaller helpers, and simplifying the SHM connector to a single key-addressed SHM mechanism. It also updates the affected GPU model runners and connector tests to match the new behavior.
Changes:
- Simplify
OmniConnectorModelRunnerMixininitialization/signatures and remove legacy wrapper helpers. - Simplify
SharedMemoryConnectorby removing inline-payload/threshold logic and related metrics. - Refactor
MooncakeTransferEngineConnector.get()into helper methods, add configurable buffer TTL, and prevent TTL cleanup of in-flight buffers.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vllm_omni/worker/omni_connector_model_runner_mixin.py | Removes deprecated wrappers/helpers and updates connector initialization API. |
| vllm_omni/worker/gpu_generation_model_runner.py | Updates connector initialization callsite to match new mixin signature. |
| vllm_omni/worker/gpu_ar_model_runner.py | Updates connector initialization callsite to match new mixin signature (incl. KV manager passing). |
| vllm_omni/distributed/omni_connectors/connectors/shm_connector.py | Retires inline/threshold paths; always uses key-addressed SHM metadata. |
| vllm_omni/distributed/omni_connectors/connectors/mooncake_transfer_engine_connector.py | Refactors init + get/recv flow; adds in-flight tracking and configurable TTL cleanup. |
| tests/distributed/omni_connectors/test_basic_connectors.py | Updates SHM connector tests to reflect key-addressed SHM-only behavior and ensures cleanup via close(). |
| tests/distributed/omni_connectors/test_adapter_and_flow.py | Updates flow test configuration/comments to reflect SHM metadata behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def init_omni_connectors( | ||
| self, | ||
| vllm_config: Any, | ||
| model_config: Any, | ||
| kv_transfer_manager: OmniKVTransferManager | None = None, | ||
| **_ignored_kwargs: Any, | ||
| ) -> None: |
| self._metrics["gets"] += 1 | ||
| return result | ||
|
|
||
| if "shm" in metadata: |
There was a problem hiding this comment.
Please fix _get_data_with_lock before routing every payload through this path. Its finally block uses if obj, so deserializing a multi-element tensor raises RuntimeError: Boolean value of Tensor with more than one value is ambiguous before get() returns; falsey payloads also leave the lock file behind. I reproduced this with torch.ones(2, 2). Track successful deserialization explicitly and clean up independently of payload truthiness.
| fcntl.flock(lockf, fcntl.LOCK_UN) | ||
|
|
||
| # meta contains {'name': ..., 'size': ...} | ||
| metadata = {"shm": meta, "size": size} |
There was a problem hiding this comment.
This now makes every payload use SHM, but shm_threshold_bytes is still exposed in the CLI/config and forwarded by connector initialization. Those existing settings are now silently ignored. Please remove/deprecate the stale plumbing or preserve the documented threshold behavior.
88d4c96 to
1b47e9f
Compare
| # === Cross-stage Communication === | ||
| shm_threshold_bytes: int = 65536 | ||
| # Deprecated compatibility flag; SharedMemoryConnector always uses SHM. | ||
| shm_threshold_bytes: int | None = None |
There was a problem hiding this comment.
I think it's fine to remove it (and other deprecated warning) directly instead of keeping compatible. Sometimes we forget to remove it in next version, then it will be the new dead code. And this field doesn't seem to be used frequently by users.
1b47e9f to
3434bd3
Compare
Signed-off-by: spencerr221 <liubingyu62@gmail.com>
Signed-off-by: spencerr221 <liubingyu62@gmail.com>
Signed-off-by: spencerr221 <liubingyu62@gmail.com>
Signed-off-by: spencerr221 <liubingyu62@gmail.com>
fe66ff7 to
96c1354
Compare
…ject#5096) Signed-off-by: spencerr221 <liubingyu62@gmail.com>
…ject#5096) Signed-off-by: spencerr221 <liubingyu62@gmail.com>
Since vllm-project#5096, SharedMemoryConnector no longer reads shm_threshold_bytes, all payloads use shared memory. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Nick Cao <ncao@redhat.com>
Since vllm-project#5096, SharedMemoryConnector no longer reads shm_threshold_bytes, all payloads use shared memory. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Nick Cao <ncao@redhat.com>
…ject#5096) Signed-off-by: spencerr221 <liubingyu62@gmail.com>
Purpose
SharedMemoryConnectorby removing unused device, threshold, inline-payload paths, and related metrics.MooncakeTransferEngineConnectorinitialization and receive flow into focused helpers.vllm_configargument fromOmniConnectorModelRunnerMixin.Test Plan
vLLM Version: 0.25.0
vLLM-Omni Commit: 88d4c96
Test Result
17 passed.
BEFORE SUBMITTING: read CONTRIBUTING.md and run the precheck-pr skill with the code agent for a self-check against project conventions.
(anything written below this line will be removed by GitHub Actions)