feat: update Docker setup and dependencies for OneVision Encoder#32
feat: update Docker setup and dependencies for OneVision Encoder#32anxiangsir merged 1 commit intomainfrom
Conversation
- Rename Docker image to `onevision-encoder:2601` - Add `--rm` flag to docker run command for cleaner cleanup - Refactor Dockerfile to improve layer caching and reduce size: - Install system deps and static ffmpeg binary - Use requirements.txt for Python dependencies - Set environment variables for better container behavior - Add `torchmetrics` to requirements.txt - Minor formatting fixes in README.md
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Docker setup for the OneVision Encoder project by consolidating Python dependencies into requirements.txt, optimizing the Dockerfile for better caching and reduced image size, and updating Docker naming conventions.
- Adds
torchmetricsdependency to requirements.txt (used by evaluation scripts) - Refactors Dockerfile to use multi-stage layer optimization, install ffmpeg from static binaries, and leverage requirements.txt
- Updates Docker image naming from
ov_encoder:25.12toonevision-encoder:2601and adds--rmflag for automatic cleanup
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| requirements.txt | Adds torchmetrics dependency without version constraint |
| dockerfile | Refactored to optimize layer caching, install system dependencies and ffmpeg in one layer, and use requirements.txt for Python packages |
| README.md | Updates Docker image name and adds --rm flag to docker run command; fixes trailing whitespace on two lines |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Set default command | ||
| CMD ["/bin/bash"] |
There was a problem hiding this comment.
The Dockerfile is missing a WORKDIR directive. The docker run command in the README uses "-w /workspace/OneVision-Encoder" to set the working directory at runtime, but setting a default WORKDIR in the Dockerfile (e.g., "WORKDIR /workspace") would provide better defaults when the container is run without explicit working directory specification.
| && cd /tmp \ | ||
| && curl -L https://johnvansickle.com/ffmpeg/releases/ffmpeg-release-amd64-static.tar.xz -o ffmpeg.tar.xz \ | ||
| && tar -xf ffmpeg.tar.xz \ | ||
| && cd ffmpeg-*-static \ |
There was a problem hiding this comment.
The wildcard pattern "ffmpeg-*-static" in the directory change command could fail if the extracted archive structure changes or if multiple matching directories exist. Consider using a more robust approach, such as storing the extracted directory name in a variable or using "find" to locate the directory reliably.
| && cd ffmpeg-*-static \ | |
| && FFMPEG_DIR="$(find . -maxdepth 1 -type d -name 'ffmpeg-*-static' | head -n 1)" \ | |
| && cd "$FFMPEG_DIR" \ |
| PYTHONUNBUFFERED=1 \ | ||
| PIP_NO_CACHE_DIR=1 | ||
|
|
There was a problem hiding this comment.
The environment variable PIP_NO_CACHE_DIR is set to 1, but pip also uses --no-cache-dir flags in the RUN commands. While this redundancy doesn't cause issues, it's unnecessary. The --no-cache-dir flags in the RUN commands are sufficient, so the PIP_NO_CACHE_DIR environment variable could be removed to simplify the configuration.
| PYTHONUNBUFFERED=1 \ | |
| PIP_NO_CACHE_DIR=1 | |
| PYTHONUNBUFFERED=1 |
| && curl -L https://johnvansickle.com/ffmpeg/releases/ffmpeg-release-amd64-static.tar.xz -o ffmpeg.tar.xz \ | ||
| && tar -xf ffmpeg.tar.xz \ | ||
| && cd ffmpeg-*-static \ | ||
| && install -m 0755 ffmpeg /usr/local/bin/ffmpeg \ | ||
| && install -m 0755 ffprobe /usr/local/bin/ffprobe \ |
There was a problem hiding this comment.
The dockerfile downloads and installs a precompiled ffmpeg binary from https://johnvansickle.com/ffmpeg/releases/ffmpeg-release-amd64-static.tar.xz without any checksum or signature verification. If that host, its DNS, or the network path is compromised, an attacker could supply a malicious ffmpeg/ffprobe binary that is baked into the image and executed at build or runtime, leading to full compromise of containers built from this image. Use a trusted source (e.g., distribution packages or the official project) and pin the artifact with an integrity check (such as a specific version plus SHA256 or a verified signature) before installing it.
|
|
||
| ```bash | ||
| docker run -it --gpus all --ipc host --net host --privileged \ | ||
| docker run -it --rm --gpus all --ipc host --net host --privileged \ |
There was a problem hiding this comment.
The recommended docker run command uses --privileged and --net host, which largely removes container isolation by giving the container broad device and kernel capabilities and direct access to the host network. If the workload inside the container (e.g., ffmpeg or model code processing untrusted inputs) is compromised, an attacker can more easily pivot to the host and other services. Prefer a least-privilege configuration by avoiding --privileged/--net host unless absolutely required and instead granting only the minimal capabilities and network access needed.
| docker run -it --rm --gpus all --ipc host --net host --privileged \ | |
| docker run -it --rm --gpus all --ipc host \ |
onevision-encoder:2601--rmflag to docker run command for cleaner cleanuptorchmetricsto requirements.txt