Skip to content

feat(multimodal): vLLM SHM tensor transport - #1893

Merged
slin1237 merged 2 commits into
mainfrom
feat/vllm-shm-video
Jul 9, 2026
Merged

feat(multimodal): vLLM SHM tensor transport#1893
slin1237 merged 2 commits into
mainfrom
feat/vllm-shm-video

Conversation

@slin1237

@slin1237 slin1237 commented Jul 8, 2026

Copy link
Copy Markdown
Member

Description

Problem

The /dev/shm tensor transport (added for TokenSpeed) and the engine-neutral
ShmHandle (hoisted to common.proto) + configurable transport layer are all
in place, but vLLM still only accepts multimodal tensors inline in the gRPC
message. Large image tensors to a co-located vLLM worker pay a full gRPC copy.

Solution

Bring the same-host SHM transport to vLLM, reusing the common ShmHandle and
the --multimodal-tensor-transport config. Default is inline, so existing
deployments are unaffected.

Changes

Proto (vllm_engine.proto, already imports common.proto):

  • TensorData: bytes data = 1oneof payload { bytes inline = 1; smg.grpc.common.ShmHandle shm = 4; smg.grpc.common.RemoteTensorHandle remote = 5; }. Field 1 stays inline for wire compatibility.
  • GetServerInfoResponse: + shm_namespace_id = 10 (so the router can verify a
    shared /dev/shm under auto).

Gateway (Rust):

  • Extracted an engine-neutral resolve_mm_tensor_payload (the inline-vs-SHM
    decision + metrics); both tokenspeed_tensor_payload and the new
    vllm_tensor_payload map its result onto their own oneof.
  • VllmMultimodalData carries resolved shm_enabled/shm_min_bytes;
    assemble_vllm resolves them (transport config → worker override); into_proto
    emits the oneof payload.
  • collect_vllm_*_shm_handles + the vLLM client cleans up SHM on send failure
    (mirrors TokenSpeed). Renamed the engine-neutral cleanup_tokenspeed_shm_handles
    cleanup_mm_shm_handles.

Servicer (Python):

  • New shared mm_shm module (reads a TensorData payload inline-or-SHM,
    validates SHM names, unlinks after read, computes the /dev/shm namespace id).
  • The vLLM servicer reads tensors through mm_shm (the oneof renames .data
    .inline, so this is required) and advertises shm_namespace_id.

Test Plan

  • cargo clippy -p smg --all-targets -- -D warnings — clean
  • cargo test -p smg --lib — 1147 pass (one unrelated middleware::metrics
    interner test is a known parallel-test flake; passes in isolation)
  • cargo +nightly fmt — clean
  • Python proto regen (grpc_tools) — verified vllm_engine_pb2.TensorData
    exposes the inline|shm|remote oneof (shmcommon_pb2.ShmHandle) and
    GetServerInfoResponse.shm_namespace_id exists
  • python -m py_compile on mm_shm.py + the vLLM servicer — OK

e2e for the SHM path (co-located worker, assert SHM is taken) is deferred to the
multimodal coverage PR.

Follow-ups

  • Migrate the TokenSpeed servicer onto the shared mm_shm module (mechanical
    dedup; kept out of this PR to avoid churning the critical servicer).
  • vLLM video (is_videopixel_values_videos) — PR #3b.

Checklist

  • Conventional commit + DCO sign-off
  • cargo +nightly fmt + clippy -- -D warnings clean
  • Tests pass (Rust) + Python proto regen / py_compile verified
  • Wire-compatible proto (inline keeps field 1)
  • SHM cleanup on failure (no /dev/shm leak)

Summary by CodeRabbit

  • New Features
    • Added shared-memory transport for vLLM multimodal tensor payloads, with automatic fallback to inline when SHM is unavailable or below size thresholds.
    • Extended server information with a shared-memory namespace identifier to improve compatibility checks between workers and clients.
  • Bug Fixes
    • Improved multimodal shared-memory cleanup across both success and error paths to reduce leaked segments.
    • Hardened shared-memory payload reading with stricter validation and safer file access.
    • Fixed vLLM sequential decode reuse to avoid carrying over vision inputs.

Bring the same-host /dev/shm tensor transport to the vLLM backend, reusing the
engine-neutral ShmHandle (common.proto) and the configurable transport layer.

Proto (vllm_engine.proto):
- TensorData: `bytes data = 1` -> `oneof payload { bytes inline = 1;
  smg.grpc.common.ShmHandle shm = 4; smg.grpc.common.RemoteTensorHandle
  remote = 5; }`. Field 1 stays inline for wire compatibility.
- GetServerInfoResponse: add `shm_namespace_id = 10` so the router can verify a
  shared /dev/shm under `auto`.

Gateway (Rust):
- Extract an engine-neutral `resolve_mm_tensor_payload` (inline-vs-SHM decision +
  metrics), used by both `tokenspeed_tensor_payload` and the new
  `vllm_tensor_payload`.
- `VllmMultimodalData` carries the resolved shm_enabled / shm_min_bytes;
  `assemble_vllm` resolves them from the transport config + worker; `into_proto`
  emits the oneof payload.
- Add `collect_vllm_*_shm_handles`; the vLLM client cleans up SHM handles on
  send failure (mirrors TokenSpeed). Rename the engine-neutral
  `cleanup_tokenspeed_shm_handles` -> `cleanup_mm_shm_handles`.

Servicer (Python):
- New shared `mm_shm` module (reads a TensorData payload inline-or-SHM, computes
  the /dev/shm namespace id). The vLLM servicer reads tensors through it and
  advertises `shm_namespace_id` in GetServerInfo.

Default transport is `inline`, so existing deployments are unaffected. Migrating
the TokenSpeed servicer onto the shared `mm_shm` module (dedup) is a follow-up.
e2e for the SHM path is deferred to the multimodal coverage PR.

Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
@github-actions github-actions Bot added grpc gRPC client and router changes model-gateway Model gateway crate changes labels Jul 8, 2026
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds SHM-aware tensor payload transport to the vLLM proto, wires Python tensor decoding and server info reporting through shared-memory helpers, and updates the Rust gateway to build, transport, clean up, and reuse multimodal SHM payloads across vLLM request paths.

Changes

Multimodal SHM Tensor Transport

Layer / File(s) Summary
Proto schema: oneof tensor payload and shm namespace id
crates/grpc_client/proto/vllm_engine.proto
TensorData uses a oneof payload with inline, shm, and remote; GetServerInfoResponse adds shm_namespace_id.
Python mm_shm module: SHM read and namespace identity helpers
grpc_servicer/smg_grpc_servicer/mm_shm.py
Adds SHM payload dispatch, /dev/shm reads, SHM name validation, unlink control, and namespace ID computation.
vLLM servicer integration with mm_shm
grpc_servicer/smg_grpc_servicer/vllm/servicer.py
Uses mm_shm.tensor_payload_bytes when reconstructing tensors and returns shm_namespace_id in GetServerInfo.
Rust payload resolution, assembly, cleanup, and request flow
model_gateway/src/routers/grpc/proto_wrapper.rs, model_gateway/src/routers/grpc/multimodal/assemble.rs, model_gateway/src/routers/grpc/client.rs, model_gateway/src/routers/grpc/epd_encode.rs, model_gateway/src/routers/grpc/common/stages/request_execution.rs
Generalizes SHM payload selection for vLLM, propagates worker-based SHM settings, renames shared cleanup helpers, collects vLLM SHM handles on error, and clears reused decode multimodal pixels.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client as model_gateway
  participant Wrapper as proto_wrapper.rs
  participant Servicer as vllm/servicer.py
  participant Shm as /dev/shm
  Client->>Wrapper: resolve_mm_tensor_payload(...)
  Wrapper->>Shm: write or reference SHM payload
  Wrapper-->>Client: ProtoGenerateRequest
  Client->>Servicer: gRPC request with TensorData
  Servicer->>Servicer: mm_shm.tensor_payload_bytes(td)
  Servicer->>Shm: os.pread(fd, nbytes, offset)
  Shm-->>Servicer: raw tensor bytes
  Client->>Wrapper: cleanup_mm_shm_handles(handles) on failure
Loading

Possibly related PRs

  • lightseekorg/smg#1879: Shares the same Rust SHM cleanup and transport plumbing area touched by the gateway changes here.
  • lightseekorg/smg#1890: Related multimodal gRPC refactor that overlaps the assembly and request-building paths used in this PR.
  • lightseekorg/smg#1891: Introduces the common SHM handle message types referenced by the updated proto schema.

Suggested labels: multimodal, protocols

Suggested reviewers: key4ng, njhill

Poem

I hopped through bytes in moonlit air,
with SHM-tuned tensors everywhere.
Inline or shared, the payload glows,
and every burrow now cleanly knows.
🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Clear and specific; it summarizes the main change to add shared-memory tensor transport for vLLM multimodal inputs.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/vllm-shm-video

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements shared memory (SHM) tensor-transport support for vLLM multimodal inputs, matching the existing TokenSpeed SHM transport mechanism. It updates protobuf definitions, introduces a shared Python helper (mm_shm.py) for reading payloads and identifying /dev/shm namespaces, and refactors the Rust model gateway to handle vLLM SHM transport and cleanup. The review feedback highlights several performance optimization opportunities: caching the SHM namespace ID in Python to avoid repeated system calls, passing the payload directly to torch.frombuffer to achieve true zero-copy deserialization, and caching the environment variable lookup in Rust using OnceLock to prevent lock contention in hot paths.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread grpc_servicer/smg_grpc_servicer/mm_shm.py Outdated
Comment on lines +82 to +83
payload = mm_shm.tensor_payload_bytes(td)
return torch.frombuffer(bytearray(payload), dtype=torch_dtype).reshape(*td.shape)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using bytearray(payload) creates a mutable copy of the entire tensor data in CPU memory, which introduces an unnecessary copy and defeats the zero-copy benefit of the shared memory (SHM) transport. Since torch.frombuffer natively supports read-only buffers (like bytes) and returns a read-only tensor, you can pass payload directly to torch.frombuffer to achieve true zero-copy deserialization.

Suggested change
payload = mm_shm.tensor_payload_bytes(td)
return torch.frombuffer(bytearray(payload), dtype=torch_dtype).reshape(*td.shape)
payload = mm_shm.tensor_payload_bytes(td)
return torch.frombuffer(payload, dtype=torch_dtype).reshape(*td.shape)

Comment thread model_gateway/src/routers/grpc/proto_wrapper.rs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a59bde072

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

batched_keys,
flat_keys,
keep_on_cpu_keys: intermediate.keep_on_cpu_keys,
shm_enabled: resolve_mm_shm_enabled(workers, false),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid reusing one SHM handle for vLLM PD legs

In vLLM PD, execute_sequential_pd sends a cloned request to prefill and later reuses the original request as the decode request, so both legs carry the same mm_inputs. With SHM enabled here whenever the selected workers share /dev/shm, the prefill servicer reads and unlinks each segment, then the decode servicer receives the same ShmHandle and fails to open it before generation. Please either disable SHM for the multi-consumer vLLM PD path, create separate SHM payloads per leg, or keep the segment alive until both legs have consumed it.

Useful? React with 👍 / 👎.

data: self.pixel_values,
shape: self.pixel_values_shape,
dtype: "float32".to_string(),
payload: Some(vllm_tensor_payload(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clean up vLLM SHM files when request building fails

When vllm_tensor_payload chooses SHM, this call creates /dev/shm files during into_proto(), before the vLLM chat/messages builders run sampling and tool-constraint validation. Those builders still return with ? and do not use a finish_* cleanup wrapper like TokenSpeed, so invalid multimodal requests with SHM enabled leak one file per tensor before the request is ever sent. Please collect and unlink the vLLM handles on build errors as well as on send errors.

Useful? React with 👍 / 👎.

(Self::Vllm(client), ProtoGenerateRequest::Vllm(boxed_req)) => {
let stream = client.generate(*boxed_req).await?;
Ok(ProtoStream::Vllm(stream))
let shm_handles = collect_vllm_generate_request_shm_handles(&boxed_req);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Important: SHM cleanup covers the send-failure path here, but the build-failure path is unguarded. When into_proto() writes SHM segments during build_generate_request (e.g. at client.rs:495), then build_grpc_sampling_params_from_chat fails via ?, the SHM files leak — the segments are dropped without cleanup and generate() never runs.

TokenSpeed handles this with finish_tokenspeed_request (proto_wrapper.rs:675), which collects SHM handles before the build and cleans up on Err. The vLLM path needs an equivalent guard — either a finish_vllm_request wrapper or an explicit collect-and-cleanup around the into_proto() + build sequence in each build_generate_request arm.

pub fn cleanup_mm_shm_handles(handles: &[common::ShmHandle]) {
for handle in handles {
let Some(name) = validate_tokenspeed_shm_name_for_cleanup(&handle.name) else {
tracing::warn!(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: cleanup_mm_shm_handles was renamed to be engine-neutral, but its internals still reference "TokenSpeed" — the validation function (validate_tokenspeed_shm_name_for_cleanup) and the log messages ("invalid TokenSpeed SHM name", "Failed to cleanup TokenSpeed SHM file"). When debugging vLLM SHM cleanup issues, these messages will be confusing.

Since the naming prefix (smg-tokenspeed-) is shared for now (both engines use write_tokenspeed_shm), the behavior is correct — this is purely about log/diagnostic clarity. Consider updating the messages to say "multimodal SHM" or parameterizing by engine, similar to what resolve_mm_tensor_payload does.

# Unlink each /dev/shm segment right after the worker reads it (default on) so
# same-host SHM tensors don't accumulate. Disable with
# TOKENSPEED_UNLINK_MM_SHM_AFTER_READ=0 (e.g. for debugging).
UNLINK_MM_SHM_AFTER_READ = os.getenv("TOKENSPEED_UNLINK_MM_SHM_AFTER_READ", "1").lower() not in (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: The env var is TOKENSPEED_UNLINK_MM_SHM_AFTER_READ but this module is engine-neutral (used by both vLLM and TokenSpeed servicers). A SMG_UNLINK_MM_SHM_AFTER_READ name would be consistent with the module's scope. Not urgent — fine to defer if you plan to rename during the TokenSpeed servicer migration (mentioned in follow-ups).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@grpc_servicer/smg_grpc_servicer/mm_shm.py`:
- Line 10: The `/dev/shm` file open path is still vulnerable to symlink
traversal even after `validated_shm_name()`. Update the shm-open logic in
`mm_shm.py` to use `os.open(..., os.O_RDONLY | os.O_NOFOLLOW)` and then verify
the opened descriptor is a regular file before reading. If the target is a
symlink or any non-regular file, reject it and fail closed.

In `@model_gateway/src/routers/grpc/proto_wrapper.rs`:
- Around line 237-248: Add cleanup for SHM-backed payloads in the vLLM request
build path, since `into_proto` in `GrpcProtoWrapper` can create `/dev/shm` files
before request construction succeeds. Update the vLLM chat/messages builders to
wrap the request creation logic in a helper similar to
`finish_tokenspeed_request`, and ensure any `Err` from `into_proto` or later
request assembly triggers cleanup of the SHM handles so leaked files are removed
when the Python worker never receives the request.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 43a8cdd1-3a40-48ae-b5a7-6ce8effa7298

📥 Commits

Reviewing files that changed from the base of the PR and between 9b64940 and 6a59bde.

📒 Files selected for processing (7)
  • crates/grpc_client/proto/vllm_engine.proto
  • grpc_servicer/smg_grpc_servicer/mm_shm.py
  • grpc_servicer/smg_grpc_servicer/vllm/servicer.py
  • model_gateway/src/routers/grpc/client.rs
  • model_gateway/src/routers/grpc/epd_encode.rs
  • model_gateway/src/routers/grpc/multimodal/assemble.rs
  • model_gateway/src/routers/grpc/proto_wrapper.rs

Comment thread grpc_servicer/smg_grpc_servicer/mm_shm.py
Comment on lines +237 to +248
let shm_enabled = self.shm_enabled;
let shm_min_bytes = self.shm_min_bytes;
let model_specific_tensors = self
.model_specific_tensors
.into_iter()
.map(|(k, v)| {
(
k,
vllm::TensorData {
data: v.data,
shape: v.shape,
dtype: v.dtype,
payload: Some(vllm_tensor_payload(v.data, shm_enabled, shm_min_bytes)),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Add vLLM build-failure cleanup for SHM handles.

Line 248 and Line 264 can materialize /dev/shm files during into_proto. If vLLM request construction later returns Err, the request is never sent for the Python worker to read/unlink it; unlike TokenSpeed, the shown vLLM build paths do not wrap request construction with cleanup.

Proposed direction
+pub(crate) fn finish_vllm_request(
+    vllm_mm: Option<vllm::MultimodalInputs>,
+    build: impl FnOnce(Option<vllm::MultimodalInputs>) -> Result<vllm::GenerateRequest, String>,
+) -> Result<ProtoGenerateRequest, String> {
+    let shm_handles = vllm_mm
+        .as_ref()
+        .map(collect_vllm_multimodal_inputs_shm_handles)
+        .unwrap_or_default();
+    match build(vllm_mm) {
+        Ok(req) => Ok(ProtoGenerateRequest::Vllm(Box::new(req))),
+        Err(error) => {
+            cleanup_mm_shm_handles(&shm_handles);
+            Err(error)
+        }
+    }
+}

Then use this wrapper in the vLLM chat/messages request builders, mirroring finish_tokenspeed_request.

Also applies to: 260-268

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@model_gateway/src/routers/grpc/proto_wrapper.rs` around lines 237 - 248, Add
cleanup for SHM-backed payloads in the vLLM request build path, since
`into_proto` in `GrpcProtoWrapper` can create `/dev/shm` files before request
construction succeeds. Update the vLLM chat/messages builders to wrap the
request creation logic in a helper similar to `finish_tokenspeed_request`, and
ensure any `Err` from `into_proto` or later request assembly triggers cleanup of
the SHM handles so leaked files are removed when the Python worker never
receives the request.

- Sequential vLLM PD: clear the decode leg's multimodal inputs (mirrors the
  parallel PD path). The decode leg doesn't run the vision encoder, and with SHM
  the prefill leg already read+unlinked the /dev/shm segments, so a reused
  ShmHandle on decode would be unreadable. (codex P1)
- vLLM build path: add `finish_vllm_request`, which collects the SHM handles
  before building the request and unlinks them if the build fails (sampling /
  tool-constraint validation). `into_proto` can write /dev/shm files before the
  request is assembled, so a build error would otherwise leak them. Mirrors
  `finish_tokenspeed_request`. (claude, coderabbit, codex)
- mm_shm read: open with O_NOFOLLOW and require a regular file, so a crafted
  TensorData.shm name resolving to a pre-existing /dev/shm symlink can't redirect
  the read. (coderabbit)
- mm_shm: cache the /dev/shm namespace id (static per process, read on every
  GetServerInfo). (gemini)
- proto_wrapper: cache SMG_LOG_MM_TIMING via OnceLock instead of re-reading the
  env per tensor payload. (gemini)
- Fix ruff import ordering (mm_shm is first-party).

Deferred to the TokenSpeed-servicer migration follow-up: rename the shared
cleanup's TokenSpeed-flavored log/validation strings and the
TOKENSPEED_UNLINK_MM_SHM_AFTER_READ env var to engine-neutral names.

Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
@slin1237

slin1237 commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Thanks all — addressed in 0a7d4ad:

  • P1: sequential vLLM PD reuses the SHM handle across prefill+decode (codex) — fixed. The decode leg now clears its multimodal inputs (mirrors the parallel PD path); it doesn't run the vision encoder, and with SHM the prefill leg already read+unlinked the segments. Confirmed the parallel path already did this (clear_mm_pixel_values), so decode provably doesn't need the tensors.
  • Build-failure SHM leak (claude, coderabbit, codex) — fixed. Added finish_vllm_request (mirrors finish_tokenspeed_request): collects the SHM handles before the build and unlinks them if sampling/tool-constraint validation fails, so a bad request no longer leaks /dev/shm files. Both the chat and messages vLLM build arms use it. The send-failure path was already covered.
  • SHM symlink hardening (coderabbit) — fixed. mm_shm opens with O_NOFOLLOW and requires a regular file (fail closed), so a crafted name resolving to a pre-existing /dev/shm symlink can't redirect the read.
  • Cache shm_namespace_id (gemini) — fixed (module-level cache; static per process).
  • log_..._timing env re-read (gemini) — fixed (OnceLock cache).
  • Pre-commit ruff import order — fixed (mm_shm is first-party).

Declined / deferred:

  • bytearray → zero-copy frombuffer (gemini) — kept bytearray to preserve the existing writable-tensor behavior; passing bytes yields a read-only tensor, a behavior change I'd rather validate separately (perf follow-up).
  • cleanup_mm_shm_handles internals / TOKENSPEED_UNLINK_MM_SHM_AFTER_READ still say "TokenSpeed" (claude nits) — deferred to the TokenSpeed-servicer migration follow-up, where the shared SHM naming (smg-tokenspeed- prefix, env var, log strings) gets made engine-neutral together.

Verified: cargo clippy --all-targets -D warnings clean, 1147 lib tests pass, ruff check/ruff format clean, py_compile OK.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0a7d4ade9c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

// transfer), so drop the multimodal inputs — mirrors the parallel PD
// path. Load-bearing for SHM: prefill already read and unlinked the
// /dev/shm segments, so a reused ShmHandle here would be unreadable.
decode_request.clear_mm_pixel_values();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep multimodal inputs when decode must recompute

For vLLM PD requests with sampling n > 1, relay_kv_params is false, so the decode leg is intentionally not handed prefill KV and the NIXL path logs that decode will recompute the prompt locally. Clearing mm_inputs here means a multimodal decode request reaches vLLM with the expanded image tokens but without the image tensors/model-specific tensors, so NIXL n > 1 image requests will fail or generate from an incomplete prompt. Only drop these tensors on paths where decode actually consumes transferred KV, or give the recompute path its own payloads.

Useful? React with 👍 / 👎.

kv_role=kv_role,
kv_engine_id=kv_engine_id,
data_parallel_size=parallel.data_parallel_size,
shm_namespace_id=mm_shm.shm_namespace_id(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Require a proto release that contains the SHM fields

This servicer now constructs GetServerInfoResponse with shm_namespace_id, and _tensor_from_proto also calls WhichOneof("payload"), but grpc_servicer/pyproject.toml still allows smg-grpc-proto>=0.4.11 while the proto package version in this repo was not bumped past the pre-change 0.4.12. A normal package install can therefore pair the new servicer with generated stubs that do not define these fields, causing worker registration to raise TypeError: Protocol message GetServerInfoResponse has no "shm_namespace_id" field before SHM is even negotiated. Please bump/pin smg-grpc-proto to a new release before using the new field.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@grpc_servicer/smg_grpc_servicer/mm_shm.py`:
- Around line 46-48: Avoid blocking when opening shared-memory paths that may
point to FIFOs. In mm_shm.py, update the os.open call in the TensorData.shm path
handling to include O_NONBLOCK alongside O_RDONLY and O_NOFOLLOW, so the
subsequent stat.S_ISREG check can safely reject non-regular files without
hanging. Keep the fix localized to the same open-and-validate logic that uses
shm_handle.name and os.fstat(fd).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2a8e02c9-f280-46fe-80db-ce83ec109637

📥 Commits

Reviewing files that changed from the base of the PR and between 6a59bde and 0a7d4ad.

📒 Files selected for processing (5)
  • grpc_servicer/smg_grpc_servicer/mm_shm.py
  • grpc_servicer/smg_grpc_servicer/vllm/servicer.py
  • model_gateway/src/routers/grpc/client.rs
  • model_gateway/src/routers/grpc/common/stages/request_execution.rs
  • model_gateway/src/routers/grpc/proto_wrapper.rs

Comment on lines +46 to +48
fd = os.open(path, os.O_RDONLY | os.O_NOFOLLOW)
if not stat.S_ISREG(os.fstat(fd).st_mode):
raise ValueError(f"TensorData.shm is not a regular file: {shm_handle.name!r}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

sed -n '1,220p' grpc_servicer/smg_grpc_servicer/mm_shm.py

Repository: lightseekorg/smg

Length of output: 1955


🏁 Script executed:

python3 - <<'PY'
import os, tempfile, multiprocessing, time, errno, stat, sys

td = tempfile.mkdtemp(prefix="fifo-probe-")
fifo = os.path.join(td, "x")
reg = os.path.join(td, "r")
os.mkfifo(fifo, 0o600)
with open(reg, "wb") as f:
    f.write(b"hi")

def try_open(path, flags, q):
    try:
        fd = os.open(path, flags)
        try:
            mode = stat.S_IFMT(os.fstat(fd).st_mode)
            q.put(("ok", mode))
        finally:
            os.close(fd)
    except Exception as e:
        q.put(("err", type(e).__name__, getattr(e, "errno", None), str(e)))

def run(path, flags, timeout=1.0):
    q = multiprocessing.Queue()
    p = multiprocessing.Process(target=try_open, args=(path, flags, q))
    p.start()
    p.join(timeout)
    if p.is_alive():
        p.terminate()
        p.join()
        return ("timeout",)
    return q.get() if not q.empty() else ("no-result",)

cases = [
    ("fifo_rd", fifo, os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0)),
    ("fifo_rd_nonblock", fifo, os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) | os.O_NONBLOCK),
    ("reg_rd", reg, os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0)),
    ("reg_rd_nonblock", reg, os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) | os.O_NONBLOCK),
]
for name, path, flags in cases:
    print(name, run(path, flags))
PY

Repository: lightseekorg/smg

Length of output: 1955


Avoid blocking FIFO opens

os.open(path, os.O_RDONLY | os.O_NOFOLLOW) can still block if /dev/shm contains a FIFO at path. Add O_NONBLOCK so the regular-file check can reject non-regular targets without hanging.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@grpc_servicer/smg_grpc_servicer/mm_shm.py` around lines 46 - 48, Avoid
blocking when opening shared-memory paths that may point to FIFOs. In mm_shm.py,
update the os.open call in the TensorData.shm path handling to include
O_NONBLOCK alongside O_RDONLY and O_NOFOLLOW, so the subsequent stat.S_ISREG
check can safely reject non-regular files without hanging. Keep the fix
localized to the same open-and-validate logic that uses shm_handle.name and
os.fstat(fd).

@slin1237
slin1237 merged commit fb365c6 into main Jul 9, 2026
88 of 90 checks passed
@slin1237
slin1237 deleted the feat/vllm-shm-video branch July 9, 2026 00:01
slin1237 added a commit that referenced this pull request Jul 9, 2026
Enable video inputs on the vLLM gRPC path, previously rejected by
ensure_image_only. Video pixel tensors ride the same inline/SHM transport as
images (#1893), routed to vLLM's video modality.

Instead of a one-off is_video bool, hoist the Modality enum into common.proto
(like ShmHandle in #1) so the single-modality, precomputed-tensor engines
(vLLM + TokenSpeed) share one modality type; TokenSpeed's proto now references
smg.grpc.common.Modality. (SGLang keeps its string `modalities` for mixed-modality
inputs; converging it onto the common enum is a follow-up.)

- proto: Modality enum -> common.proto; tokenspeed references it; vLLM
  MultimodalInputs gets `common.Modality modality = 10`
- assemble: vLLM accepts image or video; assemble_vllm maps the modality and
  takes mm_hashes from videos vs images. Mixed rejected upstream in process.
- servicer: video -> pixel_values_videos under vLLM's `video`
  MultiModalFieldConfig; tokenspeed servicer reads modality from common_pb2
- test: modality proto round-trip

Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
slin1237 added a commit that referenced this pull request Jul 9, 2026
Enable video inputs on the vLLM gRPC path, previously rejected by
ensure_image_only. Video pixel tensors ride the same inline/SHM transport as
images (#1893), routed to vLLM's video modality.

Instead of a one-off is_video bool, hoist the Modality enum into common.proto
(like ShmHandle in #1) so the single-modality, precomputed-tensor engines
(vLLM + TokenSpeed) share one modality type; TokenSpeed's proto now references
smg.grpc.common.Modality. (SGLang keeps its string `modalities` for mixed-modality
inputs; converging it onto the common enum is a follow-up.)

- proto: Modality enum -> common.proto; tokenspeed references it; vLLM
  MultimodalInputs gets `common.Modality modality = 10`
- assemble: vLLM accepts image or video; assemble_vllm maps the modality and
  takes mm_hashes from videos vs images. Mixed rejected upstream in process.
- servicer: video -> pixel_values_videos under vLLM's `video`
  MultiModalFieldConfig; tokenspeed servicer reads modality from common_pb2
- test: modality proto round-trip

Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

grpc gRPC client and router changes model-gateway Model gateway crate changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant