Skip to content

apptainer update#1262

Open
CallumWalley wants to merge 6 commits into
mainfrom
apptainer-update
Open

apptainer update#1262
CallumWalley wants to merge 6 commits into
mainfrom
apptainer-update

Conversation

@CallumWalley
Copy link
Copy Markdown
Member

Fix formatting

update examples

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 10, 2026

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

Avast! Ye've overhauled the Apptainer charts, swapping those nested containers for proper headers and dragging in some boilerplate macros. While the new structure is a sight better than a barnacle-covered hull, yer Slurm scripts are a disaster. Ye've ignored the code of the sea by using spaces instead of tabs for headers, forgetting to purge yer modules like a scurvy dog, and leaving code blocks as naked as a castaway. Also, stop hardcoding yer project bounty unless ye want every pirate in the archipelago raiding yer allocation. Fix those typos before the crew mutinies over yer sloppy penmanship.


We recommend adding those environment variables to your `~/.bashrc` to make the future use cases of Apptainer. Please make sure to replace `/nesi/nobackup/nesi12345/apptainer-cache` with your nobackup directory.
```bash
export APPTAINER_CACHEDIR="/nesi/nobackup/nesi12345/apptainer-cache"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low

Hardcoding nesi12345? I suppose ye want every sailor on the Seven Seas to be using yer project's bounty. Use a placeholder or a variable, ye landlubber!

Suggested change
export APPTAINER_CACHEDIR="/nesi/nobackup/nesi12345/apptainer-cache"
export APPTAINER_CACHEDIR="/nesi/nobackup/nesi99991/apptainer-cache"

echo 'export APPTAINER_CACHEDIR="/nesi/nobackup/nesi12345/apptainer-cache"' >> ~/.bashrc
echo 'export APPTAINER_TMPDIR=${APPTAINER_CACHEDIR}' >> ~/.bashrc
```
To make these changed permanent, add them to your `~/.bashrc`:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low

Blimey, matey! Ye've written 'changed' when ye meant 'changes'. I suppose the sea spray is getting in yer eyes.

Suggested change
To make these changed permanent, add them to your `~/.bashrc`:
To make these changes permanent, add them to your ~/.bashrc:

Comment on lines +78 to +82
#SBATCH --job-name apptainer-build
#SBATCH --time 00:30:00
#SBATCH --mem 4GB
#SBATCH --cpus-per-task 2
#SBATCH --account nesi12345
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low

Avast! Those be spaces I see? The charts (Style Guide line 402) clearly state ye must use a tab to separate yer Slurm keywords from their values. Don't make me walk the plank for yer sloppy spacing!

Suggested change
#SBATCH --job-name apptainer-build
#SBATCH --time 00:30:00
#SBATCH --mem 4GB
#SBATCH --cpus-per-task 2
#SBATCH --account nesi12345
#SBATCH --job-name apptainer-build
#SBATCH --time 00:30:00
#SBATCH --mem 4GB
#SBATCH --cpus-per-task 2
#SBATCH --account nesi99991
References
  1. Use tab for your Slurm header delimiter. (link)

#SBATCH --time=0-00:30:00
#SBATCH --mem=4GB
#SBATCH --cpus-per-task=2
module load Apptainer/{{app.default}}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low

I see ye've decided that module purge is beneath ye. How very brave of ye to risk a mutiny of conflicting software versions. (Style Guide line 406).

Suggested change
module load Apptainer/{{app.default}}
module purge
module load Apptainer/{{app.default}}
References
  1. Always module purge before module load. (link)

The `fakeroot` build method does not work for all container types. If you encounter issues, contact [support@nesi.org.nz](mailto:support@nesi.org.nz).

If you see the following error, it is likely caused by a bad upstream image on Docker Hub. Try an older version or a different base image:
```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low

Arrr, ye've left this code block naked! Give it a class like txt or stderr so the crew knows what they're looking at. (Style Guide line 355).

Suggested change
```
```txt
References
  1. For plain text code blocks, still good to use a class as descriptor (e.g. txt, stdout, stderr). (link)

Comment on lines +143 to +147
#SBATCH --job-name container-job
#SBATCH --time 01:00:00
#SBATCH --mem 4G
#SBATCH --cpus-per-task 4
#SBATCH --account nesi12345
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low

More spaces where tabs should be! Are ye trying to confuse the sea monsters? Use tabs for yer Slurm headers as the law (Style Guide line 402) dictates.

Suggested change
#SBATCH --job-name container-job
#SBATCH --time 01:00:00
#SBATCH --mem 4G
#SBATCH --cpus-per-task 4
#SBATCH --account nesi12345
#SBATCH --job-name container-job
#SBATCH --time 01:00:00
#SBATCH --mem 4G
#SBATCH --cpus-per-task 4
#SBATCH --account nesi99991
References
  1. Use tab for your Slurm header delimiter. (link)

# Run container %runscript
apptainer run tensorflow-latest-gpu.sif my_tensorflow.py some_argument
```
module load Apptainer/{{app.default}}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low

Ye be repeating yer sins! Purge the deck before ye load the Apptainer, or ye'll find no treasure in this script. (Style Guide line 406).

Suggested change
module load Apptainer/{{app.default}}
module purge
module load Apptainer/{{app.default}}
References
  1. Always module purge before module load. (link)

Comment on lines +184 to +189
#SBATCH --job-name gpu-container-job
#SBATCH --time 01:00:00
#SBATCH --mem 8G
#SBATCH --cpus-per-task 4
#SBATCH --gpus-per-node 1
#SBATCH --account nesi12345
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low

Tabs, man! Tabs! (Style Guide line 402).

Suggested change
#SBATCH --job-name gpu-container-job
#SBATCH --time 01:00:00
#SBATCH --mem 8G
#SBATCH --cpus-per-task 4
#SBATCH --gpus-per-node 1
#SBATCH --account nesi12345
#SBATCH --job-name gpu-container-job
#SBATCH --time 01:00:00
#SBATCH --mem 8G
#SBATCH --cpus-per-task 4
#SBATCH --gpus-per-node 1
#SBATCH --account nesi99991
References
  1. Use tab for your Slurm header delimiter. (link)

#SBATCH --gpus-per-node 1
#SBATCH --account nesi12345

module load Apptainer/{{app.default}}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low

Ye'd think ye'd learn by now. Purge the modules! (Style Guide line 406).

Suggested change
module load Apptainer/{{app.default}}
module purge
module load Apptainer/{{app.default}}
References
  1. Always module purge before module load. (link)

@CallumWalley
Copy link
Copy Markdown
Member Author

Might be worth adding --no-xattrs to suprress build warning about acls

@CallumWalley
Copy link
Copy Markdown
Member Author

A lot of this is no longer correct.

For example fakeroot is not enabled on the login nodes.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants