Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Python dependency management in the Docker build by installing uv (a fast Python package installer) and switching from pip install to uv sync. The changes also configure the appropriate environment variables to support uv's virtual environment setup.
Key Changes:
- Install uv package manager using the official installation script
- Update PATH to include uv binary location and virtual environment binaries
- Replace
pip installworkflow withuv syncfor dependency management
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ENV OPTICKS_BUILD=/opt/eic-opticks/build | ||
| ENV LD_LIBRARY_PATH=${OPTICKS_PREFIX}/lib:${LD_LIBRARY_PATH} | ||
| ENV PATH=${OPTICKS_PREFIX}/bin:${PATH} | ||
| ENV PATH=${OPTICKS_PREFIX}/bin:~/.local/bin:$OPTICKS_HOME/.venv/bin:${PATH} |
There was a problem hiding this comment.
The tilde ~ in the PATH will not expand correctly in the Dockerfile ENV directive. Docker's ENV does not expand ~ to the home directory. Since the uv installation script installs to $HOME/.local/bin, you should either use an absolute path or the $HOME variable instead.
Suggested fix: Replace ~/.local/bin with /root/.local/bin or $HOME/.local/bin
| ENV PATH=${OPTICKS_PREFIX}/bin:~/.local/bin:$OPTICKS_HOME/.venv/bin:${PATH} | |
| ENV PATH=${OPTICKS_PREFIX}/bin:$HOME/.local/bin:$OPTICKS_HOME/.venv/bin:${PATH} |
| WORKDIR $OPTICKS_HOME | ||
|
|
||
| # Install Python dependencies | ||
| COPY pyproject.toml $OPTICKS_HOME/ |
There was a problem hiding this comment.
The uv sync command requires a uv.lock file to be present, but the Dockerfile only copies pyproject.toml. Without the lock file, uv sync will fail. You should either:
- Add
COPY uv.lock $OPTICKS_HOME/before this line, or - Use
uv pip install -e .instead if you want to install without a lock file
Note: If using uv sync, you should also ensure uv.lock is committed to your repository and updated with uv lock when dependencies change.
| COPY pyproject.toml $OPTICKS_HOME/ | |
| COPY pyproject.toml $OPTICKS_HOME/ | |
| COPY uv.lock $OPTICKS_HOME/ |
|
|
||
| RUN mkdir -p /opt/optix && curl -sL https://github.com/NVIDIA/optix-dev/archive/refs/tags/v${OPTIX_VERSION}.tar.gz | tar -xz --strip-components 1 -C /opt/optix | ||
|
|
||
| RUN curl -LsSf https://astral.sh/uv/install.sh | sh |
There was a problem hiding this comment.
The uv installation at line 49 runs before the SHELL directive at line 51 switches to bash. The install script installs uv to ~/.local/bin and modifies shell initialization files, but this may not take effect properly since the SHELL is changed afterwards. Additionally, since ~/.local/bin needs to be in PATH (see line 72), you should ensure the uv installation completes successfully.
Consider adding a verification step after installation, such as:
RUN curl -LsSf https://astral.sh/uv/install.sh | sh && \
/root/.local/bin/uv --version| RUN curl -LsSf https://astral.sh/uv/install.sh | sh | |
| RUN curl -LsSf https://astral.sh/uv/install.sh | sh && /root/.local/bin/uv --version |
No description provided.