[DO NOT MERGE] perf(multimodal): reduce video decode, Qwen preprocess, and TokenSpeed handoff overhead - #1820
Closed
yechank-nvidia wants to merge 47 commits into
Closed
Conversation
yechank-nvidia
changed the title
yechank-nvidia
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request optimizes video decoding with OpenCV by sequentially grabbing intervening frames instead of seeking directly to each sampled frame. A review comment identified a critical bug where decoded_pos is not updated if capture.read() fails, which would cause subsequent frame decoding to go out of sync. The reviewer provided a code suggestion to correctly update the decoder position and handle grab failures explicitly.
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates multimodal decoding, vision preprocessing, gateway assembly, TokenSpeed serialization, and SHM handling. It adds borrowed-image preprocessing, raw-RGB patchification, duration-aware video backend routing, and deferred/shared-memory validation changes. ChangesMultimodal pipeline and TokenSpeed transport
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
documentation
yechank-nvidia
force-pushed
the
yechan/mm-video-decode-seq
branch
from
June 26, 2026 02:22
6215045 to
e99ae9e
Compare
yechank-nvidia
changed the title
yechank-nvidia
force-pushed
the
yechan/mm-video-decode-seq
branch
from
June 26, 2026 02:39
e99ae9e to
60d6d7b
Compare
yechank-nvidia
marked this pull request as ready for review
June 26, 2026 02:39
yechank-nvidia
requested review from
CatherineSue,
key4ng and
slin1237
as code owners
June 26, 2026 02:39
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60d6d7b68f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@crates/multimodal/src/media.rs`:
- Around line 323-332: The `read_and_hash` and `decode_video_frames_from_path`
paths in `media.rs` are operating on different snapshots of the same video file,
which can make `VideoClip.raw_bytes` and `VideoClip.hash` disagree with the
decoded frames. Change the `decode` flow so both hashing and frame decoding use
one immutable file snapshot or a stable descriptor/snapshot created once, rather
than reopening `canonical` separately. Keep the fix localized around
`read_and_hash`, `decode_video_frames_from_path`, and the `tokio::try_join!`
call so the returned `VideoClip` always reflects a single consistent file
version.
- Around line 1039-1043: The missing-binary branch in the `command.spawn()`
error handling still hardcodes a `video_url inputs` message, but
`MediaConnectorError::VideoDecode` is now used by `File`, `DataUrl`, and
`InlineBytes` paths too. Update the error text in this `map_err` block to use a
source-agnostic decode message tied to the `program` being spawned, so missing
`ffmpeg`/`ffprobe` guidance applies to all decode inputs.
In `@crates/multimodal/src/vision/processors/qwen_vl_base.rs`:
- Around line 1361-1398: The new test only covers the serial resize/preprocess
path and does not verify the image-parallel branch. Add a parity test around
QwenVLProcessorBase::patchify_image_rgb_block_band that exercises the same
resize/normalize inputs and compares its output against the existing serial
patchify/preprocess path, so the parallel image fast path gets the same
regression coverage as the video path.
In `@crates/multimodal/src/vision/transforms.rs`:
- Around line 446-456: The row-threshold check in par_threads can overflow when
computing 2 * cfg.min_rows_per_thread from
par_config/SMG_MM_PREPROCESS_PAR_MIN_ROWS. Update the early-return condition to
use saturating arithmetic for that comparison so it cannot panic in debug/tests
or wrap in release, while keeping the existing behavior of returning 1 for small
workloads.
In `@grpc_servicer/smg_grpc_servicer/tokenspeed/servicer.py`:
- Around line 1192-1193: The cast fallback in
TokenSpeedSchedulerServicer._feature_from_proto can trigger _tensor_from_proto
to unlink a shared packed SHM segment too early when multiple packed inputs
share the same backing file. Update the fallback path so it does not destroy
shared SHM before all packed items are read—either prevent packing when cast_to
is required on the servicer side, or change _tensor_payload_bytes_from_shm and
related SHM handling to defer unlinking until every offset/reference in the
shared segment has been materialized.
In `@model_gateway/src/routers/grpc/multimodal.rs`:
- Around line 1161-1186: Reject placeholder/item count mismatches before
constructing TokenSpeed items in the multimodal path: in the loop that builds
`pending_items`, stop relying on
`mm_placeholders_by_item.next().unwrap_or_default()` and instead validate that
`placeholders_for_items(&intermediate.placeholders, patch_offsets)` yields
exactly `item_count` groups before iteration. If the counts differ, return an
error early from the surrounding multimodal serialization flow so
`encoder_input_for_item` and `serialize_model_specific_for_item` are not used to
build partially invalid `PendingTokenSpeedItem` values.
🪄 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: f1d34efa-d749-452c-9e07-649de652678d
📒 Files selected for processing (12)
crates/multimodal/Cargo.tomlcrates/multimodal/src/media.rscrates/multimodal/src/vision/processor.rscrates/multimodal/src/vision/processors/qwen2_vl.rscrates/multimodal/src/vision/processors/qwen3_vl.rscrates/multimodal/src/vision/processors/qwen_vl_base.rscrates/multimodal/src/vision/transforms.rsdocs/reference/configuration.mdgrpc_servicer/smg_grpc_servicer/tokenspeed/servicer.pygrpc_servicer/tests/test_tokenspeed_multimodal_shm.pymodel_gateway/src/routers/grpc/multimodal.rsmodel_gateway/src/routers/grpc/proto_wrapper.rs
yechank-nvidia
force-pushed
the
yechan/mm-video-decode-seq
branch
from
June 26, 2026 05:59
60d6d7b to
054fbf1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 054fbf170d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
grpc_servicer/smg_grpc_servicer/tokenspeed/servicer.py (1)
961-1066: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnsure deferred SHM cleanup runs on error paths.
deferred_shm_unlinksis only drained after the whole multimodal input is built, so any later validation error after a tensor has been read/deferred leaks the SHM file. Wrap item reconstruction intry/finallyand call_unlink_deferred_shm(deferred_shm_unlinks)from thefinally.Suggested fix shape
- for item_proto in mm_inputs.items: + try: + for item_proto in mm_inputs.items: + ... + finally: + self._unlink_deferred_shm(deferred_shm_unlinks) ... - self._unlink_deferred_shm(deferred_shm_unlinks) return MultimodalInputs(🤖 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/tokenspeed/servicer.py` around lines 961 - 1066, The multimodal input reconstruction in the item-building loop leaks deferred SHM files when an exception is raised after `_tensor_from_proto` or `_feature_from_proto` has added names to `deferred_shm_unlinks`. Wrap the per-request rebuild logic around `mm_inputs.items` in a try/finally and ensure `_unlink_deferred_shm(deferred_shm_unlinks)` is called from the finally block in `servicer.py`, so cleanup runs even if `_validate_item_tensor_consistency`, `_offsets_from_proto_placeholders`, or later validation fails.
🤖 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 `@crates/multimodal/Cargo.toml`:
- Around line 38-44: Add the missing tokio features in the multimodal crate so
the production code builds correctly: update the tokio dependency in Cargo.toml
to include both io-util for AsyncReadExt and macros for try_join!. Check the
tokio entry used by crates/multimodal and keep the existing workspace
configuration while extending the feature list; do not rely on dev-dependencies
since media.rs is part of the main build.
In `@crates/multimodal/src/media.rs`:
- Around line 513-517: Update the sample_fps validation in media.rs to reject
non-finite values as well as non-positive ones, since the current check in the
sample_fps guard only catches <= 0.0 and still allows NaN and inf to reach
fps_filter_for_optional_duration. Add a finite-number check in the same
validation path around the sample_fps handling in the media decoding logic, and
keep the existing MediaConnectorError::VideoDecode error reporting consistent
for invalid input.
In `@crates/multimodal/src/vision/processors/qwen_vl_base.rs`:
- Around line 1721-1769: The test currently relies on preprocess_video_rgb and
par_threads() to hit the threaded path, but available_parallelism() can force
serial execution and skip the parallel branch. Update
test_preprocess_video_rgb_matches_dynamic_video_parallel_blocks to invoke the
block-band path directly, similar to the image block-band parity test, so it
exercises patchify_video_rgb_block_band with multiple disjoint bands regardless
of CPU count.
- Around line 898-1001: The planning logic in Qwen image/video preprocessing is
computing grid sizes, patch counts, and token metadata from
smart_resize/smart_resize_video even when do_resize is false, so the metadata
can describe a size that is never actually patchified. Update the plan-building
path in the relevant image and video processors (around the Qwen image planning
and the matching video preprocessing code) to derive grid_thw/num_patches/tokens
from the dimensions that will actually reach patchify when resizing is skipped,
and keep the resize decision aligned with that plan.
In `@grpc_servicer/smg_grpc_servicer/tokenspeed/servicer.py`:
- Around line 1160-1165: Validate the shared-memory byte size before calling
_tensor_payload_bytes in TokenSpeedSchedulerServicer._tensor_from_proto (and the
similar path around the second occurrence). Compute the expected tensor byte
count from the proto shape and dtype first, compare it against shm.nbytes, and
reject mismatches before any pread/opened read occurs. Keep the fix localized to
the tensor decoding flow so malformed requests fail early without reading the
SHM payload.
In `@grpc_servicer/tests/test_tokenspeed_multimodal_shm.py`:
- Around line 51-55: The SHM setup in test_tokenspeed_multimodal_shm.py writes
directly to /dev/shm without checking availability, so add a small preflight
check in the test before the file creation logic in the SHM setup block. If
/dev/shm is missing or not writable, call pytest.skip(...) early so the test is
skipped instead of failing before exercising the servicer path.
---
Outside diff comments:
In `@grpc_servicer/smg_grpc_servicer/tokenspeed/servicer.py`:
- Around line 961-1066: The multimodal input reconstruction in the item-building
loop leaks deferred SHM files when an exception is raised after
`_tensor_from_proto` or `_feature_from_proto` has added names to
`deferred_shm_unlinks`. Wrap the per-request rebuild logic around
`mm_inputs.items` in a try/finally and ensure
`_unlink_deferred_shm(deferred_shm_unlinks)` is called from the finally block in
`servicer.py`, so cleanup runs even if `_validate_item_tensor_consistency`,
`_offsets_from_proto_placeholders`, or later validation fails.
🪄 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: fa8995cd-c135-41c9-bf40-3d78e73d2a9a
📒 Files selected for processing (12)
crates/multimodal/Cargo.tomlcrates/multimodal/src/media.rscrates/multimodal/src/vision/processor.rscrates/multimodal/src/vision/processors/qwen2_vl.rscrates/multimodal/src/vision/processors/qwen3_vl.rscrates/multimodal/src/vision/processors/qwen_vl_base.rscrates/multimodal/src/vision/transforms.rsdocs/reference/configuration.mdgrpc_servicer/smg_grpc_servicer/tokenspeed/servicer.pygrpc_servicer/tests/test_tokenspeed_multimodal_shm.pymodel_gateway/src/routers/grpc/multimodal.rsmodel_gateway/src/routers/grpc/proto_wrapper.rs
yechank-nvidia
force-pushed
the
yechan/mm-video-decode-seq
branch
from
June 26, 2026 06:33
054fbf1 to
2f32d07
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@crates/multimodal/src/media.rs`:
- Around line 369-376: The non-file video path in the media.rs flow is currently
serializing independent work in the video clip creation logic, which increases
latency. Update the code around the hash and decode steps so
`crate::hasher::hash_video` and `decode_video_frames` run concurrently for
non-file sources, then join both results before calling
`video_clip_from_decoded`; use the existing `validate_video_fetch_config`,
`decode_video_frames`, and `MediaConnectorError::Blocking` handling to keep the
error path consistent.
- Around line 316-325: The local-file branch in media.rs is still using
decode_video_frames(bytes_for_decode, cfg), which forces a temp-file rewrite and
skips the new path-aware fast path. Update the decode path in the local-file
handling code to call the path-aware decoder directly with the canonical file
path (the same branch that computes the hash with crate::hasher::hash_video),
and keep the existing async join structure so hashing and decoding still run
concurrently.
In `@crates/multimodal/src/vision/processors/qwen_vl_base.rs`:
- Around line 919-980: `calculate_num_tokens()` is still using `smart_resize()`
even though `preprocess_images()` now supports the `do_resize=false` path with
unresized, factor-aligned dimensions, so placeholder counts can diverge from the
actual `feature_token_counts`. Update `calculate_num_tokens()` in `QwenVlBase`
to mirror the same resize-vs-validate branch used by the preprocessing path,
using the same `do_resize`/`validate_unresized_patch_dimensions()` logic and
`calculate_grid_thw()` token math so token reservation stays aligned with
`VisionPreProcessor` expectations.
In `@grpc_servicer/smg_grpc_servicer/tokenspeed/servicer.py`:
- Around line 971-1005: The TokenSpeed handling in servicer.py is missing
protection for preserved encoder SHM when deferred unlink cleanup runs, so add
tracking in the _feature_from_proto / tensor materialization flow to record
preserved encoder_input shm names, reject any overlap with model_specific_data
SHM names, and make sure preserved names are only added to deferred cleanup if
the request aborts or validation fails. Update the logic around the
encoder_input handle, deferred_shm_unlinks, and the finally/cleanup path so
preserved ShmTensorHandle segments are not unlinked before they are opened and
are cleaned up only on error.
In `@model_gateway/src/routers/grpc/multimodal.rs`:
- Around line 1155-1160: In assemble_tokenspeed, add a preflight validation
before slicing that the first dimension of every batched tensor exactly matches
item_count, and that the sum of all flat_item_spans for each flat tensor exactly
matches the tensor’s first dimension. Use the existing helpers
precomputed_multimodal_item_count and flat_item_spans to locate the check, and
reject any under-consumption cases before serialization so no rows are silently
dropped.
🪄 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: 0913a882-118e-4673-9e33-0fc9f4b78f16
📒 Files selected for processing (12)
crates/multimodal/Cargo.tomlcrates/multimodal/src/media.rscrates/multimodal/src/vision/processor.rscrates/multimodal/src/vision/processors/qwen2_vl.rscrates/multimodal/src/vision/processors/qwen3_vl.rscrates/multimodal/src/vision/processors/qwen_vl_base.rscrates/multimodal/src/vision/transforms.rsdocs/reference/configuration.mdgrpc_servicer/smg_grpc_servicer/tokenspeed/servicer.pygrpc_servicer/tests/test_tokenspeed_multimodal_shm.pymodel_gateway/src/routers/grpc/multimodal.rsmodel_gateway/src/routers/grpc/proto_wrapper.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
grpc_servicer/smg_grpc_servicer/tokenspeed/servicer.py (1)
978-1017: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject preserved handles that reuse already-deferred SHM.
The overlap guard is one-directional: if an earlier item materializes SHM
Xintodeferred_shm_unlinks, and a later item preserves encoder SHMX,completed=Truelets thefinallyunlinkXbefore TokenSpeed opens the preserved handle.Suggested fix
if isinstance(feature, ShmTensorHandle): + if feature.shm_name in deferred_shm_unlinks: + raise ValueError( + "preserved encoder_input handles must not share SHM segments " + "with materialized tensors" + ) preserved_encoder_shm_names.add(feature.shm_name)🤖 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/tokenspeed/servicer.py` around lines 978 - 1017, The SHM overlap check in the TokenSpeed servicer is only rejecting model-specific tensors that match preserved encoder handles, but it misses the reverse case where a preserved encoder SHM name was already queued in deferred_shm_unlinks by an earlier item. Update the handling around item_proto.model_specific_tensors and preserved_encoder_shm_names in servicer.py to also reject any preserved handle whose shm_name is already pending deferred unlink, using the existing _validated_shm_name, preserved_encoder_shm_names, and deferred_shm_unlinks symbols to keep the guard symmetric.
🤖 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/tests/test_tokenspeed_multimodal_shm.py`:
- Around line 116-156: The SHM cleanup in test_tokenspeed_multimodal_shm.py is
only performed after the ValueError assertion, so a failing expectation or
missing exception can leave the /dev/shm file behind. Keep the existing
pytest.raises check around
TokenSpeedSchedulerServicer._mm_inputs_from_itemized_proto, but wrap the test
body in a try/finally so the shared-memory path is always removed in the
fallback cleanup path, even if the assertion fails.
---
Outside diff comments:
In `@grpc_servicer/smg_grpc_servicer/tokenspeed/servicer.py`:
- Around line 978-1017: The SHM overlap check in the TokenSpeed servicer is only
rejecting model-specific tensors that match preserved encoder handles, but it
misses the reverse case where a preserved encoder SHM name was already queued in
deferred_shm_unlinks by an earlier item. Update the handling around
item_proto.model_specific_tensors and preserved_encoder_shm_names in servicer.py
to also reject any preserved handle whose shm_name is already pending deferred
unlink, using the existing _validated_shm_name, preserved_encoder_shm_names, and
deferred_shm_unlinks symbols to keep the guard symmetric.
🪄 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: 1967c4cd-5484-434c-bc35-1074867cc3c0
📒 Files selected for processing (5)
crates/multimodal/src/media.rscrates/multimodal/src/vision/processors/qwen_vl_base.rsgrpc_servicer/smg_grpc_servicer/tokenspeed/servicer.pygrpc_servicer/tests/test_tokenspeed_multimodal_shm.pymodel_gateway/src/routers/grpc/multimodal.rs
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc21fc87c4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 349528e780
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+924
to
+928
| let frame = OwnedRgbFrame { | ||
| width, | ||
| height, | ||
| data: Bytes::copy_from_slice(&rgb_bytes[..frame_size]), | ||
| }; |
There was a problem hiding this comment.
When the OpenCV streaming path is used (the low-concurrency RgbStream branch), each sampled frame is copied into an owned Bytes here without checking SMG_VIDEO_MAX_DECODED_BYTES; the non-streaming OpenCV path calls ensure_decoded_byte_limit before appending decoded RGB data. For a very high-resolution video, this path can allocate frame buffers beyond the configured decoded-payload guard instead of returning a VideoDecode error, so check the frame size (and any bounded in-flight total you intend to allow) before Bytes::copy_from_slice.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
grpc_servicer/smg_grpc_servicer/tokenspeed/servicer.py (1)
980-1019: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject SHM overlap when materialized tensors appear before preserved encoder handles.
The current check only catches
encoder_inputpreserved first, then model-specific SHM. If an earlier item’s model-specific tensor adds a name todeferred_shm_unlinks, a laterencoder_inputcan preserve the same name and then Line 1092 unlinks it on successful return.Proposed fix
if isinstance(feature, ShmTensorHandle): + if feature.shm_name in deferred_shm_unlinks: + raise ValueError( + "preserved encoder_input handles must not share SHM segments " + "with materialized tensors" + ) preserved_encoder_shm_names.add(feature.shm_name)Also applies to: 1089-1092
🤖 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/tokenspeed/servicer.py` around lines 980 - 1019, The SHM overlap validation in the token speed servicer is one-sided: the loop over item_proto.model_specific_tensors in servicer.py only rejects model-specific SHM that conflicts with already-preserved encoder_input names, but it misses the reverse case. Update the overlap check in the same parsing flow so encoder_input preservation also rejects any SHM name already present in deferred_shm_unlinks, using the existing helpers like _validated_shm_name and the preserved_encoder_shm_names/deferred_shm_unlinks sets. This should be enforced in the item processing path around the model_specific_data construction so a later preserved handle cannot accidentally be unlinked on successful return.
🤖 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 `@crates/multimodal/build.rs`:
- Around line 4-19: The build script in build.rs does not tell Cargo to rerun
when OPENCV_INCLUDE_PATHS changes, so header-path updates can be missed. Add a
rerun-if-env-changed directive alongside the existing rerun-if-changed logic
near the build setup in the build() flow, before reading
env::var_os("OPENCV_INCLUDE_PATHS"), so changes to that environment variable
correctly trigger a rebuild.
In `@crates/multimodal/src/media.rs`:
- Around line 881-927: The streamed OpenCV decode path in media.rs is missing
the same safety checks used by the buffered decoder. Update the frame loop in
the video decode helper around sampled_frame_counts so it rechecks
video_process_timeout() not only during intervening grab() calls but also
before/after capture.read() and the RGB conversion/copy path. Also add the
decoded-byte cap check used by the buffered path (ensure_decoded_byte_limit)
before copying the frame into OwnedRgbFrame, using rawvideo_frame_size and the
rgb_frame.data_bytes() length to reject oversized frames consistently.
- Around line 679-685: The `decode_video_with_opencv_bytes_logged` path is
copying the encoded video buffer unnecessarily, adding extra O(n) work and
memory. Change this helper to take `Bytes` by value instead of `&[u8]`, and pass
that `Bytes` directly into `decode_video_with_opencv_bytes_stream()`; also
update the caller in the blocking task to forward its existing `Bytes` clone
rather than rebuilding from a slice.
In `@crates/multimodal/src/opencv_buffer.rs`:
- Around line 18-43: `open_capture` currently returns a plain
`videoio::VideoCapture` while passing `bytes.as_ptr()` into
`smg_opencv_capture_from_buffer`, so the input buffer is not tied to the
capture’s lifetime and can be dropped too early. Update the API around
`open_capture` to either return an owning wrapper that keeps `bytes` alive for
as long as the capture exists, or mark the function `unsafe` and document the
caller’s lifetime responsibility. Use the `open_capture` and
`smg_opencv_capture_from_buffer` symbols to ensure the ownership/lifetime
contract is enforced at the boundary.
In `@crates/multimodal/src/vision/processor.rs`:
- Around line 408-414: The deferred path in
VisionProcessor::new_deferred_normalized leaves encoder_input as a placeholder,
so all encoder-input accessors must consistently read from the deferred metadata
instead of the backing tensor. Update channels(), height(), width(), and
encoder_input_flat() alongside ndim() and encoder_input_shape() to branch on
deferred state and derive values from DeferredNormalizedEncoderInput, so valid
deferred inputs do not return errors or empty data. Use the existing deferred
fields and helper methods in VisionProcessor to keep serialization and shape
reporting aligned.
- Around line 258-268: The deferred input validation in the processor setup
currently only checks total size and non-zero channel_run, but it must also
reject inputs that do not align with RGB LUT grouping. Update the validation
around the tensor size check and the fill_* path so channel_run must evenly
divide the deferred values and form complete 3-channel groups before any group %
3 LUT selection happens. Use the existing shape/data checks in the processor
logic to fail fast with InvalidShape rather than silently applying the wrong
normalization channel.
In `@model_gateway/src/routers/grpc/proto_wrapper.rs`:
- Around line 625-639: The temporary shared-memory setup in proto_wrapper.rs
only uses file.set_len before map_tokenspeed_shm, which does not reserve tmpfs
space and can still crash during write_fn. Update the allocation path to reserve
backing storage first (for example via fallocate/posix_fallocate) before
creating the mapping, and if reservation fails, fall back to the buffered path
instead of proceeding. Keep the cleanup behavior in the existing error branches
around map_tokenspeed_shm and write_fn so failures still remove the temporary
file.
---
Duplicate comments:
In `@grpc_servicer/smg_grpc_servicer/tokenspeed/servicer.py`:
- Around line 980-1019: The SHM overlap validation in the token speed servicer
is one-sided: the loop over item_proto.model_specific_tensors in servicer.py
only rejects model-specific SHM that conflicts with already-preserved
encoder_input names, but it misses the reverse case. Update the overlap check in
the same parsing flow so encoder_input preservation also rejects any SHM name
already present in deferred_shm_unlinks, using the existing helpers like
_validated_shm_name and the preserved_encoder_shm_names/deferred_shm_unlinks
sets. This should be enforced in the item processing path around the
model_specific_data construction so a later preserved handle cannot accidentally
be unlinked on successful return.
🪄 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: 5ddae15d-f4fa-4dda-ac66-15a94d0cabf7
📒 Files selected for processing (25)
Cargo.tomlcrates/multimodal/Cargo.tomlcrates/multimodal/build.rscrates/multimodal/src/lib.rscrates/multimodal/src/media.rscrates/multimodal/src/opencv_buffer.rscrates/multimodal/src/opencv_buffer_capture.cppcrates/multimodal/src/registry/mod.rscrates/multimodal/src/types.rscrates/multimodal/src/vision/mod.rscrates/multimodal/src/vision/processor.rscrates/multimodal/src/vision/processors/llama4_vision.rscrates/multimodal/src/vision/processors/phi3_vision.rscrates/multimodal/src/vision/processors/phi4_vision.rscrates/multimodal/src/vision/processors/pixtral.rscrates/multimodal/src/vision/processors/qwen2_vl.rscrates/multimodal/src/vision/processors/qwen3_vl.rscrates/multimodal/src/vision/processors/qwen_vl_base.rscrates/multimodal/src/vision/transforms.rsdocs/reference/configuration.mdgrpc_servicer/smg_grpc_servicer/tokenspeed/servicer.pygrpc_servicer/tests/test_tokenspeed_multimodal_shm.pymodel_gateway/Cargo.tomlmodel_gateway/src/routers/grpc/multimodal.rsmodel_gateway/src/routers/grpc/proto_wrapper.rs
Comment on lines
+625
to
+639
| if let Err(error) = file.set_len(file_len) { | ||
| drop(file); | ||
| let _ = remove_file(&path); | ||
| return Err(error); | ||
| } | ||
|
|
||
| let mut mapping = match map_tokenspeed_shm(&file, nbytes) { | ||
| Ok(mapping) => mapping, | ||
| Err(error) => { | ||
| drop(file); | ||
| let _ = remove_file(&path); | ||
| return Err(error); | ||
| } | ||
| }; | ||
| if let Err(error) = write_fn(&mut mapping) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect available platform/dependency options before choosing fallocate/posix_fallocate.
fd -a 'Cargo.toml' . | xargs -r rg -n 'memmap2|nix|rustix|libc|rust-version'
rg -n 'write_tokenspeed_shm_mapped|map_tokenspeed_shm|write_tokenspeed_shm_with|tokenspeed_shm_path' model_gateway/src/routers/grpc/proto_wrapper.rsRepository: lightseekorg/smg
Length of output: 931
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the mmap writer and any buffered fallback around it.
sed -n '540,690p' model_gateway/src/routers/grpc/proto_wrapper.rs
# Check whether the code already reserves space or otherwise handles ENOSPC/SIGBUS-related failures.
rg -n 'fallocate|posix_fallocate|set_len\(|remove_file\(|map_tokenspeed_shm|write_tokenspeed_shm_with|write_all\(' model_gateway/src/routers/grpc/proto_wrapper.rsRepository: lightseekorg/smg
Length of output: 6027
Reserve tmpfs backing before mapping
set_len() only changes the file size. In /dev/shm, the first page fault inside write_fn can still hit SIGBUS if tmpfs is full, crashing the gateway instead of returning an io::Error. Reserve the space first (fallocate/posix_fallocate) or fall back to the buffered path when reservation fails.
🤖 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 625 - 639, The
temporary shared-memory setup in proto_wrapper.rs only uses file.set_len before
map_tokenspeed_shm, which does not reserve tmpfs space and can still crash
during write_fn. Update the allocation path to reserve backing storage first
(for example via fallocate/posix_fallocate) before creating the mapping, and if
reservation fails, fall back to the buffered path instead of proceeding. Keep
the cleanup behavior in the existing error branches around map_tokenspeed_shm
and write_fn so failures still remove the temporary file.
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
yechank-nvidia
changed the title
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Signed-off-by: yechank-nvidia <161688079+yechank-nvidia@users.noreply.github.com>
Contributor
|
Hi @yechank-nvidia, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
Collaborator
|
Three PRs have been merged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Will divide into 3 pieces.