Repository navigation
[Misc] treewide: remove shm_threshold_bytes from deploy config - #7522
Conversation
|
This PR appears to belong to: docs/design/module/vllm_omni_config.md, docs/design/module/entrypoints.md. Module owners: @alex-jw-brooks @lishunyang12 @linyueqian Routing: @alex-jw-brooks via module of the changed files, semantic router, CODEOWNERS; @lishunyang12 via module of the changed files, semantic router, CODEOWNERS; @linyueqian via module of the changed files @NickCao, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Omni ReviewBot routing recordAssigned Strict under experiment |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
Scan:
| Category | Result |
|---|---|
| Tests / verification | 1 finding(s) below |
| Security | no finding reported |
| Docs / comments | 1 finding(s) below |
| Behavior / compatibility | 2 finding(s) below |
| Correctness | 1 finding(s) below |
Validated:
- [resolved] Concern 'YAML still gates small payloads to non-SHM': connector never branched on threshold since #5096; residual is CLI/examples only, not deploy
- [resolved] Concern 'sibling deploy YAMLs still pin 65536': at this head zero deploy files retain the key
- [resolved] Concern that docs still document the threshold: stage_configs.md:120 and shared_memory_connector.md:164-167 already describe always-SHM; residual is CLI/examples only.
- [claim-verified] SharedMemoryConnector no longer reads shm_threshold_bytes: shm_connector.py put/get always serialize via shm_write_bytes/shm_read_bytes; init only uses stage_id (vllm_omni/distributed/omni_connectors/connectors/shm_connector.py:27-61).
- [validated] Blast radius of YAML removal: audex_s2s(_30b), audex_tta(_30b), audex_tts(_30b), minimax_music3(_2gpu), nemotron_labs_voicechat(_duplex|streaming), personaplex — remaining extra keys (codec_streaming, connector_get*, codec_chunk_frames, etc.) intact; voicechat.yaml correctly drops empty extra:.
- [validated] personaplex.yaml:22-34 same pattern; remaining extras are chunk-adapter knobs, not SHM size thresholds.
Merged outcome keeps one minor finding: after removing inert shm_threshold_bytes from all 12 deploy YAMLs, the public --shm-threshold-bytes serve CLI flag and matching offline example/README surfaces remain and are silently dropped, contradicting the PR’s treewide/Python-unused claim—remove them here or explicitly track follow-up. All connector/docs/YAML structural validations and tombstone/sibling-residue candidates are dropped as non-issues or excluded polish.
Checked, no defect found:
vllm_omni/deploy/audex_s2s.yaml:16— After deleting shm_threshold_bytes, audex_s2s SharedMemoryConnector.extra is coherent (codec_streaming + wait/codec knobs only); SharedMemoryConnector.put has no size/threshold gate.vllm_omni/deploy/audex_s2s_30b.yaml:11— 30B s2s connector.extra matches 2B after shm_threshold_bytes removal (both start at codec_streaming: true; threshold absent).vllm_omni/deploy/audex_tta.yaml:23— After deleting shm_threshold_bytes, audex_tta.yaml SharedMemoryConnector.extra still keeps connector_get_* waits and audiocodec_*; audex_tta_30b.yaml mirrors the same keys.vllm_omni/deploy/audex_tts.yaml:28— audex_tts.yaml and audex_tts_30b.yaml only drop shm_threshold_bytes; codec_streaming and codec_* extras remain (connector_get_* waits also retained).vllm_omni/deploy/minimax_music3.yaml:24— minimax_music3 and minimax_music3_2gpu keep window/wait commentary after codec_streaming; only shm_threshold_bytes was removed.vllm_omni/deploy/nemotron_labs_voicechat.yaml:27— Removing the sole extra key correctly yields a name-only SharedMemoryConnector; same shape as bagel/hunyuan/dynin deploy siblings.
Verdict: COMMENT
Findings
- [P2] This diff deletes
shm_threshold_bytes: 65536from 12 deploy YAMLs because Sha… —vllm_omni/entrypoints/cli/serve.py
This diff deletesshm_threshold_bytes: 65536from 12 deploy YAMLs because SharedMemoryConnector no longer honors a size gate (putalwaysserialize_obj→shm_write_bytes). The same dead knob remains as public--shm-threshold-bytesinvllm_omni/entrypoints/cli/serve.py:396-399(default 65536; help still describes an SHM size threshold) and in eight offlineend2end.pyparsers plusexamples/offline_inference/covo_audio/README.md:50.OrchestratorArgshas no such field (unchanged by this diff); underscore-formshm_threshold_bytesis absent treewide, so the hyphen CLI flag cannot affect the connector. Please remove the serve CLI arg and the offline/example mirrors in this PR, or explicitly narrow the treewide claim and track follow-up—otherwise any Test Plan assertion that the key is not referenced in Python is false and the most user-visible dead surface survives.
Evidence: unchanged by this diff, present in the PR-time tree: serve.py:396-399 "--shm-threshold-bytes", / type=int, / default=65536, / help="The threshold for the shared memory size.",; shm_connector.py:45-51 payload = self.serialize_obj(data) then meta = shm_write_bytes(payload, name=put_key) with no size-threshold branch; OrchestratorArgs (arg_utils.py:490-610) defines cross-stage fields through tokenizer with no shm_threshold_bytes; tree grep: zero shm_threshold_bytes literals; hyphen-form remains in serve.py + examples/offline_inference/{qwen3_omni,qwen2_5_omni,qwen3_tts,ming_tts,minicpmo,step_audio2,mimo_audio,covo_audio}/end2end.py and covo_audio/README.md:50 | --shm-threshold-bytes| |65536 | Shared memory threshold (bytes) |.
- [P2] Confirmed: SharedMemoryConnector does not branch on size —
put()always SHM-w… — ``
Confirmed: SharedMemoryConnector does not branch on size —put()always SHM-writes (shm_connector.py:45-55). Residual is dead user-facing advertising only: `--shm-threshold-bytes` in serve.py:396-399 (and offline end2end parsers); no `shm_threshold_bytes` consumer remains in vllm_omni (unchanged by this deploy-YAML cleanup diff).
Evidence: unchanged by this diff, present in the PR-time tree: shm_connector.py:45-55 payload = self.serialize_obj(data) / size = len(payload) / meta = shm_write_bytes(payload, name=put_key) / metadata = {"shm": meta, "size": size} — no threshold compare before SHM write; serve.py:396-399 --shm-threshold-bytes / default=65536 / help="The threshold for the shared memory size." — CLI advertising only; repo-wide shm_threshold_bytes identifier grep empty (no transport consumer).
- [P2] Resolved: deploy YAML shm_threshold_bytes never gated transport—SharedMemoryCon… — ``
Resolved: deploy YAML shm_threshold_bytes never gated transport—SharedMemoryConnector always SHM-writes (shm_connector.py put path). Residual --shm-threshold-bytes is CLI/examples-only and unconsumed; not deploy.
Evidence: unchanged by this diff, present in the PR-time tree: vllm_omni/distributed/omni_connectors/connectors/shm_connector.py:45-51 payload = self.serialize_obj(data) / size = len(payload) / … / meta = shm_write_bytes(payload, name=put_key) — no threshold branch, always SHM. Residual CLI only: vllm_omni/entrypoints/cli/serve.py:396-399 --shm-threshold-bytes / default=65536 / help="The threshold for the shared memory size." — argparse definition with no vllm_omni consumer of shm_threshold_bytes. Deploy: repo-wide grep for shm_threshold_bytes is empty after this PR's YAML removals.
- [P2] Resolved: at this head, zero files retain
shm_threshold_bytes(repo-wide grep… — ``
Resolved: at this head, zero files retainshm_threshold_bytes(repo-wide grep empty); sibling deploy YAMLs do not still pin 65536 for that key.
Evidence: PR-time tree grep shm_threshold_bytes / shm_threshold → 0 matches (unchanged absence outside the deletion diff). Representative post-removal connectors: vllm_omni/deploy/audex_s2s.yaml:16-18 extra: / codec_streaming: true / connector_get_sleep_s: 0.01 — no shm_threshold_bytes; vllm_omni/deploy/nemotron_labs_voicechat.yaml:26-29 connector_of_shared_memory: / name: SharedMemoryConnector then blank line then stages: — entire extra: (incl. former shm_threshold_bytes) removed.
- **[P2] Resolved: connector docs already describe always-SHM (stage_configs.md:120
None** —stage_configs.md:120Resolved: connector docs already describe always-SHM (stage_configs.md:120None. All payloads use shared memory.; shared_memory_connector.md:164-167 no inline path/size threshold). No in-tree shm_threshold_bytes keys; residual is CLI/examples--shm-threshold-bytes` (e.g. serve.py) plus a stale dynin_omni_multiconnector.yaml:31 comment.
Evidence: unchanged by this diff, present in the PR-time tree: docs/configuration/stage_configs.md:120 | SharedMemoryConnector | Same-host KV transfer between stages (default for bundled YAMLs). | None. All payloads use shared memory. |; docs/design/feature/omni_connectors/shared_memory_connector.md:164-167 #### 6.1 All Payloads Use Shared Memory / put() writes every serialized payload to shared memory. The connector has no / inline-payload path or size threshold.; residual outside connector docs: vllm_omni/entrypoints/cli/serve.py:396-399 --shm-threshold-bytes / help="The threshold for the shared memory size."
|
I added |
23d8700 to
57b2402
Compare
|
Rebased, check that no shm_threshold_bytes reference is left. |
linyueqian
left a comment
There was a problem hiding this comment.
Approving at 23d87005. This removes the shm_threshold_bytes: 65536 line from twelve deploy profiles (and the extra: block it was the only child of, in one of them). I checked the claim rather than the description: since #5096 nothing under vllm_omni/distributed or anywhere else in the package reads shm_threshold_bytes, so the key was inert configuration that would only mislead the next person tuning a connector, and after this PR the string no longer appears anywhere in the tree. Pure YAML removal, no behaviour change. Validation: static read of the diff and a tree-wide search on the PR head; no PR code executed.
Rebased head 57b24021 carries the same twelve-line, config-only removal as the reviewed 23d87005 (patches are byte-identical) and now sits on current main with zero commits behind, so the earlier read stands. Approving once the general lane is green on this head, then merging.
|
Checked as asked: no |
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>
57b2402 to
7f0fea1
Compare
…project#7522) Signed-off-by: Nick Cao <ncao@redhat.com> Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Matthieu Laneuville <matthieu.laneuville@surf.nl>
…project#7522) Signed-off-by: Nick Cao <ncao@redhat.com> Co-authored-by: Codex <noreply@openai.com>
Purpose
Since #5096, SharedMemoryConnector no longer reads shm_threshold_bytes, all payloads use shared memory.
Test Plan
vLLM Version: 0.29.0
vLLM-Omni Commit: 23d8700
N/A, only removal of dead code, this key is not referenced in any python code.
Test Result
N/A
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)