fix(vllm): use Dynamo FFmpeg for media bindings - #11817
Conversation
github-actions
Bot
added
fix
backend::vllm
|
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
e2dce4e to
cd97b1a
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| match = COMPONENT_RE.match(line.strip()) | ||
| if match is not None: | ||
| components.add(match.group(1)) | ||
| return components |
There was a problem hiding this comment.
🔴 Media codec safety check always fails, blocking the container image build
The tool that inventories the enabled video decoders and encoders mistakenly counts the help legend's "=" symbol as a codec name (match.group(1) at container/deps/vllm/validate_ffmpeg_provider.py:76) before comparing against the allowed set, so the exact-match comparison never succeeds and the guard rejects even a correctly built image.
Impact: The CUDA runtime image can never pass the final validation step, so the build aborts even when FFmpeg is built exactly as required.
How the legend rows are parsed as codecs
ffmpeg -hide_banner -decoders (and -encoders) prints a legend block before the actual component list, e.g.:
V..... = Video
.F.... = Frame-level multithreading
...X.. = Codec is experimental
------
V....D vp8 On2 VP8
ffmpeg_components at container/deps/vllm/validate_ffmpeg_provider.py:70-77 strips each line and applies COMPONENT_RE = ^[A-Z.]{6}\s+(\S+) (:43). For a legend row like V..... = Video, the first six characters V..... match [A-Z.]{6}, \s+ consumes the space, and (\S+) captures =. Every legend row yields the same = token, so the returned set becomes {"=", "vp8", "vp9", "rawvideo"}.
In main() at :156-168, decoders != ALLOWED_DECODERS (and the encoder equivalent) then evaluates to true because of the stray =, raising RuntimeError and failing the RUN step regardless of the real decoder/encoder set. The separator line ------ is correctly ignored (dashes are not in [A-Z.]), but the legend rows are not.
A fix should ignore the legend — e.g. only collect component names after the ------ separator, or discard matches whose captured name is =.
Prompt for agents
In ffmpeg_components (container/deps/vllm/validate_ffmpeg_provider.py:70-77), the parser incorrectly treats the ffmpeg -decoders/-encoders help legend as codec entries. The legend block lists flag descriptions like ` V..... = Video`, ` .F.... = Frame-level multithreading`, etc. before a `------` separator, followed by the real component rows like ` V....D vp8 ...`. COMPONENT_RE (`^[A-Z.]{6}\s+(\S+)`) matches the legend rows too, capturing `=` as the component name. This makes the returned set contain `=`, so the strict equality checks in main() (decoders != ALLOWED_DECODERS and encoders != ALLOWED_ENCODERS) always fail and raise RuntimeError, breaking the build even for a correctly configured FFmpeg. Fix by only collecting component names that appear after the `------` separator line, or by discarding any captured name equal to `=`.
Was this helpful? React with 👍 or 👎 to provide feedback.
| assert_links_to_dynamo_ffmpeg("torchcodec", f"libtorchcodec_core{ffmpeg_major}.so") | ||
|
|
There was a problem hiding this comment.
🔍 torchcodec must ship a core lib for the exact FFmpeg major
assert_links_to_dynamo_ffmpeg("torchcodec", f"libtorchcodec_core{ffmpeg_major}.so") at container/deps/vllm/validate_ffmpeg_provider.py:193-194 derives the major from EXPECTED_FFMPEG_VERSION (e.g. 8 for 8.1.2) and requires a file named libtorchcodec_core8.so. torchcodec ships versioned core libraries per supported FFmpeg major (core4/5/6/7/...). If the pinned torchcodec build does not include a core8 library, extension_libraries returns empty and the guard raises RuntimeError("...contains no extension libraries to inspect"). This depends on the torchcodec version bundled by the base image supporting FFmpeg 8 — worth confirming before the guard is enabled with #11628.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "--disable-bsfs", | ||
| "--disable-decoders", | ||
| "--disable-demuxers", | ||
| "--disable-encoders", | ||
| "--disable-parsers", | ||
| "--disable-protocols", | ||
| "--enable-decoder=vp8,vp9,rawvideo", | ||
| "--enable-demuxer=mov,matroska,rawvideo", | ||
| "--enable-encoder=h264_nvenc,libvpx_vp9", | ||
| "--enable-parser=vp8,vp9", | ||
| } | ||
| FORBIDDEN_DISTRIBUTIONS = { |
There was a problem hiding this comment.
🔍 buildconf/decoder allowlist intentionally diverges from current main
REQUIRED_BUILD_CONFIGURATION (:19-30) requires flags such as --disable-decoders, --disable-demuxers, --disable-parsers, --disable-protocols, --enable-decoder=vp8,vp9,rawvideo, --enable-demuxer=mov,matroska,rawvideo, --enable-parser=vp8,vp9, and ALLOWED_DECODERS/ALLOWED_ENCODERS expect a narrow set. The current wheel_builder.Dockerfile FFmpeg configure (container/templates/wheel_builder.Dockerfile:333-351) does NOT pass these narrowing flags, so both the buildconf check and the decoder-set check would fail against main. This is consistent with the PR description stating the build 'fails by design' until dependent #11628 lands, so it is not flagged as a bug — but reviewers should ensure #11628 actually introduces exactly these configure flags (including the comma-ordering, since --enable-decoder=vp8,vp9,rawvideo is compared as an exact token via build_configuration.split()).
Was this helpful? React with 👍 or 👎 to provide feedback.
WalkthroughChangesFFmpeg runtime
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
container/templates/vllm_runtime.Dockerfile (1)
357-363: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse the target environment’s Python when removing imageio’s bundled binary.
For CPU/XPU,
uvinstalls into/opt/venv, but Line 362 queries systempython3. The removal therefore targets the system site-packages and leaves/opt/venv/.../imageio_ffmpeg/binariesintact.Proposed fix
- SITE_PACKAGES="$(python3 -c 'import sysconfig; print(sysconfig.get_paths()["purelib"])')" && \ + SITE_PACKAGES="$({{ "python3" if device == "cuda" else "/opt/venv/bin/python" }} \ + -c 'import sysconfig; print(sysconfig.get_paths()["purelib"])')" && \ rm -rf "${SITE_PACKAGES}"/imageio_ffmpeg/binaries🤖 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/vllm_runtime.Dockerfile` around lines 357 - 363, Update the imageio-ffmpeg cleanup in the RUN instruction to query site-packages using the same target Python environment selected by pip_target, rather than the system python3. Ensure the resulting SITE_PACKAGES path points to the installed target environment so imageio_ffmpeg/binaries is removed for CPU/XPU and other targets.
🤖 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/deps/vllm/validate_ffmpeg_provider.py`:
- Around line 36-40: Track libvpx in both cleanup and validation: update
MEDIA_LIBRARY_RE in validate_ffmpeg_provider.py to match libvpx shared
libraries, and update the residual-library cleanup expression in
container/templates/vllm_runtime.Dockerfile to remove private libvpx copies
outside /usr/local/lib.
- Around line 18-29: Add “--disable-gpl” and “--disable-nonfree” to
REQUIRED_BUILD_CONFIGURATION, and update the validator’s configuration-checking
logic to reject contradictory “--enable-gpl” and “--enable-nonfree” tokens.
Ensure builds pass only when the explicit LGPL-only flags are present and
neither enabling token is configured.
- Around line 14-15: The validation currently trusts every library beneath
ALLOWED_LIBRARY_ROOT, allowing site-packages bundles such as av.libs to pass.
Tighten the artifact and linkage checks in the provider validation flow to
accept only resolved libraries whose immediate parent is /usr/local/lib, or an
explicit expected-file allowlist; preserve SEARCH_ROOTS for discovery without
treating nested directories as trusted.
- Around line 138-143: Update the FFmpeg validation around expected_version and
first_version_line to extract the reported version token from the `ffmpeg
-version` output and compare it for exact equality with `expected_version.
Preserve the existing RuntimeError and diagnostic output for mismatches, while
rejecting version prefixes such as `8.1.20` when `8.1.2` is expected.
In `@container/templates/vllm_runtime.Dockerfile`:
- Around line 46-57: Update the media wheel build command in the Dockerfile so
PEP 517 build dependencies for av and opencv-python are pinned as well. Either
preinstall the pinned build requirements and add --no-build-isolation to the
python3 -m pip wheel invocation, or provide an equivalent pinned
build-constraints file while preserving hash verification.
---
Outside diff comments:
In `@container/templates/vllm_runtime.Dockerfile`:
- Around line 357-363: Update the imageio-ffmpeg cleanup in the RUN instruction
to query site-packages using the same target Python environment selected by
pip_target, rather than the system python3. Ensure the resulting SITE_PACKAGES
path points to the installed target environment so imageio_ffmpeg/binaries is
removed for CPU/XPU and other targets.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: af9c9e64-0791-4e21-a013-cfa6b2f5967b
📒 Files selected for processing (3)
container/deps/vllm/requirements.media-source.txtcontainer/deps/vllm/validate_ffmpeg_provider.pycontainer/templates/vllm_runtime.Dockerfile
| ALLOWED_LIBRARY_ROOT = Path("/usr/local/lib") | ||
| SEARCH_ROOTS = (Path("/usr"), Path("/opt"), Path("/workspace")) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not treat all of /usr/local/lib as the trusted provider.
Python site-packages reside below /usr/local/lib, so bundled paths such as .../site-packages/av.libs/libavcodec.so pass both the artifact and linkage checks. A later upstream-wheel reinstall can therefore satisfy this guard while restoring private FFmpeg libraries.
Only accept resolved libraries whose direct parent is /usr/local/lib, or compare against an explicit set of expected provider files.
Proposed tightening
+def is_allowed_media_library(path: Path) -> bool:
+ return path.resolve().parent == ALLOWED_LIBRARY_ROOT
+
- if not path.resolve().is_relative_to(ALLOWED_LIBRARY_ROOT):
+ if not is_allowed_media_library(path):
artifacts.append(path)
...
- if not resolved.is_relative_to(ALLOWED_LIBRARY_ROOT):
+ if not is_allowed_media_library(resolved):Also applies to: 79-94, 106-131
🤖 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/deps/vllm/validate_ffmpeg_provider.py` around lines 14 - 15, The
validation currently trusts every library beneath ALLOWED_LIBRARY_ROOT, allowing
site-packages bundles such as av.libs to pass. Tighten the artifact and linkage
checks in the provider validation flow to accept only resolved libraries whose
immediate parent is /usr/local/lib, or an explicit expected-file allowlist;
preserve SEARCH_ROOTS for discovery without treating nested directories as
trusted.
| REQUIRED_BUILD_CONFIGURATION = { | ||
| "--disable-bsfs", | ||
| "--disable-decoders", | ||
| "--disable-demuxers", | ||
| "--disable-encoders", | ||
| "--disable-parsers", | ||
| "--disable-protocols", | ||
| "--enable-decoder=vp8,vp9,rawvideo", | ||
| "--enable-demuxer=mov,matroska,rawvideo", | ||
| "--enable-encoder=h264_nvenc,libvpx_vp9", | ||
| "--enable-parser=vp8,vp9", | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce the LGPL configuration explicitly.
The validator does not require --disable-gpl or --disable-nonfree, despite the runtime’s stated LGPL-only contract. A build with the expected codecs but GPL/nonfree mode enabled can pass.
Proposed fix
REQUIRED_BUILD_CONFIGURATION = {
+ "--disable-gpl",
+ "--disable-nonfree",
"--disable-bsfs",Also reject contradictory --enable-gpl and --enable-nonfree tokens.
Also applies to: 145-153
🤖 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/deps/vllm/validate_ffmpeg_provider.py` around lines 18 - 29, Add
“--disable-gpl” and “--disable-nonfree” to REQUIRED_BUILD_CONFIGURATION, and
update the validator’s configuration-checking logic to reject contradictory
“--enable-gpl” and “--enable-nonfree” tokens. Ensure builds pass only when the
explicit LGPL-only flags are present and neither enabling token is configured.
| MEDIA_LIBRARY_RE = re.compile( | ||
| r"^(?:libav(?:codec|device|filter|format|util)|libpostproc|" | ||
| r"libsw(?:resample|scale)|libx26[45]|libopenh264|libfdk-aac|libfaac|" | ||
| r"libvo-aacenc|libaacplus).*\.so(?:\..*)?$" | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Track libvpx across both cleanup and validation. The provider intentionally supplies the allowlisted libvpx, but private copies outside /usr/local/lib are neither removed nor detected.
container/deps/vllm/validate_ffmpeg_provider.py#L36-L40: addlibvpxtoMEDIA_LIBRARY_RE.container/templates/vllm_runtime.Dockerfile#L342-L344: addlibvpxto the residual-library cleanup expression.
📍 Affects 2 files
container/deps/vllm/validate_ffmpeg_provider.py#L36-L40(this comment)container/templates/vllm_runtime.Dockerfile#L342-L344
🤖 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/deps/vllm/validate_ffmpeg_provider.py` around lines 36 - 40, Track
libvpx in both cleanup and validation: update MEDIA_LIBRARY_RE in
validate_ffmpeg_provider.py to match libvpx shared libraries, and update the
residual-library cleanup expression in
container/templates/vllm_runtime.Dockerfile to remove private libvpx copies
outside /usr/local/lib.
| expected_version = os.environ["EXPECTED_FFMPEG_VERSION"] | ||
| first_version_line = run(str(FFMPEG), "-version").splitlines()[0] | ||
| if not first_version_line.startswith(f"ffmpeg version {expected_version}"): | ||
| raise RuntimeError( | ||
| f"expected FFmpeg {expected_version}, found: {first_version_line}" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compare the FFmpeg version token exactly.
startswith() accepts unintended versions such as 8.1.20 when 8.1.2 is expected. Parse the version token and compare it for equality.
Proposed fix
first_version_line = run(str(FFMPEG), "-version").splitlines()[0]
- if not first_version_line.startswith(f"ffmpeg version {expected_version}"):
+ match = re.match(r"^ffmpeg version (\S+)", first_version_line)
+ if match is None or match.group(1) != expected_version:
raise RuntimeError(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expected_version = os.environ["EXPECTED_FFMPEG_VERSION"] | |
| first_version_line = run(str(FFMPEG), "-version").splitlines()[0] | |
| if not first_version_line.startswith(f"ffmpeg version {expected_version}"): | |
| raise RuntimeError( | |
| f"expected FFmpeg {expected_version}, found: {first_version_line}" | |
| ) | |
| first_version_line = run(str(FFMPEG), "-version").splitlines()[0] | |
| match = re.match(r"^ffmpeg version (\S+)", first_version_line) | |
| if match is None or match.group(1) != expected_version: | |
| raise RuntimeError( | |
| f"expected FFmpeg {expected_version}, found: {first_version_line}" | |
| ) |
🤖 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/deps/vllm/validate_ffmpeg_provider.py` around lines 138 - 143,
Update the FFmpeg validation around expected_version and first_version_line to
extract the reported version token from the `ffmpeg -version` output and compare
it for exact equality with `expected_version. Preserve the existing RuntimeError
and diagnostic output for mismatches, while rejecting version prefixes such as
`8.1.20` when `8.1.2` is expected.
| RUN --mount=type=bind,source=./container/deps/vllm/requirements.media-source.txt,target=/tmp/requirements.media-source.txt,readonly \ | ||
| --mount=type=cache,target=/root/.cache/pip,sharing=locked \ | ||
| set -eux; \ | ||
| export CMAKE_BUILD_PARALLEL_LEVEL="$(nproc)"; \ | ||
| export CMAKE_PREFIX_PATH=/usr/local; \ | ||
| export CMAKE_ARGS="-DWITH_FFMPEG=ON -DOPENCV_FFMPEG_USE_FIND_PACKAGE=OFF -DOPENCV_FFMPEG_ENABLE_LIBAVDEVICE=OFF -DOPENCV_FFMPEG_SKIP_DOWNLOAD=ON -DWITH_GSTREAMER=OFF -DWITH_QT=OFF -DWITH_GTK=OFF -DWITH_WIN32UI=OFF -DCMAKE_BUILD_RPATH=/usr/local/lib -DCMAKE_INSTALL_RPATH=/usr/local/lib"; \ | ||
| python3 -m pip wheel \ | ||
| --wheel-dir /opt/vllm-media-wheels \ | ||
| --no-deps \ | ||
| --no-binary=av,opencv-python \ | ||
| --require-hashes \ | ||
| --requirement /tmp/requirements.media-source.txt |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline container/templates/vllm_runtime.Dockerfile --view expanded || true
printf '\n== Relevant file sections ==\n'
nl -ba container/templates/vllm_runtime.Dockerfile | sed -n '1,140p'
printf '\n== Referenced requirements file ==\n'
nl -ba container/deps/vllm/requirements.media-source.txt | sed -n '1,200p'Repository: ai-dynamo/dynamo
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Dockerfile excerpt ==\n'
awk 'NR>=1 && NR<=140 { printf "%4d %s\n", NR, $0 }' container/templates/vllm_runtime.Dockerfile
printf '\n== requirements.media-source.txt ==\n'
awk 'NR>=1 && NR<=220 { printf "%4d %s\n", NR, $0 }' container/deps/vllm/requirements.media-source.txt
printf '\n== Search for pyproject/build metadata in referenced packages ==\n'
rg -n --hidden --glob '!**/.git/**' 'build-system|pyproject.toml|setuptools|scikit-build|cmake|wheel|meson' container/deps/vllmRepository: ai-dynamo/dynamo
Length of output: 11165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Search for build-isolation / build constraints ==\n'
rg -n --hidden --glob '!**/.git/**' 'no-build-isolation|build-constraint|build constraint|PEP 517|require-hashes|--no-binary=av,opencv-python|scikit-build-core|build-system' .
printf '\n== Search for related requirements files ==\n'
fd -a -t f 'requirements*.txt' .Repository: ai-dynamo/dynamo
Length of output: 2225
Pin the PEP 517 build inputs too. --require-hashes only covers the source archives here; pip wheel still resolves build requirements for av and opencv-python in isolated envs, so those executable inputs remain unpinned. Preinstall the build deps and add --no-build-isolation, or use a pinned build-constraints file.
🤖 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/vllm_runtime.Dockerfile` around lines 46 - 57, Update the
media wheel build command in the Dockerfile so PEP 517 build dependencies for av
and opencv-python are pinned as well. Either preinstall the pinned build
requirements and add --no-build-isolation to the python3 -m pip wheel
invocation, or provide an equivalent pinned build-constraints file while
preserving hash verification.
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
| continue | ||
| resolved = Path(match.group(1)).resolve() | ||
| resolved_media_libraries.add(resolved) | ||
| if not resolved.is_relative_to(ALLOWED_LIBRARY_ROOT): |
There was a problem hiding this comment.
The validator trusts any FFmpeg library under /usr/local/lib, so private wheel-bundled libraries in /usr/local/lib/python.../site-packages/*.libs pass both the artifact scan and ldd checks instead of being rejected. Fix: only allow resolved FFmpeg libraries whose parent is exactly /usr/local/lib, and use that stricter helper in both unexpected_media_artifacts and assert_links_to_dynamo_ffmpeg.
🤖 AI Fix
In container/deps/vllm/validate_ffmpeg_provider.py, add an is_dynamo_ffmpeg_library(path: Path) -> bool helper that returns path.resolve().parent == ALLOWED_LIBRARY_ROOT, replace the is_relative_to(ALLOWED_LIBRARY_ROOT) checks in unexpected_media_artifacts and assert_links_to_dynamo_ffmpeg with is_dynamo_ffmpeg_library(...), and keep raising for any matched media library under site-packages.
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR has been closed due to inactivity. If you believe this PR is still relevant, please feel free to reopen it with additional context or information. |
1 participant
Summary
/usr/local/bin/ffmpeg/usr/local/lib/libav*libraries, then verify that PyAV, OpenCV, TorchCodec, and imageio resolve to that providerBlocked by #11628 and intended to merge after it. The final guard deliberately requires FFmpeg 8.1.2 with decoders
rawvideo,vp8, andvp9, and encodersh264_nvencandlibvpx_vp9; a full image build against currentmain's broader FFmpeg 8.1 configuration will fail by design.This cleans the merged runtime filesystem. Because the image still inherits
vllm/vllm-openai, deleted upstream bytes remain present in inherited OCI layers; eliminating those distributed bytes requires rebuilding from a clean base or producing a flattened image.Validation
linux/amd64andlinux/arm64docker buildx build --check --platform linux/amd64 -f container/vllm-runtime-cuda13.0-amd64-rendered.Dockerfile .(passes with the existing manylinux platform warning)ruff check container/deps/vllm/validate_ffmpeg_provider.pyruff format --check container/deps/vllm/validate_ffmpeg_provider.pypython3 -m py_compile container/deps/vllm/validate_ffmpeg_provider.pySummary by CodeRabbit