Skip to content

Review episode 2: Resource Requirements - #20

Open
stderr-enst wants to merge 9 commits into
mainfrom
review-episode02
Open

Review episode 2: Resource Requirements#20
stderr-enst wants to merge 9 commits into
mainfrom
review-episode02

Conversation

@stderr-enst

Copy link
Copy Markdown
Collaborator

This is addressing comments on episode 02 from this issue: #14

In a next iteration I'll also go through the material and will suggest changes / simplifications.

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown

Thank you!

Thank you for your pull request 😃

🤖 This automated message can help you check the rendered files in your submission for clarity. If you have any questions, please feel free to open an issue in {sandpaper}.

If you have files that automatically render output (e.g. R Markdown), then you should check for the following:

  • 🎯 correct output
  • 🖼️ correct figures
  • ❓ new warnings
  • ‼️ new errors

Rendered Changes

🔍 Inspect the changes: https://github.com/carpentries-incubator/hpc-job-efficiency/compare/md-outputs..md-outputs-PR-20

The following changes were observed in the rendered markdown documents:

 02_resourcerequirements.md | 246 ++++++++++++++++++++++++---------------------
 md5sum.txt                 |   3 +-
 2 files changed, 136 insertions(+), 113 deletions(-)
What does this mean?

If you have source files that require output and figures to be generated (e.g. R Markdown), then it is important to make sure the generated figures and output are reproducible.

This output provides a way for you to inspect the output in a diff-friendly manner so that it's easy to see the changes that occur due to new software versions or randomisation.

⏱️ Updated at 2026-07-28 14:16:42 +0000

github-actions Bot pushed a commit that referenced this pull request Jul 24, 2026
github-actions Bot pushed a commit that referenced this pull request Jul 27, 2026
@stderr-enst

Copy link
Copy Markdown
Collaborator Author

@jjegg01 I've added some changes to the second episode and tried to address suggestions of #14

Everything is intended as a suggestion, so please disagree, if you do! :)

There are three points I thought about:

  1. I think we should go with top -u ${USER} -H instead of htop, since htops header can be overwhelming if the number of nodes is too large
  2. The last "Discussion" activity in the CPU core section seems a bit out of the blue. I think for many it may not be clear how operating systems "default to oversubscription" and it may not be clear what the question is aiming at exactly. I don't have a better idea for the activity, yet. But it does feel like a good sport for some form of challenge or discussion.
  3. I think I'd take out the mpi-cgroup-memory sections alltogether. It's a good chance to keep it simple. Downloading the script, running it with LD_PRELOAD, and discussing how it works may introduce too many trip-wires and confusion. What do you think?

@stderr-enst stderr-enst changed the title WIP: Review episode 2: Resource Requirements Review episode 2: Resource Requirements Jul 27, 2026

@jjegg01 jjegg01 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the review! The episode looks much better now. I have added a few comments below (mostly nitpicks).

Comment thread episodes/02_resourcerequirements.md Outdated
find /home -maxdepth 1 -mindepth 1 -type d | wc -l
```
```output
250

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

On our cluster it's more like 700, but I guess the concrete number does not matter much (also we need to decommission inactive users more often...).

Comment thread episodes/02_resourcerequirements.md Outdated
Comment thread episodes/02_resourcerequirements.md Outdated
Comment thread episodes/02_resourcerequirements.md Outdated
This behavior seems contradictory at first: Slurm reported previously that our job only used around 367 MB of memory at most, which is well below the 500 MB limit we set. The explanation for this discrepancy lies in the fact that Slurm measures the peak memory consumption of jobs by *polling*, i.e., by periodically sampling how much memory the job currently uses. Unfortunately, if the program has spikes in memory consumption that are small enough to fit between two samples, Slurm will miss them and report an incorrect peak memory value. Spikes in memory usage are quite common, for example if your application uses short-lived subprocesses. Most annoyingly, many programs allocate a large chunk of memory right at the end of the computation to write out the results. In the case of the snowman raytracer, we encode the raw pixel data into a PNG at the end, which means we temporarily keep both the raw image and the PNG data in memory.

![](fig/slurm-memory-sampling.svg)
![](fig/slurm-memory-sampling.svg){alt="Slurm determines memory consuption by *polling*, i.e., periodically checking on the memory consumption of your job. If you job has a memory allocation profile with short spikes in memory usage, the value reported by `seff` can be incorrect. In particular, if the job gets cancelled due to memory exhaustion, you should not rely on the value reported by `seff` as it is likely significantly too low."}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I can't test this right now, but isn't the alt text what is shown when the image can't be loaded (or for people with impaired vision using a screen reader)? If so, it might make more sense to add a description of the image instead of just a copy of the paragraph below.

Suggested change
![](fig/slurm-memory-sampling.svg){alt="Slurm determines memory consuption by *polling*, i.e., periodically checking on the memory consumption of your job. If you job has a memory allocation profile with short spikes in memory usage, the value reported by `seff` can be incorrect. In particular, if the job gets cancelled due to memory exhaustion, you should not rely on the value reported by `seff` as it is likely significantly too low."}
![](fig/slurm-memory-sampling.svg){alt="A graph plotting memory consumption of a job over time. The curve has an upwards trend with intermittent spikes. The periodic sampling of Slurm are shown as vertical lines intersecting the graph in regular intervals. While one of the memory spikes coincides with a sampling point, two others do not. One of them is near the start of the execution time and falls between two sample points. The other is at the very end of the execution time and occurs after the last sample point."}

Comment thread episodes/02_resourcerequirements.md Outdated
Comment thread episodes/02_resourcerequirements.md Outdated
Comment on lines +384 to +394
::: callout
# Too tight memory limits can reduce performance!

Slurm enforces memory limits for all processes in a job.
Besides the applications memory demand (RSS - resident set size), this also includes caching mechanism of the Linux Kernel, e.g. for file I/O.

Too tight memory limits can cause too small caches next to the applications memory demand.
This in turn can severely reduce the jobs performance in some cases.

:::

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we demo that?

@jjegg01

jjegg01 commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator
1. I think we should go with `top -u ${USER} -H` instead of `htop`, since `htop`s header can be overwhelming if the number of nodes is too large

You are right, on a 1080p screen, the CPUs can blow up htop's header quite a lot. I always find the UI of top very confusing for beginners since it is very "numeric", but I don't know a better solution right now either.

2. The last "Discussion" activity in the CPU core section seems a bit out of the blue. I think for many it may not be clear how operating systems "default to oversubscription" and it may not be clear what the question is aiming at exactly. I don't have a better idea for the activity, yet. But it does feel like a good sport for some form of challenge or discussion.

This was basically a callback to the start of that section, where OS multitasking is discussed briefly. The aim was twofold: Firstly, I wanted the learners to realize that HPC workloads are different than normal desktop applications in the sense that they need the CPU core all the time and not just occasionally, so CPU is actually a contested resource. The other was to bridge the gap between essentially bad-talking CPU oversubscription for most of the section, but it still being the default everywhere but HPC. However, I think you might be right that only learners with a computer science background would even ask that second question. Maybe we can rephrase the question? A suggestion: "Can you imagine a situation where CPU oversubscription might be sensible (perhaps also outside the HPC context)?" Discussing this would lead the the same realization that not everything is CPU-bound, but wouldn't force anyone down the more CS-heavy road of OS scheduling.

3. I think I'd take out the `mpi-cgroup-memory` sections alltogether. It's a good chance to keep it simple. Downloading the script, running it with LD_PRELOAD, and discussing how it works may introduce too many trip-wires and confusion. What do you think?

Yeah, that was a bit of a long shot and solutions with LD_PRELOAD are always a bit hacky. I basically wanted to address the concern that we don't really cover memory allocation issues for multi-node MPI jobs too well here (which does lead to real problems I have seen in our support cases!). Perhaps we can move that into some kind of bonus section that the instructor can use if anyone actually asks for this?

stderr-enst and others added 6 commits July 28, 2026 16:13
Co-authored-by: Julian Jeggle <43239440+jjegg01@users.noreply.github.com>
Co-authored-by: Julian Jeggle <43239440+jjegg01@users.noreply.github.com>
Co-authored-by: Julian Jeggle <43239440+jjegg01@users.noreply.github.com>
Co-authored-by: Julian Jeggle <43239440+jjegg01@users.noreply.github.com>
Co-authored-by: Julian Jeggle <43239440+jjegg01@users.noreply.github.com>
Co-authored-by: Julian Jeggle <43239440+jjegg01@users.noreply.github.com>
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