fix(multimodal): own NVDEC frame memory and CUDA-gate PyNvVideoCodec in the SGLang image (OPS-8040) - #12733
Open
dmitry-tokarev-nv wants to merge 2 commits into
Open
Conversation
_frame_to_rgb_hwc converted frames with tensor.cpu().numpy(). The decoder is constructed with use_device_memory=False, so the DLPack-wrapped frame is already host memory: .cpu() is a no-op and .numpy() aliases memory the decoder owns and may recycle between indexed reads. Correct behaviour relied on PyNvVideoCodec handing each DecodedFrame its own buffer, which is undocumented. Copy each frame into owned memory instead; the docstring also still described the device-memory path, so rewrite it. Covers both call sites: decode_video_nvdec and the SGLang NvdecVideoDecoder.get_frames_as_tensor convert through this helper. The new regression test drives the helper with a numpy source (numpy exports DLPack exactly like a DecodedFrame): it fails on the previous code and passes with the copy. Raised by automated review on the release cherry-pick (#12703). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
The shared requirements.sglang.txt install was device-unguarded, so the NVIDIA-only NVDEC wheel landed in the Intel XPU image, where it is inert (no libnvcuvid). Mirror the existing branch in vllm_runtime.Dockerfile: filter it out with a line-anchored grep for non-CUDA devices and fail the build if the package still imports, so a silently broken filter cannot look like success. Rendered CUDA output is unchanged. Raised by automated review on the release cherry-pick (#12703). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
| grep -v '^PyNvVideoCodec' /tmp/requirements.sglang.txt > /tmp/requirements.sglang.nonvidia.txt && \ | ||
| pip install --break-system-packages --force-reinstall --no-deps \ | ||
| --requirement /tmp/requirements.sglang.nonvidia.txt && \ | ||
| ! python3 -c "import PyNvVideoCodec" 2>/dev/null |
There was a problem hiding this comment.
The non-CUDA guard passes when PyNvVideoCodec is installed but cannot import because libnvcuvid is missing, so the XPU image can still ship the unusable wheel. Fix: check module/package presence without importing PyNvVideoCodec.
🤖 AI Fix
In container/templates/sglang_runtime.Dockerfile, in the non-CUDA SGLang requirements RUN block, replace ! python3 -c "import PyNvVideoCodec" 2>/dev/null with python3 -c "import importlib.util, sys; sys.exit(importlib.util.find_spec('PyNvVideoCodec') is not None)".
Contributor
WalkthroughThe decoder now returns an owned ChangesNVDEC runtime behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/src/dynamo/common/multimodal/nvdec_decoder.py (2)
170-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
torchto module scope.This local import hides a third-party dependency and delays import errors until
_frame_to_rgb_hwcruns. Move it to the module-level third-party import block and runisort.As per coding guidelines, Python imports must stay at the top of the file. As per path instructions,
.ai/python-guidelines.mdrequires NVDEC imports at module scope and ordered via isort. Verify thattorchis available in every environment that imports this module.🤖 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 `@components/src/dynamo/common/multimodal/nvdec_decoder.py` at line 170, Move the torch import from the local scope of _frame_to_rgb_hwc to the module-level third-party import block, then run isort to maintain the required ordering. Verify torch is available in every environment that imports this module.Sources: Coding guidelines, Path instructions
172-175: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the DLPack fallback exception.
except Exceptiontreats everytorch.from_dlpack(frame)failure as an unsupported protocol. A malformed frame or conversion failure then falls through totorch.as_tensor(frame, device="cuda"), which can hide the original error and produce a misleading CUDA error. Catch only the documented unsupported-protocol or type exceptions. Re-raise other failures.As per coding guidelines, catch specific exceptions and fail fast instead of continuing after a broad
Exceptioncatch. Verify the exact exception classes for the supported Torch and PyNvVideoCodec versions.🤖 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 `@components/src/dynamo/common/multimodal/nvdec_decoder.py` around lines 172 - 175, In the DLPack conversion block around torch.from_dlpack, replace the broad Exception handler with only the documented unsupported-protocol or type-related exception classes supported by the project’s Torch and PyNvVideoCodec versions. Keep the torch.as_tensor CUDA fallback only for those exceptions, allowing all other conversion failures to propagate unchanged.Sources: Coding guidelines, Path instructions
🤖 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 `@container/templates/sglang_runtime.Dockerfile`:
- Around line 153-163: The non-CUDA guard in the sglang dependency installation
RUN block must verify package presence rather than importability. Replace the
PyNvVideoCodec import check with python3 -m pip show PyNvVideoCodec, and make
the guard fail with an explicit error message when the package is installed.
---
Nitpick comments:
In `@components/src/dynamo/common/multimodal/nvdec_decoder.py`:
- Line 170: Move the torch import from the local scope of _frame_to_rgb_hwc to
the module-level third-party import block, then run isort to maintain the
required ordering. Verify torch is available in every environment that imports
this module.
- Around line 172-175: In the DLPack conversion block around torch.from_dlpack,
replace the broad Exception handler with only the documented
unsupported-protocol or type-related exception classes supported by the
project’s Torch and PyNvVideoCodec versions. Keep the torch.as_tensor CUDA
fallback only for those exceptions, allowing all other conversion failures to
propagate unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a2acee82-3ca6-48fd-8aca-4e4c709e9002
📒 Files selected for processing (3)
components/src/dynamo/common/multimodal/nvdec_decoder.pycomponents/src/dynamo/common/tests/multimodal/test_nvdec_decoder.pycontainer/templates/sglang_runtime.Dockerfile
Comment on lines
+153
to
+163
| # import check fails the build if the package arrives by another route -- a | ||
| # filter that silently stopped matching would otherwise look like success. | ||
| # Whole-RUN branches rather than a conditional inside one RUN, for the reasons | ||
| # documented at the equivalent block in vllm_runtime.Dockerfile. | ||
| RUN --mount=type=bind,source=./container/deps/requirements.sglang.txt,target=/tmp/requirements.sglang.txt \ | ||
| --mount=type=cache,target=/root/.cache/pip,sharing=locked \ | ||
| export PIP_CACHE_DIR=/root/.cache/pip && \ | ||
| grep -v '^PyNvVideoCodec' /tmp/requirements.sglang.txt > /tmp/requirements.sglang.nonvidia.txt && \ | ||
| pip install --break-system-packages --force-reinstall --no-deps \ | ||
| --requirement /tmp/requirements.sglang.nonvidia.txt && \ | ||
| ! python3 -c "import PyNvVideoCodec" 2>/dev/null |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target Dockerfile ---'
sed -n '135,175p' container/templates/sglang_runtime.Dockerfile
printf '%s\n' '--- equivalent vLLM block ---'
rg -n -A24 -B8 'PyNvVideoCodec|nonvidia|non-CUDA|non-CUDA' container/templates/vllm_runtime.Dockerfile
printf '%s\n' '--- all relevant references ---'
rg -n 'PyNvVideoCodec|pip show|requirements\.sglang' container container/deps
printf '%s\n' '--- file status ---'
git status --shortRepository: ai-dynamo/dynamo
Length of output: 15572
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import os
import pathlib
import subprocess
import sys
import tempfile
with tempfile.TemporaryDirectory() as td:
root = pathlib.Path(td)
site = root / "site"
site.mkdir()
(site / "PyNvVideoCodec").mkdir()
(site / "PyNvVideoCodec" / "__init__.py").write_text(
"raise ImportError('libnvcuvid.so.1: cannot open shared object file')\n"
)
(site / "PyNvVideoCodec-2.2.0.dist-info").mkdir()
(site / "PyNvVideoCodec-2.2.0.dist-info" / "METADATA").write_text(
"Metadata-Version: 2.1\nName: PyNvVideoCodec\nVersion: 2.2.0\n"
)
env = os.environ.copy()
env["PYTHONPATH"] = str(site)
import_result = subprocess.run(
[sys.executable, "-c", "import PyNvVideoCodec"],
env=env, capture_output=True, text=True,
)
show_result = subprocess.run(
[sys.executable, "-m", "pip", "show", "PyNvVideoCodec"],
env=env, capture_output=True, text=True,
)
print("import return code:", import_result.returncode)
print("import stderr contains libnvcuvid:", "libnvcuvid" in import_result.stderr)
print("pip show return code:", show_result.returncode)
print("pip show reports package:", "Name: PyNvVideoCodec" in show_result.stdout)
absent = subprocess.run(
[sys.executable, "-m", "pip", "show", "PyNvVideoCodec"],
env={**os.environ, "PYTHONPATH": str(root / "empty")},
capture_output=True, text=True,
)
print("absent pip show return code:", absent.returncode)
PY
printf '%s\n' '--- SGLang post-install guards ---'
sed -n '365,390p' container/templates/sglang_runtime.Dockerfile
printf '%s\n' '--- SGLang image and device conditions ---'
rg -n -A8 -B8 'sglang|device ==|device !=|base.*image|FROM ' container/templates/sglang_runtime.Dockerfile | head -160Repository: ai-dynamo/dynamo
Length of output: 9866
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import pathlib
import subprocess
import sys
import tempfile
import venv
with tempfile.TemporaryDirectory() as td:
env_dir = pathlib.Path(td) / "venv"
venv.EnvBuilder(with_pip=True).create(env_dir)
py = env_dir / "bin" / "python"
site = pathlib.Path(
subprocess.check_output(
[py, "-c", "import sysconfig; print(sysconfig.get_paths()['purelib'])"],
text=True,
).strip()
)
(site / "PyNvVideoCodec").mkdir()
(site / "PyNvVideoCodec" / "__init__.py").write_text(
"raise ImportError('libnvcuvid.so.1: cannot open shared object file')\n"
)
dist = site / "PyNvVideoCodec-2.2.0.dist-info"
dist.mkdir()
(dist / "METADATA").write_text(
"Metadata-Version: 2.1\nName: PyNvVideoCodec\nVersion: 2.2.0\n"
)
import_result = subprocess.run(
[py, "-c", "import PyNvVideoCodec"],
capture_output=True, text=True,
)
show_result = subprocess.run(
[py, "-m", "pip", "show", "PyNvVideoCodec"],
capture_output=True, text=True,
)
negated_import = subprocess.run(
["/bin/sh", "-c", f"! '{py}' -c 'import PyNvVideoCodec' 2>/dev/null"],
capture_output=True, text=True,
)
presence_guard = subprocess.run(
["/bin/sh", "-c",
f"if '{py}' -m pip show PyNvVideoCodec >/dev/null 2>&1; then "
"echo 'ERROR: PyNvVideoCodec is installed in a non-CUDA image' >&2; "
"exit 1; fi"],
capture_output=True, text=True,
)
print("import return code:", import_result.returncode)
print("pip show return code:", show_result.returncode)
print("pip show reports package:", "Name: PyNvVideoCodec" in show_result.stdout)
print("negated import guard return code:", negated_import.returncode)
print("presence guard return code:", presence_guard.returncode)
print("presence guard message:", presence_guard.stderr.strip())
PYRepository: ai-dynamo/dynamo
Length of output: 870
🏁 Script executed:
#!/bin/bash
set -eu
python3 -m pip show pip >/tmp/pip-show-present.txt
if python3 -m pip show __definitely_missing_pynvvideocode__ >/dev/null 2>&1; then
echo "unexpected package found" >&2
exit 1
fi
printf 'pip show detects an installed distribution: '
grep -q '^Name: pip$' /tmp/pip-show-present.txt && echo yes
tmpdir=$(mktemp -d)
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/python3" <<'EOF'
#!/bin/sh
exit 1
EOF
chmod +x "$tmpdir/python3"
if PATH="$tmpdir:$PATH" sh -c '! python3 -c "import PyNvVideoCodec" 2>/dev/null'; then
echo "negated import guard passes when import fails"
else
echo "negated import guard failed unexpectedly" >&2
exit 1
fiRepository: ai-dynamo/dynamo
Length of output: 192
Make the non-CUDA guard test package presence. ! python3 -c "import PyNvVideoCodec" passes when the installed package fails to import because libnvcuvid is unavailable. Use python3 -m pip show PyNvVideoCodec and fail with an explicit error instead.
🤖 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 `@container/templates/sglang_runtime.Dockerfile` around lines 153 - 163, The
non-CUDA guard in the sglang dependency installation RUN block must verify
package presence rather than importability. Replace the PyNvVideoCodec import
check with python3 -m pip show PyNvVideoCodec, and make the guard fail with an
explicit error message when the package is installed.
|
🔄 Datadog auto-retried 1 job - 1 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: 3e0dfdb | Docs | Datadog PR Page | Give us feedback! |
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
1 participant
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.
Summary
Two hardening items surfaced by automated review on the release cherry-pick #12703. Both target code that merged in #11836, so they land on main first.
components/src/dynamo/common/multimodal/nvdec_decoder.py:_frame_to_rgb_hwcnow copies each decoded frame into memory it owns. The decoder is constructed withuse_device_memory=False, so the DLPack-wrapped frame is already host memory:.cpu()was a no-op and.numpy()aliased decoder-owned buffers — correctness relied on PyNvVideoCodec (undocumented) handing eachDecodedFrameits own buffer. The copy removes that dependency and covers both call sites (decode_video_nvdecand the SGLangNvdecVideoDecoder.get_frames_as_tensorconvert through this helper). The docstring still described the device-memory path, so it is rewritten. Raised in this review thread.container/templates/sglang_runtime.Dockerfile: dropPyNvVideoCodecfrom the non-CUDA (Intel XPU) SGLang image, mirroring the existing device branch invllm_runtime.Dockerfile: line-anchoredgrep -v '^PyNvVideoCodec'over the shared requirements plus a negative import check so a silently broken filter fails the build. The wheel is inert without libnvcuvid; this stops shipping it where it can never run. Raised in this review thread.Validation
test_frame_to_rgb_hwc_copies_out_of_decoder_memorydrives the helper with a numpy source (numpy exports DLPack exactly like aDecodedFrame): it fails on the previous code (mutating the source corrupts the collected frame) and passes with the copy. Fulltest_nvdec_decoder.py: 33 passed.sglangtemplate for both devices withcontainer/render.py: the CUDA output is functionally unchanged (same unfiltered install; blank-line-only diff from the template branch markers), the XPU output gains exactly the filter block and import check.pre-commit run --all-files: all hooks pass.Linear: OPS-8040 (sub-issue of OPS-7665)
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores