Skip to content

MFC Containerization #971

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Malmahrouqi3
Copy link
Collaborator

@Malmahrouqi3 Malmahrouqi3 commented Jul 29, 2025

User description

Description

Replacing (#935), since it was overly inefficient workflow without yielding any tangible advantages.

Notes to Self:

  • MFC Docker Repo Overview (README)
  • Docker Desktop GUI to run containers.
image
  • SLURM job script templates
  • Config case (case.py) and run/profile on a cluster.
  • Checkout Resource constraints if useful.
  • Obsoleteness vs. Potential of QEMU to support more systems.
    tests/UUID     (s)      Summary
QEMU Emulation
     8F78E60C     46.35    1D -> MHD -> HLLD
     2ADA983F    135.76    2D -> MHD -> HLLD
Native Arch Support
     8F78E60C      3.17    1D -> MHD -> HLLD
     2ADA983F      8.79    2D -> MHD -> HLLD
---------------------------------------------------
    "supported": [
      "linux/amd64",
      "linux/amd64/v2",
      "linux/amd64/v3",
      "linux/arm64",
      "linux/riscv64",
      "linux/ppc64le",
      "linux/s390x",
      "linux/386",
      "linux/mips64le",
      "linux/mips64",
      "linux/loong64",
      "linux/arm/v7",
      "linux/arm/v6"
    ],
    "emulators": [
      "cli",
      "llvm-13-runtime.binfmt",
      "llvm-14-runtime.binfmt",
      "llvm-15-runtime.binfmt",
      "python3.10",
      "qemu-aarch64",
      "qemu-arm",
      "qemu-loongarch64",
      "qemu-mips64",
      "qemu-mips64el",
      "qemu-ppc64le",
      "qemu-riscv64",
      "qemu-s390x"
    ]
  • Improve User Guide

Containerization of MFC Releases (v4.3.0-v5.0.5)

├── .github/
│   ├── Dockerfile           # Contains docker build Instructions
│   ├── .dockerignore        # Contains a list of files and directories to exclude in the docker.
│   └── workflows/
│          └── docker.yml    # Builds and deploys images triggered by MFC release tag input.
│          └── release.yml   # Triggers docker.yml to containerize a range of releases specified by user
  1. Fork https://github.com/Malmahrouqi3/MFC-mo2.git --branch docker, and add credentials to its GitHub secrets.
  2. On GitHub Actions, go to Release Dispatcher Workflow (release.yml), then hit Run workflow and specify a range of desired releases. Except the first few published releases where --gpu flag was not defined yet, the rest should build without any issues. Repeated use of Release Dispatcher for debugging could exhaust docker limits.
    e.g. "You have reached your pull rate limit as"
  3. On Docker Huh, keep track of pushed releases.

Setup MFC Container on Codespaces

├── .devcontainer/
│   └── devcontainer.json    # Contains codespace configuration to load MFC docker container

This is intended to pre-configure repo codespace instances to load MFC docker container for users to interact with instantaneously.

  1. Create a .devcontainer folder in the repo root, and add a devcontainer.json file.
{
  "name": "MFC Dev Container",
  "image": "sbryngelson/mfc:<tag>",
  "workspaceFolder": "/opt/MFC",
  "settings": {
    "terminal.integrated.shell.linux": "/bin/bash",
    "editor.formatOnSave": true
  },
}

Quick Start Guide

  1. Run an MFC container

GitHub Codespaces

  • Start a codespace on the repository and a container will be loaded to interact with.

Locally

  • Pull from Docker Hub and run the MFC container.
docker run -it --rm sbryngelson/mfc:v5.0.5-cpu
  1. Familiarize yourself with MFC:
  • Run the Test Suite
./mfc.sh test
  • Run a simulation case
./mfc.sh run examples/2D_shockbubble/case.py

User Guide

Make sure to mount a directory to mnt inside the container to easily share files between the host and the container, e.g. cp -r <source> /mnt/.

Docker CLI

Official Documentation
Use it for running and testing containers on your local machine.
Example Usage:

docker run -it --rm -v "$PWD":/mnt sbryngelson/mfc:<version>-<type: cpu/gpu>

For Portability,
On the source machine,

  • Pull and save the image
docker pull sbryngelson/mfc:<tag>
docker save -o mfc:<tag>.tar sbryngelson/mfc:<tag>

On the target machine,

  • Load and run the image
docker load -i mfc:<tag>.tar
docker run -it --rm mfc:<tag>

Apptainer CLI

Official Documentation
Use it for running containers on powerful machines and HPC clusters on either interactive terminals or batch jobs.
Example Usage:

apptainer exec --fakeroot --writable-tmpfs --bind "$PWD":/mnt  docker://sbryngelson/mfc:<version>-<type: cpu/gpu> bash -c "cd /opt/MFC && bash"

or

apptainer shell --fakeroot --writable-tmpfs --bind "$PWD":/mnt  docker://sbryngelson/mfc:<tag>
Apptainer>cd /opt/MFC

--fakeroot --writable-tmpfs is critical to:

  • Enable root-like permissions inside the container without actual root access
  • Allow temporary write access to the container filesystem

For Portability,
On the source machine,

  • Pull and translate the image into .sif format
apptainer build mfc:<tag>.sif docker://sbryngelson/mfc:<tag>

On the target machine,

  • Load and start an interactive shell
apptainer shell --fakeroot --writable-tmpfs --bind "$PWD":/mnt mfc:<tag>.sif

PR Type

Enhancement


Description

  • Add Docker containerization support for MFC

  • Create CPU and GPU container variants

  • Implement automated container builds on releases

  • Configure Docker Hub deployment workflow


Diagram Walkthrough

flowchart LR
  A["Source Code"] --> B["Docker Build"]
  B --> C["CPU Container"]
  B --> D["GPU Container"]
  C --> E["Docker Hub"]
  D --> E["Docker Hub"]
  F["GitHub Release"] --> B
Loading

File Walkthrough

Relevant files
Configuration changes
.dockerignore
Docker ignore configuration for clean builds                         

.github/.dockerignore

  • Add comprehensive Docker ignore patterns
  • Exclude build artifacts, temporary files, and examples output
  • Include specific exceptions for required data files
+83/-0   
Enhancement
Dockerfile
Multi-architecture Dockerfile for MFC containers                 

.github/Dockerfile

  • Create multi-stage Dockerfile with CPU/GPU variants
  • Install dependencies based on target architecture
  • Build MFC with appropriate compilers and run tests
  • Set up working environment with proper paths
+55/-0   
docker.yml
Automated container build and deployment workflow               

.github/workflows/docker.yml

  • Add GitHub Actions workflow for container builds
  • Support both release triggers and manual dispatch
  • Build CPU (Ubuntu) and GPU (NVIDIA HPC SDK) variants
  • Deploy containers to Docker Hub with version tags
+65/-0   


PR Type

Enhancement


Description

  • Add Docker containerization support for MFC releases

  • Create GitHub Actions workflow for automated container builds

  • Configure development container for Codespaces integration

  • Support both CPU and GPU container variants


Diagram Walkthrough

flowchart LR
  A["GitHub Release"] --> B["Docker Workflow"]
  B --> C["Build CPU Container"]
  B --> D["Build GPU Container"]
  C --> E["Push to DockerHub"]
  D --> E
  F["Devcontainer Config"] --> G["Codespaces Integration"]
Loading

File Walkthrough

Relevant files
Configuration changes
devcontainer.json
Add Codespaces development container configuration             

.devcontainer/devcontainer.json

  • Configure development container for GitHub Codespaces
  • Set MFC Docker image as base container
  • Define workspace folder and editor settings
+9/-0     
.dockerignore
Add Docker ignore file for build optimization                       

.github/.dockerignore

  • Exclude build artifacts and temporary files from Docker context
  • Ignore example outputs, logs, and generated files
  • Exclude development tools and cache directories
+82/-0   
Enhancement
Dockerfile
Add Dockerfile for MFC containerization                                   

.github/Dockerfile

  • Create multi-stage Dockerfile supporting CPU and GPU builds
  • Install dependencies based on target architecture
  • Build MFC with appropriate compiler configurations
  • Set up runtime environment and entrypoint
+55/-0   
docker.yml
Add GitHub Actions workflow for container builds                 

.github/workflows/docker.yml

  • Implement automated Docker build workflow
  • Support matrix builds for CPU/GPU and ARM/x64 architectures
  • Create manifest lists for multi-platform support
  • Deploy containers to DockerHub on releases
+98/-0   

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 Security concerns

Sensitive information exposure:
The workflow uses Docker Hub credentials stored in GitHub secrets, which is appropriate. However, there's no validation that these secrets exist before attempting to use them, which could lead to failed deployments or exposure of error messages containing sensitive information. Additionally, the workflow runs with elevated permissions (sudo) when staging files, which could be a security concern if the repository is compromised.

⚡ Recommended focus areas for review

Security Risk

Docker Hub credentials are referenced but may not be properly secured. The workflow assumes secrets are available but there's no validation or fallback handling if they're missing.

uses: docker/login-action@v3
with:
  username: ${{ secrets.DOCKERHUB_USERNAME }}
  password: ${{ secrets.DOCKERHUB_PASSWORD }}
Possible Issue

The conditional logic for GPU vs CPU builds may fail if TARGET variable is not exactly 'gpu'. The string comparison should be more robust or use explicit equality checks.

if [ "$TARGET" != "gpu" ]; then \
    apt-get install -y \
        build-essential git make cmake gcc g++ gfortran \
        python3 python3-venv python3-pip \
        openmpi-bin libopenmpi-dev libfftw3-dev \
        libatlas-base-dev mpich libmpich-dev; \
else \
    apt-get install -y \
        build-essential git make cmake \
        python3 python3-venv python3-pip \
        libfftw3-dev libatlas-base-dev \
        openmpi-bin libopenmpi-dev; \
fi && \
Logic Issue

The TAG environment variable setup logic may result in undefined behavior when neither manual tag input nor release tag is available, potentially causing deployment failures.

if [ -n "${{ github.event.inputs.tag }}" ]; then
  echo "TAG=${{ github.event.inputs.tag }}" >> $GITHUB_ENV
elif [[ "${{ github.ref }}" == refs/tags/* ]]; then
  echo "TAG=${{ github.ref_name }}" >> $GITHUB_ENV
fi

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add fallback tag value

The workflow doesn't set a default TAG value when neither condition is met,
which could result in an empty tag. Add a fallback to use a default tag like
'latest' or the commit SHA to ensure containers are always properly tagged.

.github/workflows/docker.yml [33-38]

 run: |
   if [ -n "${{ github.event.inputs.tag }}" ]; then
     echo "TAG=${{ github.event.inputs.tag }}" >> $GITHUB_ENV
   elif [[ "${{ github.ref }}" == refs/tags/* ]]; then
     echo "TAG=${{ github.ref_name }}" >> $GITHUB_ENV
+  else
+    echo "TAG=latest" >> $GITHUB_ENV
   fi
Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies that env.TAG could be empty on a workflow_dispatch trigger without a tag input, which would lead to an invalid Docker tag and cause the build to fail. Adding a fallback is a crucial fix for workflow robustness.

Medium
Fix library path concatenation

The LD_LIBRARY_PATH concatenation could result in a trailing colon if
LD_LIBRARY_PATH is empty, which may cause library loading issues. Use a more
robust approach to handle empty values.

.github/Dockerfile [37]

-ENV LD_LIBRARY_PATH="${COMPILER_LD_LIBRARY_PATH}:${LD_LIBRARY_PATH:-}"
+ENV LD_LIBRARY_PATH="${COMPILER_LD_LIBRARY_PATH}${LD_LIBRARY_PATH:+:${LD_LIBRARY_PATH}}"
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that the current concatenation can lead to a trailing colon if LD_LIBRARY_PATH is unset, which is poor practice. The proposed change using shell parameter expansion is more robust and idiomatic for path manipulation.

Low
Add error handling for tests

The test command may fail and cause the build to stop, but there's no error
handling. Consider adding || true to allow the build to continue even if tests
fail, or use set +e to disable exit-on-error for this step.

.github/Dockerfile [47-52]

 RUN cd /opt/MFC && \
     if [ "$TARGET" = "gpu" ]; then \
-      ./mfc.sh test --dry-run --gpu -j $(nproc); \
+      ./mfc.sh test --dry-run --gpu -j $(nproc) || true; \
     else \
-      ./mfc.sh test --dry-run -j $(nproc); \
+      ./mfc.sh test --dry-run -j $(nproc) || true; \
     fi
Suggestion importance[1-10]: 3

__

Why: The suggestion to ignore test failures with || true is a valid consideration, but it changes the build's behavior to be less strict, which might hide underlying issues. Failing the build on a test failure is often the desired behavior in CI.

Low
  • More

@Malmahrouqi3
Copy link
Collaborator Author

https://hub.docker.com/r/mohdsaid497566/mfc/tags?ordering=name
I was able to containerize MFC releases down to v4.8.3 without messing with the source code using a release dispatcher workflow to trigger docker.yml per MFC release tag. I will open a PR to deploy images for previous MFC releases on sbryngelson/mfc:tagname docker repo shortly with instructions for maintainers. I will use them for continuous benchmarking PR (#936).

@sbryngelson
Copy link
Member

sbryngelson commented Jul 30, 2025

Running docker pull --platform=linux/amd64 mohdsaid497566/mfc:v4.9.6-cpu grabs on the container on my m1 mac but when it as docker run --rm -it --platform=linux/amd64 mohdsaid497566/mfc:v4.9.6-cpu and then ./mfc.sh test all the tests fail due to segfaults.

shb-m1pro-3: spencer/Downloads $ docker run --rm -it --platform=linux/amd64 mohdsaid497566/mfc:v4.9.6-cpu
root@6175637cf718:/opt/MFC# ls
CITATION.cff    Dockerfile  README.md   build  examples  mfc.sh  src    toolchain
CMakeLists.txt  LICENSE     benchmarks  docs   mfc.bat   misc    tests
root@6175637cf718:/opt/MFC# ./mfc.sh run ./examples/1D_sodshocktube/case.py -n 1 -j 20
mfc: OK > (venv) Entered the Python 3.10.12 virtual environment (>= 3.8).

      .=++*:          -+*+=.        | root@6175637cf718 [Linux]
     :+   -*-        ==   =* .      | -------------------------
   :*+      ==      ++    .+-       |
  :*##-.....:*+   .#%+++=--+=:::.   | --jobs 20
  -=-++-======#=--**+++==+*++=::-:. | --mpi --no-gpu --no-debug --no-gcov --no-unified
 .:++=----------====+*= ==..:%..... | --targets pre_process, simulation, and post_process
  .:-=++++===--==+=-+=   +.  :=     |
  +#=::::::::=%=. -+:    =+   *:    | ----------------------------------------------------------
 .*=-=*=..    :=+*+:      -...--    | $ ./mfc.sh (build, run, test, clean, count, packer) --help

 Acquiring /opt/MFC/examples/1D_sodshocktube/case.py...
 Acquiring /opt/MFC/examples/1D_sodshocktube/case.py...
 Build | syscheck, syscheck, pre_process, simulation, and post_process | Generic Build

 Generating case.fpp.
   Writing a (new) custom case.fpp file.
 $ cmake --build /opt/MFC/build/staging/7d9b728a37 --target syscheck --parallel 20 --config Release

[100%] Built target syscheck

 $ cmake --install /opt/MFC/build/staging/7d9b728a37

-- Install configuration: "Release"
-- Up-to-date: /opt/MFC/build/install/7d9b728a37/bin/syscheck

 Generating case.fpp.
   Writing a (new) custom case.fpp file.
 $ cmake --build /opt/MFC/build/staging/9a4af0a3bd --target pre_process --parallel 20 --config Release

[100%] Built target pre_process

 $ cmake --install /opt/MFC/build/staging/9a4af0a3bd

-- Install configuration: "Release"
-- Up-to-date: /opt/MFC/build/install/9a4af0a3bd/bin/pre_process

 Generating case.fpp.
   Writing a (new) custom case.fpp file.
 $ cmake --build /opt/MFC/build/staging/98998883b5 --target simulation --parallel 20 --config Release

[100%] Built target simulation

 $ cmake --install /opt/MFC/build/staging/98998883b5

-- Install configuration: "Release"
-- Up-to-date: /opt/MFC/build/install/98998883b5/bin/simulation

 Generating case.fpp.
   Writing a (new) custom case.fpp file.
 $ cmake --build /opt/MFC/build/staging/03b34a2688 --target post_process --parallel 20 --config Release

[100%] Built target post_process

 $ cmake --install /opt/MFC/build/staging/03b34a2688

-- Install configuration: "Release"
-- Up-to-date: /opt/MFC/build/install/03b34a2688/bin/post_process

 Run
   Using queue system Interactive.
   Using baked-in template for default.
   Generating input files for syscheck...

     Generating syscheck.inp:
       INFO: Forwarded 0/49 parameters.

   Generating input files for pre_process...

     Generating pre_process.inp:
       INFO: Forwarded 31/49 parameters.

   Generating input files for simulation...

     Generating simulation.inp:
       INFO: Forwarded 33/49 parameters.

   Generating input files for post_process...

     Generating post_process.inp:
       INFO: Forwarded 20/49 parameters.

   $ /bin/bash /opt/MFC/examples/1D_sodshocktube/MFC.sh

+-----------------------------------------------------------------------------------------------------------+
| MFC case # MFC @ /opt/MFC/examples/1D_sodshocktube/case.py:                                               |
+-----------------------------------------------------------------------------------------------------------+
| * Start-time     00:47:07                            * Start-date     00:47:07                            |
| * Partition      N/A                                 * Walltime       01:00:00                            |
| * Account        N/A                                 * Nodes          1                                   |
| * Job Name       MFC                                 * Engine         interactive                         |
| * QoS            N/A                                 * Binary         N/A                                 |
| * Queue System   Interactive                         * Email          N/A                                 |
+-----------------------------------------------------------------------------------------------------------+

mfc: WARNING > This is the default template.
mfc: WARNING > It is not intended to support all systems and execution engines.
mfc: WARNING > Consider using a different template via the --computer option if you encounter problems.
mfc: OK > :) Selected MPI launcher mpirun. Use --binary to override.
mfc: OK > :) Running syscheck:

+ mpirun -np 1 /opt/MFC/build/install/7d9b728a37/bin/syscheck
 [TEST] MPI: call mpi_init(ierr)
 [TEST] MPI: call mpi_comm_rank(MPI_COMM_WORLD, rank, ierr)
 [TEST] MPI: call mpi_barrier(MPI_COMM_WORLD, ierr)
 [TEST] MPI: call assert(rank >= 0)
 [TEST] MPI: call mpi_comm_size(MPI_COMM_WORLD, nRanks, ierr)
 [TEST] MPI: call assert(nRanks > 0 .and. rank < nRanks)
 [SKIP] ACC: devtype = acc_get_device_type()
 [SKIP] ACC: num_devices = acc_get_num_devices(devtype)
 [SKIP] ACC: call assert(num_devices > 0)
 [SKIP] ACC: call acc_set_device_num(mod(rank, nRanks), devtype)
 [SKIP] ACC: allocate(arr(1:N))
 [SKIP] ACC: !$acc enter data create(arr(1:N))
 [SKIP] ACC: !$acc parallel loop
 [SKIP] ACC: !$acc update host(arr(1:N))
 [SKIP] ACC: !$acc exit data delete(arr)
 [TEST] MPI: call mpi_barrier(MPI_COMM_WORLD, ierr)
 [TEST] MPI: call mpi_finalize(ierr)

 Syscheck: PASSED.

mfc: OK > :) Running pre_process:

+ mpirun -np 1 /opt/MFC/build/install/9a4af0a3bd/bin/pre_process

Program received signal SIGILL: Illegal instruction.

Backtrace for this error:
#0  0x7fffff49e960 in ???
#1  0x7fffff49dac5 in ???
#2  0x7fffff09051f in ???
#3  0x55555555cfd3 in MAIN__
#4  0x5555555575fe in main
--------------------------------------------------------------------------
Primary job  terminated normally, but 1 process returned
a non-zero exit code. Per user-direction, the job has been aborted.
--------------------------------------------------------------------------
--------------------------------------------------------------------------
mpirun noticed that process rank 0 with PID 0 on node 6175637cf718 exited on signal 4 (Illegal instruction).
--------------------------------------------------------------------------

mfc: ERROR > :( /opt/MFC/build/install/9a4af0a3bd/bin/pre_process failed with exit code 132.

@sbryngelson
Copy link
Member

Also I no longer see codespaces available in the MFC repo

@sbryngelson
Copy link
Member

for setting up a codespace in the MFC repo (under mflowcode) i have this in my settings:

Codespaces
Prebuild configuration
Codespaces are not enabled for your organization. Please contact your org admin to enable Codespaces to use this functionality.
Prebuilds are currently disabled because MFlowCode has reached its Codespaces spending limit for this period. Please contact an administrator to enable prebuilds.

@sbryngelson
Copy link
Member

as someone unfamiliar with docker but familiar with many other "coding thing" I was unable to get this running after trying many different things

@sbryngelson
Copy link
Member

hub.docker.com/r/mohdsaid497566/mfc/tags?ordering=name I was able to containerize MFC releases down to v4.8.3 without messing with the source code using a release dispatcher workflow to trigger docker.yml per MFC release tag. I will open a PR to deploy images for previous MFC releases on sbryngelson/mfc:tagname docker repo shortly with instructions for maintainers. I will use them for continuous benchmarking PR (#936).

This sounds like the right path, I just can't run the containers on my machine right now (so new users definitely won't use it in current state. if macos + arm is the issue, well many people own Macbooks...)

@Malmahrouqi3
Copy link
Collaborator Author

Malmahrouqi3 commented Jul 30, 2025

hub.docker.com/r/mohdsaid497566/mfc/tags?ordering=name I was able to containerize MFC releases down to v4.8.3 without messing with the source code using a release dispatcher workflow to trigger docker.yml per MFC release tag. I will open a PR to deploy images for previous MFC releases on sbryngelson/mfc:tagname docker repo shortly with instructions for maintainers. I will use them for continuous benchmarking PR (#936).

This sounds like the right path, I just can't run the containers on my machine right now (so new users definitely won't use it in current state. if macos + arm is the issue, well many people own Macbooks...)

Yeah I just tried that specific image and I do not seem to have any issues running anything on wsl or codespaces. I will see if I can maybe downgrade ubuntu or get a functional base container for macos to make it run somehow.

as someone unfamiliar with docker but familiar with many other "coding thing" I was unable to get this running after trying many different things

If you want try it on Phoenix till I figure something out - or anything non-macos.

@Malmahrouqi3
Copy link
Collaborator Author

Malmahrouqi3 commented Jul 30, 2025

https://docs.docker.com/build/building/multi-platform/

Found this doc on multi-platform builds that Docker automatically selects the correct variant based on the host's architecture
The solution is to add to the GitHub Action, platform: linux/arm64,linux/amd64

@sbryngelson
Copy link
Member

hub.docker.com/r/mohdsaid497566/mfc/tags?ordering=name I was able to containerize MFC releases down to v4.8.3 without messing with the source code using a release dispatcher workflow to trigger docker.yml per MFC release tag. I will open a PR to deploy images for previous MFC releases on sbryngelson/mfc:tagname docker repo shortly with instructions for maintainers. I will use them for continuous benchmarking PR (#936).

This sounds like the right path, I just can't run the containers on my machine right now (so new users definitely won't use it in current state. if macos + arm is the issue, well many people own Macbooks...)

Yeah I just tried that specific image and I do not seem to have any issues running anything on wsl or codespaces. I will see if I can maybe downgrade ubuntu or get a functional base container for macos to make it run somehow.

Well you could deploy an ARM compatible container. That's one option. But I've run docker with x86 code in it without issue on my Mac before (just had to add a flag so it knew what to do). I tried this with your container but it didn't work.

Also I'm still confused about codespaces. Did you just enable it on your fork or something?

as someone unfamiliar with docker but familiar with many other "coding thing" I was unable to get this running after trying many different things

If you want try it on Phoenix till I figure something out - or anything non-macos.

Yeah I "tried" that but Phoenix doesn't have docker or singularity/apptainer? I must be missing something here.

@sbryngelson
Copy link
Member

docs.docker.com/build/building/multi-platform

Found this doc on multi-platform builds that Docker automatically selects the correct variant based on the host's architecture The solution is to add to the GitHub Action, platform: linux/arm64,linux/amd64

So presumably this builds both versions, I suppose? We could try it out. I feel like this might be the equivalent of me using docker pull --platform=linux/amd64 mohdsaid497566/mfc:v4.9.6-cpu cpu which is what i've been doing so far

@Malmahrouqi3
Copy link
Collaborator Author

Malmahrouqi3 commented Jul 30, 2025

Also I'm still confused about codespaces. Did you just enable it on your fork or something?

No, on the base and fork, I do not seem to be having any issues starting a codespace at all.

Yeah I "tried" that but Phoenix doesn't have docker or singularity/apptainer? I must be missing something here.

The cli's do not exist in the login node. If you start an interactive job, you should be able to use apptainer and singularity directly there.

So presumably this builds both versions, I suppose? We could try it out. I feel like this might be the equivalent of me using docker pull --platform=linux/amd64 mohdsaid497566/mfc:v4.9.6-cpu cpu which is what i've been doing so far

Whether multi-platform or arch-specific containers, I will experiment and compare whichever is better.

@sbryngelson
Copy link
Member

Also I'm still confused about codespaces. Did you just enable it on your fork or something?

No, either on the base or fork, I do not seem to be having any issues at all.

Can you post a screenshot(s)? I don't see it anymore (I used to see it... which is why i suggested it)

Yeah I "tried" that but Phoenix doesn't have docker or singularity/apptainer? I must be missing something here.

The cli's do not exist in the login node. If you start an interactive job, you should be able to use apptainer and singularity directly there.

So presumably this builds both versions, I suppose? We could try it out. I feel like this might be the equivalent of me using docker pull --platform=linux/amd64 mohdsaid497566/mfc:v4.9.6-cpu cpu which is what i've been doing so far

Whether multi-platform or arch-specific containers, I will experiment and compare whichever is better.

Sounds good to me. NVHPC should support but you may have to be careful what image of theirs you pull down.

@sbryngelson
Copy link
Member

The cli's do not exist in the login node. If you start an interactive job, you should be able to use apptainer and singularity directly there.

Oh interesting. I don't see them when doing this on Phoenix nor Bridges2. What computer showed you this, did you have to load other modules first?

@Malmahrouqi3
Copy link
Collaborator Author

Oh interesting. I don't see them when doing this on Phoenix nor Bridges2. What computer showed you this, did you have to load other modules first?

Without loading any module, I just tried phoenix, bridges2, and delta with on-demand and ssh, and I was able to use them.

@sbryngelson
Copy link
Member

Oh interesting. I don't see them when doing this on Phoenix nor Bridges2. What computer showed you this, did you have to load other modules first?

Without loading any module, I just tried phoenix, bridges2, and delta with on-demand and ssh, and I was able to use them.

Indeed... apptainer works from compute nodes.

@sbryngelson sbryngelson marked this pull request as draft July 31, 2025 14:29
@sbryngelson
Copy link
Member

sbryngelson commented Jul 31, 2025

this is still draft (not ready for review). it needs a user guide and a way for users to deploy in both user cases: 1. the programmer on a computer w/o internet and 2. the newcomer that knows very very little. In each case I think we have some improvements to make to clarify what to do and how to do it. the newcomer may benefit from codespaces but i just tried it myself and couldn't figure out what to do from the vscode codespace.

that said, i was able to pull the docker container onto my mac when looking at your dockerhub for arm64 and that did indeed work

@sbryngelson
Copy link
Member

BTW look at that old MFC logo! very cool

@sbryngelson
Copy link
Member

PS let me know when this is ready for me to look at again

@Malmahrouqi3
Copy link
Collaborator Author

Status Update:
MFC Docker Repo would look like https://hub.docker.com/r/mohdsaid497566/mfc
where almost all releases are populated along with an overview drafted with decent instructions.

Pre-configured Repo Codespaces https://github.com/Malmahrouqi3/MFC-mo2/
If you start a codespace on my fork, it will load up mohdsaid497566/mfc:v4.4.1-cpu to interact with immediately with no user intervention whatsoever.

Docker GUI can be used to run any containerized release. It would prior stop instantly after startup unless there was an active running command pre-written. However, currently, it starts and stops when instructed by user via GUI buttons. It is pretty cool right now that you can retrieve any release and play with it within seconds under Exec tab.

@sbryngelson
Copy link
Member

sbryngelson commented Aug 4, 2025

Status Update: MFC Docker Repo would look like hub.docker.com/r/mohdsaid497566/mfc where almost all releases are populated along with an overview drafted with decent instructions.

Pre-configured Repo Codespaces Malmahrouqi3/MFC-mo2 If you start a codespace on my fork, it will load up mohdsaid497566/mfc:v4.4.1-cpu to interact with immediately with no user intervention whatsoever.

Docker GUI can be used to run any containerized release. It would prior stop instantly after startup unless there was an active running command pre-written. However, currently, it starts and stops when instructed by user via GUI buttons. It is pretty cool right now that you can retrieve any release and play with it within seconds under Exec tab.

very well done @Malmahrouqi3! This is really a blast from the past... I can't even remember the ordering of commands to get it to run with mpirun properly from your codespace terminal. It's possible this wasn't even supported with version 4.4.1 (but it probably was).

tip: we'll want to keep this requiring minimal upkeep, so it just keeps rolling out containers but otherwise doesn't need to be updated. right now it looks like you have a separate readme for the dockerhub, which closely mirrors the current MFC readme with some changes. this isn't really maintainable... MFC readmes change rapidly as I occasionally kill time messing with it. A minimal but everlasting readme is better (even if it's less attractive to look at).

root@codespaces-df7db8:/opt/MFC# ./mfc.sh run -n 10 -j 1 ./examples/1D_sodshocktube/case.py  -b mpirun
mfc: OK > (venv) Entered the Python virtual environment.
      ___            ___          ___        
     /__/\          /  /\        /  /\       root@codespaces-df7db8 [Linux]
    |  |::\        /  /:/_      /  /:/       ------------------------------
    |  |:|:\      /  /:/ /\    /  /:/        --jobs 1
  __|__|:|\:\    /  /:/ /:/   /  /:/  ___    --mpi
 /__/::::| \:\  /__/:/ /:/   /__/:/  /  /\   --no-gpu
 \  \:\~~\__\/  \  \:\/:/    \  \:\ /  /:/   --no-debug
  \  \:\         \  \::/      \  \:\  /:/    --targets pre_process, simulation, and post_process
   \  \:\         \  \:\       \  \:\/:/     -----------------------------------------------------------
    \  \:\         \  \:\       \  \::/      $ ./mfc.sh [build, run, test, clean, count, packer] --help
     \__\/          \__\/        \__\/       

Run
  Acquiring /opt/MFC/examples/1D_sodshocktube/case.py...
  Configuration:
    Input               /opt/MFC/examples/1D_sodshocktube/case.py
    Job Name      (-#)  MFC
    Engine        (-e)  interactive
    Nodes         (-N)  1
    Tasks (/node) (-n)  10
    MPI Binary    (-b)  mpirun
  Generating input files for pre_process...
    
    Generating pre_process.inp.
      INFO: Forwarded 32/48 parameters.
    
    Generating pre_process/include/case.fpp.
      INFO: Existing case.fpp file is up to date.
    
  Generating input files for simulation...
    
    Generating simulation.inp.
      INFO: Forwarded 30/48 parameters.
    
    Generating simulation/include/case.fpp.
      INFO: Case optimization is disabled. Use --case-optimization to enable it.
      INFO: Existing case.fpp file is up to date.
    
  Generating input files for post_process...
    
    Generating post_process.inp.
      INFO: Forwarded 21/48 parameters.
    
    Generating post_process/include/case.fpp.
      INFO: No case.fpp file is generated for post_process.
    
  Building pre_process:

    $ cmake --build /opt/MFC/build/pre_process --target pre_process -j 1 --config Release

[100%] Built target pre_process

    $ cmake --install /opt/MFC/build/pre_process

-- Install configuration: "Release"
-- Up-to-date: /opt/MFC/build/install/bin/pre_process

  Building simulation:

    $ cmake --build /opt/MFC/build/simulation --target simulation -j 1 --config Release

[100%] Built target simulation

    $ cmake --install /opt/MFC/build/simulation

-- Install configuration: "Release"
-- Up-to-date: /opt/MFC/build/install/bin/simulation

  Building post_process:

    $ cmake --build /opt/MFC/build/post_process --target post_process -j 1 --config Release

[100%] Built target post_process

    $ cmake --install /opt/MFC/build/post_process

-- Install configuration: "Release"
-- Up-to-date: /opt/MFC/build/install/bin/post_process

  Ensuring the Interactive Engine works (30s timeout):
  $ mpirun -np 10 hostname


Error: The Interactive Engine appears to hang or exit with a non-zero status code. This may indicate that the wrong MPI binary is being used to launch parallel jobs. You can specify the 
correct one for your system using the <-b,--binary> option. For example:
 - ./mfc.sh run <myfile.py> -b mpirun
 - ./mfc.sh run <myfile.py> -b srun
Reason: Exit code.

Terminated
mfc: (venv) Exiting the Python virtual environment.

@Malmahrouqi3
Copy link
Collaborator Author

Malmahrouqi3 commented Aug 7, 2025

I just finalized the PR and it can be merged safely after populating releases' images (v4.3.0-v5.0.5) - 38 which should take about a day or so. Not much ought to be changed in the source code in old releases as a result, except for replacing the old SILO block in CMakeLists.txt and specifying the version of cantera==3.0.1 in pyproject.toml.

I added mfc:latest-xxx tag and simplified a bit the docker readme
README.txt
docker repo

@Malmahrouqi3 Malmahrouqi3 marked this pull request as ready for review August 7, 2025 02:48
@Copilot Copilot AI review requested due to automatic review settings August 7, 2025 02:48
Copy link

qodo-merge-pro bot commented Aug 7, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
The workflow uses Docker Hub credentials in build arguments (lines 64-72 in docker.yml), which could potentially expose these secrets in Docker image layers or build logs. Additionally, the workflow clones repositories and handles credentials that should be properly secured. The ENTRYPOINT using 'tail -f /dev/null' keeps containers running indefinitely which could be a resource security concern.

⚡ Recommended focus areas for review

Syntax Error

JSON syntax error with trailing comma after the last property in the object. This will cause the devcontainer configuration to fail when loaded.

  },
}
Security Risk

Docker secrets are being used in build arguments which can expose sensitive information in Docker image layers and build logs. The DOCKERHUB_USERNAME and DOCKERHUB_PASSWORD should not be passed as build arguments.

build-args: |
  BASE_IMAGE=${{ matrix.config.base_image }}
  TARGET=${{ matrix.config.name }}
  CC_COMPILER=${{ contains(matrix.config.name, 'gpu') && 'nvc' || 'gcc' }}
  CXX_COMPILER=${{ contains(matrix.config.name, 'gpu') && 'nvc++' || 'g++' }}
  FC_COMPILER=${{ contains(matrix.config.name, 'gpu') && 'nvfortran' || 'gfortran' }}
  COMPILER_PATH=${{ contains(matrix.config.name, 'gpu') && '/opt/nvidia/hpc_sdk/Linux_x86_64/compilers/bin' || '/usr/bin' }}
  COMPILER_LD_LIBRARY_PATH=${{ contains(matrix.config.name, 'gpu') && '/opt/nvidia/hpc_sdk/Linux_x86_64/compilers/lib' || '/usr/lib' }}
tags: ${{ secrets.DOCKERHUB_USERNAME }}/mfc:${{ env.TAG }}-${{ matrix.config.name }}-${{ matrix.config.runner}}
Resource Management

The container runs indefinitely with 'tail -f /dev/null' as entrypoint, which may consume unnecessary resources. Consider using a more appropriate entrypoint or making it configurable for different use cases.

ENTRYPOINT ["tail", "-f", "/dev/null"]

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces Docker containerization support for MFC, enabling both CPU and GPU container variants with automated builds and deployment. The implementation provides a comprehensive containerization solution that allows users to run MFC in standardized environments across different platforms.

Key Changes:

  • Docker containerization with CPU/GPU variants using appropriate base images and compilers
  • Automated GitHub Actions workflow for building and deploying containers on releases
  • Multi-architecture support with manifest creation for cross-platform compatibility

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
.github/workflows/docker.yml GitHub Actions workflow for automated container builds and Docker Hub deployment
.github/Dockerfile Multi-stage Dockerfile supporting both CPU (Ubuntu) and GPU (NVIDIA HPC SDK) variants
.github/.dockerignore Docker ignore patterns to exclude build artifacts and temporary files from container builds
.devcontainer/devcontainer.json GitHub Codespaces configuration to automatically load MFC container environment

Copy link

qodo-merge-pro bot commented Aug 7, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add test failure handling

Add error handling for the test command to prevent the container build from
succeeding if tests fail. The current implementation ignores test failures which
could lead to broken containers being deployed.

.github/Dockerfile [47-52]

 RUN cd /opt/MFC && \
     if [ "$TARGET" = "gpu" ]; then \
-      ./mfc.sh test --dry-run --gpu -j $(nproc); \
+      ./mfc.sh test --dry-run --gpu -j $(nproc) || exit 1; \
     else \
-      ./mfc.sh test --dry-run -j $(nproc); \
+      ./mfc.sh test --dry-run -j $(nproc) || exit 1; \
     fi
Suggestion importance[1-10]: 9

__

Why: This suggestion addresses a critical issue where the Docker build would succeed even if tests fail, potentially leading to the deployment of a broken container.

High
Add manifest creation error handling

Add error handling for manifest creation commands to prevent silent failures. If
manifest creation fails, the workflow should fail rather than continuing to push
potentially incomplete manifests.

.github/workflows/docker.yml [92-93]

-docker manifest create $REGISTRY:latest-cpu $REGISTRY:$TAG-cpu-ubuntu-22.04 $REGISTRY:$TAG-cpu-ubuntu-22.04-arm
-docker manifest create $REGISTRY:latest-gpu $REGISTRY:$TAG-gpu-ubuntu-22.04 $REGISTRY:$TAG-gpu-ubuntu-22.04-arm
+docker manifest create $REGISTRY:latest-cpu $REGISTRY:$TAG-cpu-ubuntu-22.04 $REGISTRY:$TAG-cpu-ubuntu-22.04-arm || exit 1
+docker manifest create $REGISTRY:latest-gpu $REGISTRY:$TAG-gpu-ubuntu-22.04 $REGISTRY:$TAG-gpu-ubuntu-22.04-arm || exit 1
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the workflow would not fail if manifest creation fails and proposes a fix to ensure the job exits with an error, improving the reliability of the deployment pipeline.

Medium
Possible issue
Fix JSON syntax error

Remove the trailing comma after the settings object to ensure valid JSON syntax.
JSON does not allow trailing commas and this will cause parsing errors.

.devcontainer/devcontainer.json [1-9]

 {
   "name": "MFC Container",
   "image": "sbryngelson/mfc:latest-cpu",
   "workspaceFolder": "/opt/MFC",
   "settings": {
     "terminal.integrated.shell.linux": "/bin/bash",
     "editor.formatOnSave": true
-  },
+  }
 }
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a trailing comma in the JSON file, which is a syntax error and would cause issues with standard JSON parsers.

Medium
  • More

@sbryngelson
Copy link
Member

I just finalized the PR and it can be merged safely after populating releases' images (v4.3.0-v5.0.5) - 38 which should take about a day or so. Not much ought to be changed in the source code in old releases as a result, except for replacing the old SILO block in CMakeLists.txt and specifying the version of cantera==3.0.1 in pyproject.toml.

I added mfc:latest-xxx tag and simplified a bit the docker readme README.txt docker repo

I'm not quite sure what this means, are you asking someone else to do something, or waiting for some automated process to finish?

@Malmahrouqi3
Copy link
Collaborator Author

I just finalized the PR and it can be merged safely after populating releases' images (v4.3.0-v5.0.5) - 38 which should take about a day or so. Not much ought to be changed in the source code in old releases as a result, except for replacing the old SILO block in CMakeLists.txt and specifying the version of cantera==3.0.1 in pyproject.toml.
I added mfc:latest-xxx tag and simplified a bit the docker readme README.txt docker repo

I'm not quite sure what this means, are you asking someone else to do something, or waiting for some automated process to finish?

My bad. Since I do not have access to your docker credentials, can you please do it so they all be posted in under sbryngelson/mfc?

@Malmahrouqi3
Copy link
Collaborator Author

Malmahrouqi3 commented Aug 7, 2025

Or to make it simpler can you add me as a collaborator in the docker repo sbryngelson/mfc so I would push all stuff in there?
Username: mohdsaid497566

@sbryngelson
Copy link
Member

Or to make it simpler can you add me as a collaborator in the docker repo sbryngelson/mfc so I would push all stuff in there? Username: mohdsaid497566

It seems like we should make a username mflowcode or something? I don't want to hold sole responsibility for maintaining this.

@Malmahrouqi3
Copy link
Collaborator Author

I guess you can make some sort of org in docker instead of personal account.

1 similar comment
@Malmahrouqi3
Copy link
Collaborator Author

I guess you can make some sort of org in docker instead of personal account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants