Containerization Enhancements #1092
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUpdates to Docker build infrastructure and CI: add Python 3.14, introduce GPU interface selector (acc/mp), propagate INTERFACE through builds/tests, add Docker Hub README auto-update workflow, scheduled Docker container tests, Apptainer container runner script, and documentation/tag updates. Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request / Scheduler
participant GA as GitHub Actions
participant Builder as Docker Build (buildx)
participant Registry as Docker Hub
participant Runner as Apptainer / Test Runner
PR->>GA: trigger workflows (docker.yml/docker-test.yml/docker-readme.yml)
GA->>Builder: build images (args: INTERFACE, TARGET)
Builder-->>Registry: push image tags (cpu, gpu-acc, gpu-mp)
GA->>Registry: create/push manifests and latest aliases
GA->>Runner: run container tests (pull image, detect GPUs, run mfc.sh)
Runner->>GA: upload logs / exit status
GA->>Registry: update README (on PR close) via dockerhub-description action
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
bot
added
the
size:L
There was a problem hiding this comment.
Pull request overview
This PR enhances MFC's containerization infrastructure by adding OpenMP GPU support, updating Docker Hub documentation automation, and implementing weekly container testing. The changes enable building GPU containers with either OpenACC or OpenMP parallelization interfaces, upgrade the Python version from 3.10 to 3.14, and establish automated workflows for maintaining Docker Hub documentation and validating container functionality.
Key Changes:
- Added OpenMP GPU variant support (v5.1.0-v5.1.3) alongside existing OpenACC variants via
--gpu mpbuild argument - Implemented automated Docker Hub README updates triggered by changes to
.github/docker_readme.md - Created weekly container testing workflow that validates
cpu,gpu-acc, andgpu-mpcontainers on Phoenix cluster
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
docs/documentation/docker.md |
Updated documentation to reflect OpenMP GPU variants and clarified version-specific base image information |
.github/workflows/phoenix/container.sh |
Added container testing script that pulls appropriate Docker images, builds MFC with correct interface flags, and runs tests |
.github/workflows/docker.yml |
Extended matrix to build separate OpenACC and OpenMP GPU containers, added interface argument to build process |
.github/workflows/docker-test.yml |
Created weekly scheduled workflow to test CPU, GPU-ACC, and GPU-MP containers on Phoenix |
.github/workflows/docker-readme.yml |
Added workflow to automatically update Docker Hub description when README is modified |
.github/docker_readme.md |
Created Docker Hub README with project badges and documentation references |
.github/Dockerfile |
Added INTERFACE argument, upgraded Python to 3.14, configured interface-specific build commands |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| ENV DEBIAN_FRONTEND=noninteractive | ||
| RUN apt-get update && \ | ||
| apt-get install -y software-properties-common wget && \ | ||
| add-apt-repository ppa:deadsnakes/ppa | ||
|
|
||
| RUN apt-get update -y && \ | ||
| if [ "$TARGET" != "gpu" ]; then \ | ||
| apt-get install -y \ | ||
| build-essential git make cmake gcc g++ gfortran bc\ | ||
| python3 python3-venv python3-pip \ | ||
| build-essential git make cmake gcc g++ gfortran bc \ | ||
| python3.14 python3.14-venv \ | ||
| openmpi-bin libopenmpi-dev libfftw3-dev \ | ||
| mpich libmpich-dev; \ | ||
| else \ | ||
| apt-get install -y \ | ||
| build-essential git make cmake bc\ | ||
| python3 python3-venv python3-pip \ | ||
| build-essential git make cmake bc \ | ||
| python3.14 python3.14-venv \ | ||
| libfftw3-dev \ | ||
| openmpi-bin libopenmpi-dev; \ | ||
| fi && \ | ||
| update-alternatives --install /usr/bin/python3 python3 /usr/bin/python3.14 1 && \ | ||
| update-alternatives --set python3 /usr/bin/python3.14 && \ | ||
| python3 --version && \ | ||
| rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* |
There was a problem hiding this comment.
Suggestion: Combine the two RUN instructions for apt-get operations into a single one to reduce Docker image layers and optimize image size. [general, importance: 6]
| ENV DEBIAN_FRONTEND=noninteractive | |
| RUN apt-get update && \ | |
| apt-get install -y software-properties-common wget && \ | |
| add-apt-repository ppa:deadsnakes/ppa | |
| RUN apt-get update -y && \ | |
| if [ "$TARGET" != "gpu" ]; then \ | |
| apt-get install -y \ | |
| build-essential git make cmake gcc g++ gfortran bc\ | |
| python3 python3-venv python3-pip \ | |
| build-essential git make cmake gcc g++ gfortran bc \ | |
| python3.14 python3.14-venv \ | |
| openmpi-bin libopenmpi-dev libfftw3-dev \ | |
| mpich libmpich-dev; \ | |
| else \ | |
| apt-get install -y \ | |
| build-essential git make cmake bc\ | |
| python3 python3-venv python3-pip \ | |
| build-essential git make cmake bc \ | |
| python3.14 python3.14-venv \ | |
| libfftw3-dev \ | |
| openmpi-bin libopenmpi-dev; \ | |
| fi && \ | |
| update-alternatives --install /usr/bin/python3 python3 /usr/bin/python3.14 1 && \ | |
| update-alternatives --set python3 /usr/bin/python3.14 && \ | |
| python3 --version && \ | |
| rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* | |
| ENV DEBIAN_FRONTEND=noninteractive | |
| RUN apt-get update && \ | |
| apt-get install -y software-properties-common wget && \ | |
| add-apt-repository ppa:deadsnakes/ppa && \ | |
| apt-get update -y && \ | |
| if [ "$TARGET" != "gpu" ]; then \ | |
| apt-get install -y \ | |
| build-essential git make cmake gcc g++ gfortran bc \ | |
| python3.14 python3.14-venv \ | |
| openmpi-bin libopenmpi-dev libfftw3-dev \ | |
| mpich libmpich-dev; \ | |
| else \ | |
| apt-get install -y \ | |
| build-essential git make cmake bc \ | |
| python3.14 python3.14-venv \ | |
| libfftw3-dev \ | |
| openmpi-bin libopenmpi-dev; \ | |
| fi && \ | |
| update-alternatives --install /usr/bin/python3 python3 /usr/bin/python3.14 1 && \ | |
| update-alternatives --set python3 /usr/bin/python3.14 && \ | |
| python3 --version && \ | |
| rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* |
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| ENV DEBIAN_FRONTEND=noninteractive | ||
| RUN apt-get update && \ | ||
| apt-get install -y software-properties-common wget && \ | ||
| add-apt-repository ppa:deadsnakes/ppa |
There was a problem hiding this comment.
Suggestion: The separate RUN that installs software-properties-common and calls add-apt-repository leaves apt lists in that layer (increasing image size); combine the repository add/update and cleanup in the same RUN so package lists are removed and not persisted in a previous layer. [resource leak]
Severity Level: Minor
| add-apt-repository ppa:deadsnakes/ppa | |
| add-apt-repository ppa:deadsnakes/ppa && \ | |
| apt-get update -y && \ | |
| rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* |
Why it matters? ⭐
The separate RUN that adds the PPA leaves apt lists in that layer increasing
image size. Cleaning up apt lists in the same RUN (or combining with the
subsequent package-install RUN) prevents persisting package metadata into
an earlier layer and reduces final image size. The suggested change is a
valid optimization and doesn't alter runtime behavior.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** .github/Dockerfile
**Line:** 15:15
**Comment:**
*Resource Leak: The separate RUN that installs `software-properties-common` and calls `add-apt-repository` leaves apt lists in that layer (increasing image size); combine the repository add/update and cleanup in the same RUN so package lists are removed and not persisted in a previous layer.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| cd /opt/MFC && \ | ||
| if [ "$TARGET" = "gpu" ]; then \ | ||
| ./mfc.sh build --gpu -j $(nproc); \ | ||
| ./mfc.sh build --gpu $INTERFACE -j $(nproc); \ |
There was a problem hiding this comment.
Suggestion: Unquoted shell variable expansion for INTERFACE in the build command can cause word-splitting or command injection if the argument contains spaces or shell metacharacters; quote the variable to ensure it's treated as a single argument. [security]
Severity Level: Critical 🚨
| ./mfc.sh build --gpu $INTERFACE -j $(nproc); \ | |
| ./mfc.sh build --gpu "$INTERFACE" -j $(nproc); \ |
Why it matters? ⭐
Quoting "$INTERFACE" in the build command prevents word-splitting and
limits accidental/intentional argument injection when the variable
contains spaces or shell metacharacters. This is a straightforward,
correct hardening for shell invocations and doesn't change logic.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** .github/Dockerfile
**Line:** 52:52
**Comment:**
*Security: Unquoted shell variable expansion for `INTERFACE` in the build command can cause word-splitting or command injection if the argument contains spaces or shell metacharacters; quote the variable to ensure it's treated as a single argument.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| RUN cd /opt/MFC && \ | ||
| if [ "$TARGET" = "gpu" ]; then \ | ||
| ./mfc.sh test -a --dry-run --gpu -j $(nproc); \ | ||
| ./mfc.sh test -a --dry-run --gpu $INTERFACE -j $(nproc); \ |
There was a problem hiding this comment.
Suggestion: Unquoted shell variable expansion for INTERFACE in the test command can cause word-splitting or command injection; quote the variable to pass it safely as a single argument to the script. [security]
Severity Level: Critical 🚨
| ./mfc.sh test -a --dry-run --gpu $INTERFACE -j $(nproc); \ | |
| ./mfc.sh test -a --dry-run --gpu "$INTERFACE" -j $(nproc); \ |
Why it matters? ⭐
Same as the build invocation: quoting "$INTERFACE" ensures the test command
receives a single, literal argument and avoids subtle failures or command
injection vectors. It's a minimal, correct security improvement.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** .github/Dockerfile
**Line:** 59:59
**Comment:**
*Security: Unquoted shell variable expansion for `INTERFACE` in the test command can cause word-splitting or command injection; quote the variable to pass it safely as a single argument to the script.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| gpu_count=$(nvidia-smi -L | wc -l) # number of GPUs on node | ||
| gpu_ids=$(seq -s " " 0 $(($gpu_count-1))) # 0,1,2,...,gpu_count-1 | ||
| device_opts="-g $gpu_ids" |
There was a problem hiding this comment.
Suggestion: If nvidia-smi is missing or returns 0 GPUs, the current arithmetic and seq usage can produce invalid ranges or empty values and cause runtime errors; validate gpu_count is a positive integer before using it (and fall back to a safe default), and use shell arithmetic $((...)) instead of legacy expr for clarity. [possible bug]
Severity Level: Critical 🚨
| gpu_count=$(nvidia-smi -L | wc -l) # number of GPUs on node | |
| gpu_ids=$(seq -s " " 0 $(($gpu_count-1))) # 0,1,2,...,gpu_count-1 | |
| device_opts="-g $gpu_ids" | |
| gpu_count=$(nvidia-smi -L 2>/dev/null | wc -l) | |
| if [ -z "$gpu_count" ] || [ "$gpu_count" -le 0 ]; then | |
| gpu_count=1 | |
| fi | |
| gpu_ids=$(seq -s " " 0 $((gpu_count-1))) | |
| device_opts="-g $gpu_ids" | |
| n_test_threads=$((gpu_count * 2)) |
Why it matters? ⭐
Valid and useful. If nvidia-smi is missing or reports zero, the existing code can produce bad seq ranges or empty GPU lists. The suggested checks and switching to POSIX arithmetic reduce failures and make intent clearer.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** .github/workflows/phoenix/container.sh
**Line:** 38:40
**Comment:**
*Possible Bug: If `nvidia-smi` is missing or returns 0 GPUs, the current arithmetic and `seq` usage can produce invalid ranges or empty values and cause runtime errors; validate `gpu_count` is a positive integer before using it (and fall back to a safe default), and use shell arithmetic `$((...))` instead of legacy `expr` for clarity.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
.github/workflows/phoenix/container.sh (1)
39-41: Modernize arithmetic expressions.The script uses old-style command substitution with backticks and
expr.Apply this diff for more modern bash syntax:
- gpu_ids=$(seq -s " " 0 $(($gpu_count-1))) # 0,1,2,...,gpu_count-1 + gpu_ids=$(seq -s " " 0 $((gpu_count-1))) # 0,1,2,...,gpu_count-1 device_opts="-g $gpu_ids" - n_test_threads=`expr $gpu_count \* 2` + n_test_threads=$((gpu_count * 2))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/Dockerfile(3 hunks).github/docker_readme.md(1 hunks).github/workflows/docker-readme.yml(1 hunks).github/workflows/docker-test.yml(1 hunks).github/workflows/docker.yml(4 hunks).github/workflows/phoenix/container.sh(1 hunks)docs/documentation/docker.md(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Read the full MFC codebase and documentation before changing code to understand the project context and architecture
Applied to files:
.github/docker_readme.md
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Draft a step-by-step plan before making changes; build after each step using `./mfc.sh build -t pre_process simulation -j $(nproc)`
Applied to files:
.github/workflows/phoenix/container.sh.github/Dockerfile
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: After each successful build step, run focused tests using `./mfc.sh test -j $(nproc) -f EA8FA07E -t 9E2CA336` instead of running all tests
Applied to files:
.github/workflows/phoenix/container.sh.github/Dockerfile
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Compile with Cray `ftn` or NVIDIA `nvfortran` for GPU offloading; also build CPU-only with GNU `gfortran` and Intel `ifx`/`ifort` for portability
Applied to files:
.github/workflows/phoenix/container.shdocs/documentation/docker.md.github/Dockerfile
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Ensure GPU code compiles with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort compilers
Applied to files:
.github/workflows/phoenix/container.sh
🪛 actionlint (1.7.9)
.github/workflows/docker-test.yml
15-15: label "gt" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: label "gt" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
23-23: label "gt" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🪛 markdownlint-cli2 (0.18.1)
.github/docker_readme.md
10-10: Images should have alternate text (alt text)
(MD045, no-alt-text)
13-13: Images should have alternate text (alt text)
(MD045, no-alt-text)
16-16: Images should have alternate text (alt text)
(MD045, no-alt-text)
19-19: Images should have alternate text (alt text)
(MD045, no-alt-text)
22-22: Images should have alternate text (alt text)
(MD045, no-alt-text)
25-25: Images should have alternate text (alt text)
(MD045, no-alt-text)
32-32: Images should have alternate text (alt text)
(MD045, no-alt-text)
35-35: Images should have alternate text (alt text)
(MD045, no-alt-text)
🪛 Shellcheck (0.11.0)
.github/workflows/phoenix/container.sh
[warning] 3-3: job_interface is referenced but not assigned.
(SC2154)
[warning] 4-4: job_device is referenced but not assigned.
(SC2154)
[warning] 18-18: job_slug is referenced but not assigned.
(SC2154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Oak Ridge | Frontier (cpu)
- GitHub Check: Georgia Tech | Phoenix (cpu)
- GitHub Check: Oak Ridge | Frontier (gpu-acc)
- GitHub Check: Georgia Tech | Phoenix (gpu-acc)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Oak Ridge | Frontier (gpu-omp)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Georgia Tech | Phoenix (gpu-omp)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Code Cleanliness Check
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Build & Publish
🔇 Additional comments (6)
docs/documentation/docker.md (1)
151-171: LGTM!The documentation clearly explains the new OpenACC/OpenMP distinctions and tag structure. The examples are helpful and align with the container build changes.
.github/workflows/docker.yml (1)
21-25: LGTM! Interface-based matrix expansion is well-structured.The expanded matrix with
interfacefield properly supports the new OpenACC/OpenMP distinction for GPU builds. The separation into distinct configurations (cpu/none, gpu/acc, gpu/mp) aligns well with the documentation and downstream workflows..github/Dockerfile (3)
5-5: LGTM! INTERFACE parameter properly propagated.The INTERFACE build argument is correctly declared, propagated as an environment variable, and used in both build and test commands. This properly supports the OpenACC/OpenMP distinction introduced in this PR.
Also applies to: 42-42, 52-52, 59-59
37-37: Good addition of OpenMPI environment variable.Adding
OMPI_ALLOW_RUN_AS_ROOT_CONFIRM=1alongside the existingOMPI_ALLOW_RUN_AS_ROOT=1properly suppresses OpenMPI warnings when running as root in containers.
21-21: Verify Python 3.14 is released and production-ready.The Dockerfile upgrades to Python 3.14. Confirm this version is officially released and stable enough for your use case before merging, as Python 3.14's availability and production readiness cannot be determined without current information.
Also applies to: 27-27, 31-33
.github/workflows/docker-test.yml (1)
32-33: Node 16 forced in workflow is already ineffective.The workflow forces Node 16 via
ACTIONS_RUNNER_FORCE_ACTIONS_NODE_VERSION: node16, butactions/checkout@v4(the only action in use) requires and will run on Node 20 regardless. The override has no effect on this action. Remove the unnecessary overrides if they're not needed for the self-hosted runner's other operations.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 7 files
Prompt for AI agents (all 4 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name=".github/docker_readme.md">
<violation number="1" location=".github/docker_readme.md:4">
P2: Missing closing `</p>` tag for the paragraph element opened at line 29. This unclosed HTML tag could cause rendering issues on Docker Hub. Add `</p>` after the closing `</a>` tag and before the blank lines.</violation>
</file>
<file name=".github/workflows/docker-readme.yml">
<violation number="1" location=".github/workflows/docker-readme.yml:10">
P1: Missing merge check: This workflow will run for all closed PRs, not just merged ones. Add a condition to ensure it only runs when the PR is actually merged.</violation>
</file>
<file name=".github/workflows/docker.yml">
<violation number="1" location=".github/workflows/docker.yml:146">
P1: Typo: Double hyphen `--` in manifest push command. The manifest was created as `$TAG-mp-gpu` but the push references `$TAG--mp-gpu`, which will cause the workflow to fail.</violation>
</file>
<file name=".github/workflows/docker-test.yml">
<violation number="1" location=".github/workflows/docker-test.yml:22">
P1: Interface value mismatch: workflow passes `mp` but `container.sh` checks for `omp`. This will cause the OpenMP-specific logic to be skipped, resulting in incorrect container selection and build options.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| <p align="center"> | ||
| <a href="http://mflowcode.github.io/"> | ||
| <img src="https://mflowcode.github.io/res/banner.png" alt="MFC Banner" width="500"/> | ||
| </a> |
There was a problem hiding this comment.
P2: Missing closing </p> tag for the paragraph element opened at line 29. This unclosed HTML tag could cause rendering issues on Docker Hub. Add </p> after the closing </a> tag and before the blank lines.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/docker_readme.md, line 4:
<comment>Missing closing `</p>` tag for the paragraph element opened at line 29. This unclosed HTML tag could cause rendering issues on Docker Hub. Add `</p>` after the closing `</a>` tag and before the blank lines.</comment>
<file context>
@@ -0,0 +1,41 @@
+<p align="center">
+ <a href="http://mflowcode.github.io/">
+ <img src="https://mflowcode.github.io/res/banner.png" alt="MFC Banner" width="500"/>
+ </a>
+</p>
+
</file context>
| - '.github/docker_readme.md' | ||
|
|
||
| jobs: | ||
| Container: |
There was a problem hiding this comment.
P1: Missing merge check: This workflow will run for all closed PRs, not just merged ones. Add a condition to ensure it only runs when the PR is actually merged.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/docker-readme.yml, line 10:
<comment>Missing merge check: This workflow will run for all closed PRs, not just merged ones. Add a condition to ensure it only runs when the PR is actually merged.</comment>
<file context>
@@ -0,0 +1,24 @@
+ - '.github/docker_readme.md'
+
+jobs:
+ Container:
+ runs-on: ubuntu-latest
+ steps:
</file context>
✅ Addressed in 4c9a650
| docker manifest create $REGISTRY:$TAG-mp-gpu $REGISTRY:$TAG-gpu-mp-ubuntu-22.04 $REGISTRY:$TAG-gpu-mp-ubuntu-22.04-arm | ||
| docker manifest create $REGISTRY:latest-mp-gpu $REGISTRY:$TAG-gpu-mp-ubuntu-22.04 $REGISTRY:$TAG-gpu-mp-ubuntu-22.04-arm | ||
| docker manifest push $REGISTRY:$TAG--mp-gpu | ||
| docker manifest push $REGISTRY:latest-mp-gpu |
There was a problem hiding this comment.
P1: Typo: Double hyphen -- in manifest push command. The manifest was created as $TAG-mp-gpu but the push references $TAG--mp-gpu, which will cause the workflow to fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/docker.yml, line 146:
<comment>Typo: Double hyphen `--` in manifest push command. The manifest was created as `$TAG-mp-gpu` but the push references `$TAG--mp-gpu`, which will cause the workflow to fail.</comment>
<file context>
@@ -125,8 +130,17 @@ jobs:
+ docker manifest create $REGISTRY:$TAG-mp-gpu $REGISTRY:$TAG-gpu-mp-ubuntu-22.04 $REGISTRY:$TAG-gpu-mp-ubuntu-22.04-arm
+ docker manifest create $REGISTRY:latest-mp-gpu $REGISTRY:$TAG-gpu-mp-ubuntu-22.04 $REGISTRY:$TAG-gpu-mp-ubuntu-22.04-arm
+ docker manifest push $REGISTRY:$TAG--mp-gpu
+ docker manifest push $REGISTRY:latest-mp-gpu
</file context>
✅ Addressed in 4c9a650
| - lbl: 'gt' | ||
| cluster_name: 'Georgia Tech | Phoenix' | ||
| device: 'gpu' | ||
| interface: 'mp' |
There was a problem hiding this comment.
P1: Interface value mismatch: workflow passes mp but container.sh checks for omp. This will cause the OpenMP-specific logic to be skipped, resulting in incorrect container selection and build options.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/docker-test.yml, line 22:
<comment>Interface value mismatch: workflow passes `mp` but `container.sh` checks for `omp`. This will cause the OpenMP-specific logic to be skipped, resulting in incorrect container selection and build options.</comment>
<file context>
@@ -0,0 +1,44 @@
+ - lbl: 'gt'
+ cluster_name: 'Georgia Tech | Phoenix'
+ device: 'gpu'
+ interface: 'mp'
+ - lbl: 'gt'
+ cluster_name: 'Georgia Tech | Phoenix'
</file context>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
.github/workflows/docker.yml (1)
136-146: Tag naming inconsistency between ACC and MP variants.ACC manifests use
$TAG-gpu/latest-gpu, while MP manifests use$TAG-mp-gpu/latest-mp-gpu. This asymmetry could cause confusion—users might expectlatest-gpu-accandlatest-gpu-mpas a parallel scheme.If backward compatibility with existing
gputags is required, this is acceptable. Otherwise, consider standardizing to$TAG-gpu-acc/latest-gpu-accfor ACC as well..github/workflows/phoenix/container.sh (3)
1-7: Add error handling and document expected environment variables.The script lacks
set -e(orset -euo pipefail) to fail fast on errors. Ifapptainer buildfails, subsequent commands will run against a missing/broken image.Additionally,
job_interfaceandjob_deviceare expected as environment variables from the calling workflow but should be validated.Apply this diff:
#!/bin/bash +set -euo pipefail + +# Expected environment variables from calling workflow: +# job_device: cpu or gpu +# job_interface: none, acc, or mp +# job_slug: test identifier +# SCRATCH_DIR: scratch directory path + +: "${job_device:?job_device is required}" +: "${job_interface:?job_interface is required}" if [ "$job_interface" = "mp" ]; then
16-20: Critical:job_interfacenot passed into the container.The
apptainer execcommand passesjob_slugandjob_devicevia--env, butjob_interfaceis missing. The inner script (lines 26-30) references$job_interfacewhich will be empty/unset inside the container, causing the GPU interface selection to fail silently.Apply this diff:
apptainer exec $NV_FLAG --fakeroot --writable-tmpfs \ --bind "$SCRATCH_DIR":/scratch \ --env job_slug="$job_slug" \ --env job_device="$job_device" \ + --env job_interface="$job_interface" \ "$CONTAINER" \
37-42: Add validation for GPU detection to prevent runtime errors.If
nvidia-smiis unavailable or returns 0 GPUs, theseqcommand will produce invalid ranges (e.g.,seq 0 -1) andexprwill fail. This could cause cryptic failures on nodes without expected GPU configuration.Apply this diff:
if [ "$job_device" = "gpu" ]; then - gpu_count=$(nvidia-smi -L | wc -l) # number of GPUs on node - gpu_ids=$(seq -s " " 0 $(($gpu_count-1))) # 0,1,2,...,gpu_count-1 + gpu_count=$(nvidia-smi -L 2>/dev/null | wc -l) || gpu_count=0 + if [ "$gpu_count" -le 0 ]; then + echo "Error: No GPUs detected" >&2 + exit 1 + fi + gpu_ids=$(seq -s " " 0 $((gpu_count - 1))) device_opts="-g $gpu_ids" - n_test_threads=`expr $gpu_count \* 2` + n_test_threads=$((gpu_count * 2)) fi
🧹 Nitpick comments (1)
.github/workflows/phoenix/container.sh (1)
44-45:$device_optsmay be unset for CPU runs causing potential issues.When
job_deviceiscpu, thedevice_optsvariable is never initialized, and its unquoted expansion on line 44 could cause issues withset -uif adopted. Initialize it to an empty string.Add initialization before the conditional:
n_test_threads=8 +device_opts="" if [ "$job_device" = "gpu" ]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/docker-readme.yml(1 hunks).github/workflows/docker.yml(4 hunks).github/workflows/phoenix/container.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/docker-readme.yml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Draft a step-by-step plan before making changes; build after each step using `./mfc.sh build -t pre_process simulation -j $(nproc)`
Applied to files:
.github/workflows/phoenix/container.sh
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: After each successful build step, run focused tests using `./mfc.sh test -j $(nproc) -f EA8FA07E -t 9E2CA336` instead of running all tests
Applied to files:
.github/workflows/phoenix/container.sh
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Compile with Cray `ftn` or NVIDIA `nvfortran` for GPU offloading; also build CPU-only with GNU `gfortran` and Intel `ifx`/`ifort` for portability
Applied to files:
.github/workflows/phoenix/container.sh
🪛 Shellcheck (0.11.0)
.github/workflows/phoenix/container.sh
[warning] 3-3: job_interface is referenced but not assigned.
(SC2154)
[warning] 4-4: job_device is referenced but not assigned.
(SC2154)
[warning] 18-18: job_slug is referenced but not assigned.
(SC2154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Oak Ridge | Frontier (gpu-acc)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Build & Publish
🔇 Additional comments (3)
.github/workflows/docker.yml (3)
21-25: LGTM! Matrix configuration correctly defines GPU interface variants.The matrix properly introduces the
interfaceparameter distinguishing betweenacc(OpenACC) andmp(OpenMP) GPU variants alongside the CPU configuration.
87-96: LGTM! INTERFACE build argument properly propagated for CPU builds.The INTERFACE variable is correctly passed to the Dockerfile.
106-116: LGTM! GPU build correctly includes interface in tags.The tag format
$TAG-gpu-$interface-$runner(e.g.,v5.1.3-gpu-acc-ubuntu-22.04) clearly identifies the variant.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1092 +/- ##
=======================================
Coverage 44.08% 44.08%
=======================================
Files 71 71
Lines 20332 20332
Branches 1981 1981
=======================================
Hits 8963 8963
Misses 10236 10236
Partials 1133 1133 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
due to inactivity |
1 participant
Description
Concerning #1081 and replacing #1080,
Docker Readme Update: intended to update the docker hub description on the main page. It is only triggered by merged PRs modifying
.github/docker_readme.mdwhich would take about ~10 s to execute. The test for it was by merging a branch with negligible changes which yielded this workflow run.Containerization: added OpenMP variants (v5.1.0-v5.1.3) via simply the argument
--gpu mp, along with upgrading python 3.10->3.14. Also, I manually pushed v5.1.3 for failing recently. The test was performed by ensuring all five containerization processes(cpu(amd64&arm64), gpu-acc-amd64, gpu-acc-arm64, gpu-mp-amd64, gpu-mp-arm64)take place and would be pushed neatly to docker hub. Expected behavior is similar to this run.Test Docker Containers: intended to grab the latest
cpu,gpu-acc, andgpu-mpcontainers and run them through all tests on Phoenix weekly which should match the pace of frequent MFC release posts. The test was confirmed to behave as anticipated in this workflow run of PR [Experimental PR] Self-Hosted Container Tests #1088.Docs: updated relevant docs as needed.
PR Type
Enhancement
Description
Add OpenMP GPU variant support alongside existing OpenACC containers
Introduce weekly Docker container testing workflow on Phoenix cluster
Update Dockerfile with Python 3.14 and INTERFACE build argument
Create Docker Hub readme automation and update documentation
Diagram Walkthrough
File Walkthrough
docker.yml
Extend build matrix with OpenMP GPU variants.github/workflows/docker.yml
interfaceparameter to build matrix for OpenACC and OpenMPvariants
arm64
Dockerfile
Add Python 3.14 and INTERFACE build support.github/Dockerfile
docker-readme.yml
Automate Docker Hub description updates.github/workflows/docker-readme.yml
Hub
docker-test.yml
Add weekly Docker container testing workflow.github/workflows/docker-test.yml
container.sh
Add Phoenix container test execution script.github/workflows/phoenix/container.sh
interface
count
docker_readme.md
Create Docker Hub readme file.github/docker_readme.md
codecov
docker.md
Document OpenACC and OpenMP GPU variantsdocs/documentation/docker.md
CodeAnt-AI Description
Publish and test CPU/GPU images with OpenACC and OpenMP variants; switch containers to Python 3.14
What Changed
Impact
✅ Clearer GPU image choices (OpenACC vs OpenMP)✅ Weekly container test coverage on Phoenix✅ Containers run with Python 3.14 by default💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.