Skip to content

fix(slurm): make the generated job script portable across clusters - #163

Open
mkuznet1 wants to merge 6 commits into
developfrom
mkuznet1/slurm-portability
Open

fix(slurm): make the generated job script portable across clusters#163
mkuznet1 wants to merge 6 commits into
developfrom
mkuznet1/slurm-portability

Conversation

@mkuznet1

@mkuznet1 mkuznet1 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Three clusters in a row have had to patch madengine's source before they could submit a
job. The RCCL CI integration
(ROCm/rocm-systems#9055) carries a
patch_madengine_for_cluster() helper that rewrites job.sh.j2, slurm.py and
run_orchestrator.py at runtime, and the cluster we brought up most recently needed the
same edits by hand. Every one of those patches is a cluster-specific assumption baked
into madengine, not something the cluster did wrong. This PR removes five of the six, and
hardens the filesystem probe that the last of those fixes exposed.

Ticket: AICOMNET-366.

What is in here

Commit Problem Fix
feat(slurm): allow opting out of the --gpus-per-node sbatch directive A cluster with GresTypes=(null) rejects any job script carrying --gpus-per-node, so submission fails before launch. New slurm.skip_gpus_directive (default false) omits the directive; the run relies on --exclusive + nproc_per_node.
fix(slurm): raise the madengine availability probe timeout The pre-submission madengine --version probe had a 5 s timeout, which a cold interpreter start off shared/NFS storage exceeds — a healthy environment was reported as broken. Timeout raised so the probe only catches a hang, not a slow import.
fix(run): cap the informational rocm-libs package query yum info rocm-libs can block forever on an interactive GPG-key prompt, hanging the run on a purely informational query. The query is capped with a timeout on apt/yum/zypper/tdnf alike.
fix(slurm): inherit the submitter's PATH in the sbatch job A batch job is not guaranteed to inherit the submitter's PATH (a site can default sbatch to --export=NONE, and module load can rewrite it), so the compute node aborts with "madengine not found in PATH" after the login-node check passed. The generated script prepends $HOME/.local/bin and the directory the madengine console script was resolved from at submission time.
fix(slurm): match nfs4 in the shared-filesystem probe The probe matched \bnfs\b only, but df -T reports nfs4 on most modern mounts, so a shared submission directory was treated as node-local and the whole project was copied into /tmp. Pattern widened to \bnfs[0-9]*\b.
fix(slurm): read the filesystem type, not the whole df line The same probe grepped the entire df -T line, mount point included, so a local disk at a path such as /mnt/nfs-scratch was classified as shared storage — the opposite mistake, and the more dangerous one. The type column is read on its own via df --output=fstype (with an awk fallback for pre-8.21 coreutils) and the pattern is anchored to it. beegfs and panfs join the list of shared types.

The nfs4 commit also backfills the coverage skip_gpus_directive shipped without, so
nothing fails silently if the directive creeps back into the template.

What is deliberately not in here

patch_madengine_for_cluster() has a sixth patch: a per-node venv bootstrap for clusters
whose compute nodes cannot use the submitter's interpreter. That is a design change to
how madengine reaches compute nodes, not a portability one-liner, and it is tracked
separately. After this PR the CI helper is down to that single patch.

Testing

  • tests/unit: 565 passed (544 before, plus 21 new in tests/unit/test_slurm_job_template.py
    covering the PATH exports, the fstype probe — nfs/nfs3/nfs4/lustre/gpfs/ceph/
    beegfs/panfs accepted, ext4/xfs/overlay/tmpfs and an ext4 disk under
    /mnt/nfs-scratch rejected — and both states of skip_gpus_directive).
  • tests/integration on a GPU node: 149 passed, 1 failed, 2 skipped. The failure
    (test_renderD_count_matches_gpu_count, assert 8 == 17) is a property of that node's KFD
    topology and reproduces without these changes.
  • A real 2-node SLURM run on a GresTypes=(null) cluster (Broadcom Thor2 / RoCE), a
    Primus/Megatron training workload on an RCCL overlay image:
    • the rendered job.sh carries both PATH exports and no --gpus-per-node;
    • RCCL came up on all eight bnxt_re* NICs;
    • the run completed and reported its metrics, with no behaviour change against a
      pre-change run on the same node pair.

Mikhail Kuznetsov and others added 6 commits July 31, 2026 11:53
Clusters that do not advertise GPU GRES reject any job script carrying
--gpus-per-node, so the generated sbatch fails before launch. Add
slurm.skip_gpus_directive (default false) to omit the directive and rely on
exclusive/nproc_per_node instead.

Co-authored-by: Cursor <cursoragent@cursor.com>
The pre-submission check ran `madengine --version` with a 5s timeout, which a
cold interpreter start off shared/NFS storage exceeds, aborting submission on a
perfectly healthy environment. Raise it so the probe only catches a hang.

Co-authored-by: Cursor <cursoragent@cursor.com>
The node-info step shelled out to the host package manager with no time limit.
On a node where yum wants to import a repo GPG key the command waits on a prompt
that never arrives, so the whole multi-node run hangs before the workload starts.

Co-authored-by: Cursor <cursoragent@cursor.com>
A batch job is not guaranteed to inherit the submitter's PATH: a site can default
sbatch to --export=NONE, and the module loads in the job body can rewrite it. The
pre-submission check then passes on the login node while the compute node aborts
with "madengine not found in PATH".

Render the per-user bin directory and the directory the madengine console script
was resolved from at submission time into the generated script, so the job puts
the same interpreter back on PATH instead of relying on inheritance.
The single-node workspace probe matched \bnfs\b only, but df -T reports nfs4 on
most modern NFS mounts. A shared submission directory was therefore classified as
node-local and the job copied the whole project into /tmp instead of using the
shared path.

Match \bnfs[0-9]*\b so nfs, nfs3 and nfs4 are all recognized.

The rendered job script now also has coverage for the --gpus-per-node opt-out it
grew earlier in this batch: skip_gpus_directive shipped without tests, so nothing
failed if the directive crept back into the template. Both states of the flag are
asserted against the rendered script.
The shared-filesystem probe grepped the entire `df -T` output line, which carries the
mount point as well as the type. A local disk mounted at a path such as
/mnt/nfs-scratch therefore matched, the submission directory was classified as shared,
and the single-node job worked out of storage the other side of the run could not see.

Read the type column alone via `df --output=fstype` and anchor the pattern to it. The
option is GNU coreutils 8.21 and up, so an awk fallback over `df -T` covers older
systems.

beegfs and panfs join the list of shared types while the pattern is being rewritten;
both are common enough on HPC sites to be worth recognizing.
@mkuznet1
mkuznet1 requested review from coketaste and a lite review from Copilot August 7, 2026 14:19
@mkuznet1 mkuznet1 self-assigned this Aug 7, 2026
@mkuznet1
mkuznet1 requested a review from gargrahul August 7, 2026 14:23

Copilot AI left a comment

Copy link
Copy Markdown

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 improves SLURM portability in madengine by removing cluster-specific assumptions from the generated job script and hardening environment/probing logic so jobs submit and run consistently across more SLURM sites.

Changes:

  • Adds slurm.skip_gpus_directive to optionally omit #SBATCH --gpus-per-node for clusters that reject it.
  • Makes the generated SLURM job script more robust by re-establishing PATH inside the job and improving the shared-filesystem probe (fstype-only parsing and wider shared FS matching).
  • Caps the “informational” rocm-libs package queries to avoid hangs.

Reviewed changes

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

File Description
tests/unit/test_slurm_job_template.py Adds unit coverage for PATH exports, shared-fstype probe behavior, and skip_gpus_directive.
src/madengine/orchestration/run_orchestrator.py Caps rocm-libs package-manager queries to prevent interactive hangs.
src/madengine/deployment/templates/slurm/job.sh.j2 Updates SLURM job template to support skip_gpus_directive, re-add PATH inside the job, and harden shared-filesystem detection.
src/madengine/deployment/slurm.py Plumbs skip_gpus_directive, increases CLI probe timeout, and passes submission-time madengine bin dir into the template context.
Suppressed comments (3)

src/madengine/orchestration/run_orchestrator.py:773

  • Console.sh() already supports a timeout parameter; prefer it over the external timeout command to avoid depending on coreutils being present in the runtime environment.
            print(self.console.sh("timeout 10 yum info rocm-libs", canFail=True))

src/madengine/orchestration/run_orchestrator.py:775

  • Same as above: use Console.sh(..., timeout=10) rather than shelling out to timeout so the informational query is capped without adding a dependency on the timeout binary.
            print(self.console.sh("timeout 10 zypper info rocm-libs", canFail=True))

src/madengine/orchestration/run_orchestrator.py:777

  • Same as above: use Console.sh(..., timeout=10) rather than timeout 10 ... to keep this capped on distros/environments where timeout may be unavailable.
            print(self.console.sh("timeout 10 tdnf info rocm-libs", canFail=True))

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/madengine/deployment/templates/slurm/job.sh.j2
Comment thread src/madengine/deployment/templates/slurm/job.sh.j2
Comment thread src/madengine/orchestration/run_orchestrator.py
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