Skip to content

Gate service ffmpeg install at runtime#2052

Merged
jperez999 merged 17 commits into
NVIDIA:mainfrom
charlesbluca:codex/optional-ffmpeg-install
May 21, 2026
Merged

Gate service ffmpeg install at runtime#2052
jperez999 merged 17 commits into
NVIDIA:mainfrom
charlesbluca:codex/optional-ffmpeg-install

Conversation

@charlesbluca
Copy link
Copy Markdown
Collaborator

@charlesbluca charlesbluca commented May 18, 2026

Description

Make ffmpeg/ffprobe optional in the bundled service image by removing the custom source-build installer and replacing the old build-time path with a runtime install flow.

The service image now keeps ffmpeg/ffprobe out by default. When audio or video extraction is needed, users can set INSTALL_FFMPEG=true for the service container, or set service.installFfmpeg=true in Helm. The service entrypoint installs the Ubuntu ffmpeg package at startup through narrowly scoped passwordless sudo, which provides both ffmpeg and ffprobe.

This PR also:

  • Splits media dependency checks into distinct ffmpeg-python, ffmpeg, and ffprobe checks so missing dependencies produce actionable install guidance.
  • Uses the granular checks where possible, so audio extraction and frame extraction only require ffmpeg-python plus the ffmpeg CLI, while media splitting/probing still requires ffprobe.
  • Adds a Helm service.installFfmpeg value and a duplicate-env guard so users cannot set both service.installFfmpeg=true and INSTALL_FFMPEG manually in service.env.
  • Updates README, Helm, troubleshooting, prerequisites, audio/video, and release-note docs for the runtime install flow and caveats.

Validation:

  • env UV_CACHE_DIR=/localhome/charlesb/.cache/tmp/uv-cache uv run --directory nemo_retriever --extra dev pytest tests/test_container_ffmpeg_install.py tests/test_media_dependency_availability.py -q
  • git diff --check
  • bash -n docker/scripts/retriever_service_entrypoint.sh
  • black --check and flake8 on touched Python files
  • focused pre-commit hooks for whitespace, EOF, large files, AST, and debug statements

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • If adjusting docker-compose.yaml environment variables have you ensured those are mimicked in the Helm values.yaml file.

@charlesbluca charlesbluca changed the title [codex] Make ffmpeg install optional Make ffmpeg install optional May 18, 2026
@charlesbluca charlesbluca marked this pull request as ready for review May 18, 2026 18:33
@charlesbluca charlesbluca requested review from a team as code owners May 18, 2026 18:33
@charlesbluca charlesbluca requested a review from edknv May 18, 2026 18:33
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 18, 2026

Greptile Summary

This PR makes ffmpeg/ffprobe optional in the bundled service image by removing the build-time source install and replacing it with a runtime install flow. The nemo user is granted narrowly scoped, argument-rejecting passwordless sudo to a fixed wrapper script (retriever-install-ffmpeg), which is triggered at container startup when INSTALL_FFMPEG=true or service.installFfmpeg=true in Helm.

  • Runtime install path: a dedicated retriever_install_ffmpeg.sh wrapper rejects all arguments, uses absolute paths for apt-get, and is validated with visudo; the entrypoint wraps sudo invocation inside a case-insensitive INSTALL_FFMPEG check.
  • Granular dependency checks: media_interface.py gains distinct availability checks and a centralized media_dependency_error_message() that maps missing components to actionable install instructions.
  • Helm guard: deployment.yaml fails rendering when both service.installFfmpeg=true and INSTALL_FFMPEG appear in service.env; the guard is placed before the topology mode branch so it covers both standalone and split deployments.

Confidence Score: 5/5

Safe to merge; the runtime ffmpeg install path is well-scoped and the sudoers argument-injection concern from a prior review is resolved by the wrapper script approach.

The wrapper rejects all arguments, uses absolute apt-get paths, and is validated by visudo at build time. Python-side dependency checks are granular and consistent with MEDIA_DEPENDENCIES. Tests cover the install contract end-to-end. Both findings are maintenance nits with no runtime impact.

No files require special attention; the wrapper script and sudoers rule in the Dockerfile are the highest-risk area and have been reviewed carefully.

Important Files Changed

Filename Overview
Dockerfile Removes build-time ffmpeg install; adds sudo, copies entrypoint and install script, creates a narrowly scoped sudoers rule for nemo user, validates it with visudo.
docker/scripts/retriever_install_ffmpeg.sh New wrapper script that rejects all arguments, uses full absolute paths for apt-get, and is the sole target of the sudoers NOPASSWD rule.
docker/scripts/retriever_service_entrypoint.sh New service entrypoint: lowercases INSTALL_FFMPEG, skips install if binaries already present, delegates to the restricted wrapper via sudo, and exec-replaces itself with CMD arguments.
nemo_retriever/src/nemo_retriever/audio/media_interface.py Replaces monolithic _FFMPEG_AVAILABLE flag with granular per-tool availability checks, adds centralized media_dependency_error_message(), narrows import guard to except ImportError, adds explicit RuntimeError/OSError handlers.
nemo_retriever/helm/templates/deployment.yaml Adds installFfmpeg rendering and a duplicate-env guard placed before the topology branch so it applies to both standalone and split modes.
nemo_retriever/tests/test_container_ffmpeg_install.py Good coverage of the new runtime install contract. One assertion uses a whitespace-sensitive literal to detect sudo in the Dockerfile.
nemo_retriever/tests/test_media_dependency_availability.py Comprehensive unit tests for each availability check and per-operation dependency requirements.
docs/docs/extraction/deployment-options.md Updated to describe the runtime install path via service.installFfmpeg=true; previously incorrect Kubernetes claims have been corrected.

