Add dockerfile#590
Merged
Merged
Conversation
Contributor
Reviewer's GuideIntroduce a Dockerfile that sets up system dependencies, a conda-based pymeep environment, a uv virtualenv for the Python application, installs all required packages, and specifies uvicorn as the container's startup command, along with a placeholder .dockerignore. Sequence diagram for Container Startup ProcesssequenceDiagram
participant DE as Docker Engine
participant C as Container
participant Proc as Startup Process
participant Uvicorn as Uvicorn Server
participant App as "gplugins.server:app"
DE->>+C: Start Container
C->>+Proc: Execute CMD ["uvicorn", "gplugins.server:app", "--host", "0.0.0.0", "--port", "8000"]
Proc->>+Uvicorn: Launch Uvicorn
Uvicorn->>+App: Load application 'gplugins.server:app'
App-->>-Uvicorn: Application loaded
Uvicorn->>Uvicorn: Start HTTP server
Uvicorn->>Uvicorn: Listen on 0.0.0.0:8000
Proc-->>-C: Process running
C-->>-DE: Container running
Flow diagram for Docker Image Build Processgraph TD
A[Start: FROM astral/uv:python3.11-bookworm-slim] --> B(Install system dependencies via apt-get)
B --> C(Install Miniconda to /opt/conda)
C --> D(Add conda to PATH)
D --> E(Create conda environment 'pymeep' with Python 3.11)
E --> F(Add 'pymeep' env to PATH for subsequent RUN commands)
F --> G(Install pymeep & nlopt into 'pymeep' via conda)
G --> H(Create uv virtual environment '/opt/venv')
H --> I(Add '/opt/venv/bin' to PATH)
I --> J(Set WORKDIR to /app)
J --> K(COPY application code to /app)
K --> L(Install Python application dependencies via 'uv pip install')
L --> M(Install uvicorn via 'uv pip install')
M --> N(Set CMD to run Uvicorn with gplugins.server:app)
N --> O[End: Docker Image Ready]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey @owls-on-wires - I've reviewed your changes - here's some feedback:
Blocking issues:
- By not specifying a USER, a program in the container may run as 'root'. This is a security hazard. If an attacker can control a process running as root, they may have control over the container. Ensure that the last USER in a Dockerfile is a USER other than 'root'. (link)
General comments:
- Pin the base image and Miniconda installer to specific versions to ensure reproducible Docker builds.
- Combine related RUN steps or use a multi-stage build to reduce the number of layers and shrink the final image size.
- Populate .dockerignore with common patterns (e.g. .git, pycache, venv folders) to avoid sending unneeded files in the build context.
Here's what I looked at during the review
- 🟡 General issues: 6 issues found
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| FROM --platform=linux/amd64 ghcr.io/astral-sh/uv:python3.11-bookworm-slim | ||
|
|
||
| # Install system dependencies including KLayout | ||
| RUN apt-get update && apt-get install -y \ |
Contributor
There was a problem hiding this comment.
suggestion: Set non-interactive frontend and use --no-install-recommends
This ensures the build runs without prompts and reduces image size by excluding unnecessary packages.
Comment on lines
+29
to
+35
| # Create conda environment with Python 3.11 (matching the base image) | ||
| RUN conda create -n pymeep python=3.11 -y | ||
| RUN echo "source activate pymeep" >> ~/.bashrc | ||
| ENV PATH="/opt/conda/envs/pymeep/bin:${PATH}" | ||
|
|
||
| # Install conda packages in the pymeep environment | ||
| RUN conda install -n pymeep -c conda-forge pymeep=*=mpi_mpich_* nlopt -y |
Contributor
There was a problem hiding this comment.
suggestion: Merge conda create and install steps
This will streamline the Docker image and optimize build performance.
Suggested change
| # Create conda environment with Python 3.11 (matching the base image) | |
| RUN conda create -n pymeep python=3.11 -y | |
| RUN echo "source activate pymeep" >> ~/.bashrc | |
| ENV PATH="/opt/conda/envs/pymeep/bin:${PATH}" | |
| # Install conda packages in the pymeep environment | |
| RUN conda install -n pymeep -c conda-forge pymeep=*=mpi_mpich_* nlopt -y | |
| # Create conda environment with Python 3.11 (matching the base image) and install packages | |
| RUN conda create -n pymeep python=3.11 -c conda-forge pymeep=*=mpi_mpich_* nlopt -y | |
| RUN echo "source activate pymeep" >> ~/.bashrc | |
| ENV PATH="/opt/conda/envs/pymeep/bin:${PATH}" |
Comment on lines
+34
to
+37
| # Install conda packages in the pymeep environment | ||
| RUN conda install -n pymeep -c conda-forge pymeep=*=mpi_mpich_* nlopt -y | ||
|
|
||
| # Create and activate uv virtual environment |
Contributor
There was a problem hiding this comment.
suggestion (performance): Clean up conda caches to shrink the image
Run 'conda clean --all -y' after installation to remove caches and reduce image size.
Suggested change
| # Install conda packages in the pymeep environment | |
| RUN conda install -n pymeep -c conda-forge pymeep=*=mpi_mpich_* nlopt -y | |
| # Create and activate uv virtual environment | |
| # Install conda packages in the pymeep environment | |
| RUN conda install -n pymeep -c conda-forge pymeep=*=mpi_mpich_* nlopt -y && \ | |
| conda clean --all -y | |
| # Create and activate uv virtual environment |
|
|
||
| # Create conda environment with Python 3.11 (matching the base image) | ||
| RUN conda create -n pymeep python=3.11 -y | ||
| RUN echo "source activate pymeep" >> ~/.bashrc |
Contributor
There was a problem hiding this comment.
issue: Sourcing ~/.bashrc won't affect non-interactive RUNs
Docker RUN steps don't source ~/.bashrc. Use 'conda run -n pymeep ' or set SHELL to a login shell if environment activation is needed during build.
| COPY . . | ||
|
|
||
| # Install the rest of your requirements | ||
| RUN uv pip install -e ".[dev,docs,femwell,gmsh,meow,sax,tidy3d,klayout,vlsir]" |
Contributor
There was a problem hiding this comment.
suggestion (performance): Combine pip install commands into one layer
Merging both installs into one line will reduce image layers and improve build speed.
|
|
||
| # Install the rest of your requirements | ||
| RUN uv pip install -e ".[dev,docs,femwell,gmsh,meow,sax,tidy3d,klayout,vlsir]" | ||
| RUN uv pip install uvicorn |
Contributor
There was a problem hiding this comment.
suggestion: Pin pip dependencies to fixed versions
Using fixed versions or a requirements file helps ensure consistent builds and avoids issues from upstream changes.
Suggested implementation:
# Copy requirements with pinned versions
COPY requirements.txt .
# Install the rest of your requirements with pinned versions
RUN uv pip install -r requirements.txt
# Set the default command
- You need to generate a
requirements.txtfile with all dependencies and their pinned versions (including extras and uvicorn). You can do this locally with:pip install -e ".[dev,docs,femwell,gmsh,meow,sax,tidy3d,klayout,vlsir]" pip freeze > requirements.txt pip show uvicorn | grep Version # Note the version and ensure it's in requirements.txt - Add
requirements.txtto your repository so it can be copied in the Docker build. - Remove the now-unnecessary
uv pip install uvicornline, asuvicornshould be included and pinned inrequirements.txt.
| RUN uv pip install uvicorn | ||
|
|
||
| # Set the default command | ||
| CMD ["uvicorn", "gplugins.server:app", "--host", "0.0.0.0", "--port", "8000"] No newline at end of file |
Contributor
There was a problem hiding this comment.
security (missing-user): By not specifying a USER, a program in the container may run as 'root'. This is a security hazard. If an attacker can control a process running as root, they may have control over the container. Ensure that the last USER in a Dockerfile is a USER other than 'root'.
Suggested change
| CMD ["uvicorn", "gplugins.server:app", "--host", "0.0.0.0", "--port", "8000"] | |
| USER non-root | |
| CMD ["uvicorn", "gplugins.server:app", "--host", "0.0.0.0", "--port", "8000"] |
Source: opengrep
joamatab
approved these changes
May 28, 2025
joamatab
left a comment
Contributor
There was a problem hiding this comment.
Awesome! thank you Chandler!
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
2 participants
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.
Dockerfile (x86, no ARM yet) includes Meep. Could be used as the starting point for a REST API wrapper over the gplugins, allowing it to be deployed anywhere.
Summary by Sourcery
Add Dockerfile and .dockerignore to enable containerized deployment of the gplugins REST API server with Meep integration on Linux/AMD64.
New Features:
Build: