Conversation
* add initial draft of virtualization - upload generate_brainspace_images.sh to create Docker and Singularity files using neurodocker - upload Dockerfile - upload Singularity * add initial draft of virtualization - upload generate_brainspace_images.sh to create Docker and Singularity files using neurodocker - upload Dockerfile - upload Singularity * update image build and requirements.txt - update generate_brainspace_images.sh - include nilearn in requirements.txt * update images and requirements - update generate_brainspace_images.sh - include nilearn in requirements.txt * update image env Co-authored-by: Peer <[email protected]>
There was a problem hiding this comment.
Pull request overview
Adds an initial containerization (“virtualization”) draft for BrainSpace by introducing Neurodocker-based generation of Docker and Singularity build definitions, alongside minor dependency updates.
Changes:
- Add
generate_brainspace_images.shto generate Dockerfile/Singularity recipes via Neurodocker. - Add initial
DockerfileandSingularityrecipes for building a BrainSpace container image. - Update
requirements.txt(includesnilearn).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| requirements.txt | Dependency list update (includes nilearn). |
| generate_brainspace_images.sh | Script to generate Docker/Singularity definitions and optionally build images locally. |
| Dockerfile | Neurodocker-generated Docker image definition for BrainSpace environment. |
| Singularity | Neurodocker-generated Singularity recipe for BrainSpace environment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [ '$1' = 'docker' ]; then | ||
| echo "docker image will be build locally" | ||
| # build image using the saved files | ||
| docker build -t brainspace . | ||
| elif [ '$1' = 'singularity' ]; then | ||
| echo "singularity image will be build locally" | ||
| # build image using the saved files | ||
| singularity build brainspace.simg Singularity | ||
| elif [ '$1' = 'both' ]; then |
There was a problem hiding this comment.
The argument checks use single quotes around $1 (e.g., [ '$1' = 'docker' ]), which prevents variable expansion and makes these branches never match. Use double quotes around the variable instead (and consider using case for clarity).
| if [ '$1' = 'docker' ]; then | |
| echo "docker image will be build locally" | |
| # build image using the saved files | |
| docker build -t brainspace . | |
| elif [ '$1' = 'singularity' ]; then | |
| echo "singularity image will be build locally" | |
| # build image using the saved files | |
| singularity build brainspace.simg Singularity | |
| elif [ '$1' = 'both' ]; then | |
| if [ "$1" = 'docker' ]; then | |
| echo "docker image will be build locally" | |
| # build image using the saved files | |
| docker build -t brainspace . | |
| elif [ "$1" = 'singularity' ]; then | |
| echo "singularity image will be build locally" | |
| # build image using the saved files | |
| singularity build brainspace.simg Singularity | |
| elif [ "$1" = 'both' ]; then |
| # | ||
| # Steps to build, upload, and deploy the BrainSpace docker and/or singularity image: | ||
| # | ||
| # 1. Create or update the Dockerfile and Singuarity recipe: |
There was a problem hiding this comment.
Typo in the comment header: "Singuarity" should be "Singularity".
| # 1. Create or update the Dockerfile and Singuarity recipe: | |
| # 1. Create or update the Dockerfile and Singularity recipe: |
| # docker pull your_docker_id/brainspace | ||
| # | ||
| # In the following, the Docker container can be the original (brainspace) | ||
| # or the pulled version (ypur_docker_id/brainspace:tag), and is given access to /Users/brainspace |
There was a problem hiding this comment.
Typo in the comment: "ypur_docker_id" should be "your_docker_id".
| # or the pulled version (ypur_docker_id/brainspace:tag), and is given access to /Users/brainspace | |
| # or the pulled version (your_docker_id/brainspace:tag), and is given access to /Users/brainspace |
| RUN bash -c 'curl https://raw.githubusercontent.com/PeerHerholz/BrainSpace/initial_draft_virtualization/requirements.txt > requirements.txt && chmod 777 requirements.txt' | ||
|
|
||
| RUN test "$(getent passwd brainspace)" || useradd --no-user-group --create-home --shell /bin/bash brainspace |
There was a problem hiding this comment.
The generated Dockerfile fetches requirements.txt from a GitHub raw URL on a branch name and then installs BrainSpace from a git branch. This is not reproducible (cache won’t invalidate when the file changes) and is a supply-chain risk. Prefer copying the repo’s requirements.txt into the image (so builds track the checked-in file) and installing the package from the local source or from a pinned tag/commit hash.
| && chmod 777 /opt && chmod a+s /opt \ | ||
| && mkdir -p /neurodocker \ | ||
| && if [ ! -f "$ND_ENTRYPOINT" ]; then \ | ||
| echo '#!/usr/bin/env bash' >> "$ND_ENTRYPOINT" \ | ||
| && echo 'set -e' >> "$ND_ENTRYPOINT" \ | ||
| && echo 'export USER="${USER:=`whoami`}"' >> "$ND_ENTRYPOINT" \ | ||
| && echo 'if [ -n "$1" ]; then "$@"; else /usr/bin/env bash; fi' >> "$ND_ENTRYPOINT"; \ | ||
| fi \ | ||
| && chmod -R 777 /neurodocker && chmod a+s /neurodocker |
There was a problem hiding this comment.
This Dockerfile sets very permissive permissions (chmod 777) and sets the setuid bit on /opt and /neurodocker. That unnecessarily expands the attack surface of the image. Tighten permissions to the minimum required (e.g., owned by the intended user/group with 755/775 as appropriate) and avoid setuid unless there’s a concrete need.
|
|
||
| su - root | ||
|
|
||
| bash -c 'curl https://raw.githubusercontent.com/PeerHerholz/BrainSpace/initial_draft_virtualization/requirements.txt > requirements.txt && chmod 777 requirements.txt' |
There was a problem hiding this comment.
The Singularity recipe fetches requirements.txt from a GitHub raw URL on a branch name. This is not reproducible and is a supply-chain risk (and it also bypasses build cache invalidation semantics). Prefer embedding/copying the repository’s requirements.txt into the image build context or pinning to an immutable commit hash.
| bash -c 'curl https://raw.githubusercontent.com/PeerHerholz/BrainSpace/initial_draft_virtualization/requirements.txt > requirements.txt && chmod 777 requirements.txt' | |
| bash -c 'curl https://raw.githubusercontent.com/PeerHerholz/BrainSpace/abcdef1234567890abcdef1234567890abcdef12/requirements.txt > requirements.txt && chmod 777 requirements.txt' |
| Bootstrap: docker | ||
| From: ubuntu:latest | ||
|
|
There was a problem hiding this comment.
Using From: ubuntu:latest makes Singularity builds non-deterministic and can break unexpectedly as the upstream base changes. Pin to a specific Ubuntu release (or digest) that you’ve verified works with the required system packages.
| ###################################################### | ||
| # Generate a Dockerfile and Singularity recipe for building a BrainSpace container | ||
| # (https://brainspace.readthedocs.io/en/latest/). |
There was a problem hiding this comment.
This script is intended to be run with bash, but it lacks a shebang. Add #!/usr/bin/env bash at the top (and consider set -euo pipefail) so it behaves correctly when executed directly (e.g., after chmod +x).
| FROM ubuntu:latest | ||
|
|
There was a problem hiding this comment.
Using ubuntu:latest makes builds non-deterministic and can break unexpectedly over time (especially with versioned apt packages like libvtk6.3). Pin the base image to a specific Ubuntu release (or digest) that you’ve verified works.
| su - root | ||
|
|
||
| bash -c 'curl https://raw.githubusercontent.com/PeerHerholz/BrainSpace/initial_draft_virtualization/requirements.txt > requirements.txt && chmod 777 requirements.txt' | ||
|
|
||
| test "$(getent passwd brainspace)" || useradd --no-user-group --create-home --shell /bin/bash brainspace | ||
| su - brainspace | ||
|
|
||
| export PATH="/opt/miniconda-latest/bin:$PATH" | ||
| echo "Downloading Miniconda installer ..." | ||
| conda_installer="/tmp/miniconda.sh" | ||
| curl -fsSL --retry 5 -o "$conda_installer" https://repo.continuum.io/miniconda/Miniconda3-latest-Linux-x86_64.sh | ||
| bash "$conda_installer" -b -p /opt/miniconda-latest | ||
| rm -f "$conda_installer" | ||
| conda update -yq -nbase conda | ||
| conda config --system --prepend channels conda-forge | ||
| conda config --system --set auto_update_conda false | ||
| conda config --system --set show_channel_urls true | ||
| sync && conda clean --all && sync | ||
| conda create -y -q --name brainspace | ||
| conda install -y -q --name brainspace \ | ||
| "python=3.7" \ | ||
| "panel" \ | ||
| "pyqt" \ | ||
| "pyvista" \ | ||
| "notebook" \ | ||
| "ipython" | ||
| sync && conda clean --all && sync |
There was a problem hiding this comment.
The %post section uses su - root / su - brainspace without -c, which relies on stdin/script parsing behavior and makes it harder to reason about which user runs subsequent commands. Consider replacing these with explicit su -c '...' brainspace (or runuser) blocks so user context is unambiguous and less brittle across build environments.
| su - root | |
| bash -c 'curl https://raw.githubusercontent.com/PeerHerholz/BrainSpace/initial_draft_virtualization/requirements.txt > requirements.txt && chmod 777 requirements.txt' | |
| test "$(getent passwd brainspace)" || useradd --no-user-group --create-home --shell /bin/bash brainspace | |
| su - brainspace | |
| export PATH="/opt/miniconda-latest/bin:$PATH" | |
| echo "Downloading Miniconda installer ..." | |
| conda_installer="/tmp/miniconda.sh" | |
| curl -fsSL --retry 5 -o "$conda_installer" https://repo.continuum.io/miniconda/Miniconda3-latest-Linux-x86_64.sh | |
| bash "$conda_installer" -b -p /opt/miniconda-latest | |
| rm -f "$conda_installer" | |
| conda update -yq -nbase conda | |
| conda config --system --prepend channels conda-forge | |
| conda config --system --set auto_update_conda false | |
| conda config --system --set show_channel_urls true | |
| sync && conda clean --all && sync | |
| conda create -y -q --name brainspace | |
| conda install -y -q --name brainspace \ | |
| "python=3.7" \ | |
| "panel" \ | |
| "pyqt" \ | |
| "pyvista" \ | |
| "notebook" \ | |
| "ipython" | |
| sync && conda clean --all && sync | |
| bash -c 'curl https://raw.githubusercontent.com/PeerHerholz/BrainSpace/initial_draft_virtualization/requirements.txt > requirements.txt && chmod 777 requirements.txt' | |
| test "$(getent passwd brainspace)" || useradd --no-user-group --create-home --shell /bin/bash brainspace | |
| su - brainspace -c "export PATH=\"/opt/miniconda-latest/bin:\$PATH\"; echo \"Downloading Miniconda installer ...\"; conda_installer=\"/tmp/miniconda.sh\"; curl -fsSL --retry 5 -o \"\$conda_installer\" https://repo.continuum.io/miniconda/Miniconda3-latest-Linux-x86_64.sh; bash \"\$conda_installer\" -b -p /opt/miniconda-latest; rm -f \"\$conda_installer\"; conda update -yq -nbase conda; conda config --system --prepend channels conda-forge; conda config --system --set auto_update_conda false; conda config --system --set show_channel_urls true; sync && conda clean --all && sync; conda create -y -q --name brainspace; conda install -y -q --name brainspace python=3.7 panel pyqt pyvista notebook ipython; sync && conda clean --all && sync" |
Co-authored-by: Peer [email protected]