Sequence Diagram

sequenceDiagram
    participant User
    participant Container as Service Container (nemo user)
    participant Entrypoint as retriever-service-entrypoint
    participant Sudo as sudo
    participant Wrapper as retriever-install-ffmpeg (root)
    participant APT as apt-get

    User->>Container: "docker run -e INSTALL_FFMPEG=true ..."
    Container->>Entrypoint: "exec entrypoint $@"
    Entrypoint->>Entrypoint: lowercase INSTALL_FFMPEG
    alt ffmpeg/ffprobe already present
        Entrypoint->>Entrypoint: skip install
    else not present
        Entrypoint->>Sudo: sudo /usr/local/sbin/retriever-install-ffmpeg
        Sudo->>Wrapper: exec as root (NOPASSWD sudoers rule)
        Wrapper->>Wrapper: reject if args present
        Wrapper->>APT: /usr/bin/apt-get update
        APT-->>Wrapper: ok
        Wrapper->>APT: /usr/bin/apt-get install -y ffmpeg
        APT-->>Wrapper: installs ffmpeg + ffprobe
        Wrapper->>APT: /usr/bin/apt-get clean
        Wrapper-->>Sudo: exit 0
        Sudo-->>Entrypoint: ok
    end
    Entrypoint->>Container: exec retriever service start ...
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
nemo_retriever/src/nemo_retriever/audio/media_interface.py:424-426
`is_media_available()` manually replicates the three-check conjunction instead of delegating to `missing_media_dependencies()`. If `MEDIA_DEPENDENCIES` ever gains a fourth entry (e.g. a new codec tool), `is_media_available()` would silently fall out of sync and return `True` even when the new dependency is absent, while `media_dependency_error_message()` would correctly report it missing.

```suggestion
def is_media_available() -> bool:
    """True if the full audio/video media pipeline can run."""
    return not missing_media_dependencies()
```

### Issue 2 of 2
nemo_retriever/tests/test_container_ffmpeg_install.py:38
This assertion checks for `sudo` via a whitespace-sensitive literal that embeds the exact six-space indentation and the continuation backslash from the `apt-get` block. Any reformatting of that RUN layer (e.g. switching to four-space indentation) would silently break this test while the actual Dockerfile stays correct. Checking for the `sudo` package in the package list without caring about surrounding whitespace is more robust.

```suggestion
        self.assertIn("sudo", dockerfile)
```

Reviews (14): Last reviewed commit: "Fix ffmpeg installer policy test in serv..." | Re-trigger Greptile

Comment thread nemo_retriever/src/nemo_retriever/audio/media_interface.py Outdated
Comment thread nemo_retriever/src/nemo_retriever/audio/media_interface.py Outdated
Comment thread nemo_retriever/src/nemo_retriever/audio/media_interface.py Outdated
Comment thread nemo_retriever/src/nemo_retriever/audio/media_interface.py Outdated
@@ -26,11 +26,59 @@

try:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

couldnt we do all these checks here, that way they are not peppered throughout the code modules. This way all the gating logic is always at the top of the module that is affected. Wdyt?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Think the biggest thing we'd want to be careful around is collapsing the import-check into just "are ffmpeg-python & ffmpeg/ffprobe available?" as it looks like there are methods of extraction (i.e. frame extraction) that exclusively make use of ffmpeg but not ffprobe

IMO, I would feel better about keeping this PR restricted mostly to media interface import contract stuff (making it non-default, ensuring users an informative error message when it's not available), and following up with a media interface clean-up PR post-release that cleanly centralizes the import checks

Copy link
Copy Markdown
Collaborator

@jperez999 jperez999 left a comment

Choose a reason for hiding this comment

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

Seems like this PR does not actually allow for the user to activate the installation on running the container. We want this to actually pull down the built container and before it starts running the service, it should look for this env var and it should install ffmpeg if activated. docker run --env INSTALL_FFMPEG=true ...

Comment thread nemo_retriever/helm/README.md Outdated
Comment thread nemo_retriever/helm/README.md Outdated
@charlesbluca charlesbluca force-pushed the codex/optional-ffmpeg-install branch from 00e6fa9 to ea0e927 Compare May 20, 2026 14:03
@charlesbluca charlesbluca changed the title Make ffmpeg install optional Gate service ffmpeg install at runtime May 20, 2026
@sosahi
Copy link
Copy Markdown
Collaborator

sosahi commented May 20, 2026

Just a reminder to merge this into the 26.05 branch as well when ready.

@jperez999 jperez999 merged commit 13ee35b into NVIDIA:main May 21, 2026
6 checks passed
charlesbluca added a commit that referenced this pull request May 21, 2026
kheiss-uwzoo added a commit to kheiss-uwzoo/nv-ingest that referenced this pull request May 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants