Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the container infrastructure to optimize the build process for EMOD3D. By introducing a two-stage approach with a pre-built bootstrap container and a minimal runner definition, the changes aim to create a more efficient and self-contained workflow for generating the final Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the container build process by splitting it into a bootstrap Docker image and a minimal Apptainer runner definition. This is a good approach to streamline the workflow. My review focuses on improving the reproducibility of the builds and fixing a critical issue where compiled binaries were being deleted.
Key feedback points:
- Pinning base image versions in both the
Dockerfileandrunner.defto ensure reproducible builds. - Correcting a critical bug in
runner.defwhere compiled binaries were removed during a cleanup step. - A minor cleanup in an environment variable definition.
There was a problem hiding this comment.
Pull request overview
Updates the container build approach to split “bootstrap” dependencies into a publishable Docker image, and keep the private EMOD3D install/build step in a lightweight Apptainer runner.def, enabling self-contained workflow runs without distributing large .sif artifacts.
Changes:
- Refactors
container/runner.defto build EMOD3D on top of a prebuilt bootstrap image. - Adds a new
container/Dockerfileto build the publishable bootstrap Docker image (toolchain, Python, Rust, workflow install). - Replaces the GitHub Actions workflow to build/push the Docker image and run tests via
docker runinstead of building/uploading an Apptainer image.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
container/runner.def |
Switches Apptainer base to a prebuilt bootstrap image and builds EMOD3D from a provided source directory. |
container/Dockerfile |
Introduces a bootstrap Docker image build (system deps, Python from source, Rust, venv, installs workflow). |
.github/workflows/container.yml |
Updates CI to build/push the bootstrap Docker image to Docker Hub and run pytest in a container. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| export RUSTUP_HOME=/sw/rust | ||
| export CARGO_HOME=/sw/cargo | ||
| export PATH="/sw/venv/bin:/EMOD3D/tools:/sw/cargo/bin:/sw/python/bin:$PATH" | ||
| export VIRTUAL_ENV="/sw/venv" |
| RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y | ||
|
|
||
| # Setup Virtual Environment and Install via pip | ||
| ARG WORKFLOW_BRANCH=pegasus |
| docker run --rm \ | ||
| -v ${{ github.workspace }}:/mnt \ | ||
| ${{ env.IMAGE_NAME }}:sha-${{ github.sha }} \ | ||
| bash -c "pytest /mnt/tests" |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Updates the container build pipeline to reflect my recent usage of the infrastructure. The idea is to separate the two containers into:
Then a simple flow.cylc workflow stage can actually produce the runner.sif file immediately like so
the main advantage of this is that the entire workflow is self-contained and we do not have to shuttle around large
runner.siffiles.