Skip to content

refactor: standardize srt-slurm cluster setup hooks - #3398

Closed
cquil11 wants to merge 3 commits into
codex/single-node-srt-slurmfrom
agent/srt-slurm-cluster-hooks
Closed

cquil11 wants to merge 3 commits into
codex/single-node-srt-slurmfrom
agent/srt-slurm-cluster-hooks

Conversation

@cquil11

@cquil11 cquil11 commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Moves the srt-slurm cluster hook convention from #2542 onto #3352.

  • runners/srt-slurm/hooks/<cluster>/setup.sh holds per-allocation host checks and setup; the cluster profile invokes it through default_host_setup.
  • runners/srt-slurm/hooks/common.sh holds shared host-check functions. wait_for_amd_gpu_clean moves there from benchmark_lib.sh, which now sources it.
  • MI355X gets its hook: RDMA/QoS/DCQCN preflight, GPU drain gate, and reclaim of unused hugepages. launch_mi355x-amds.sh passes GITHUB_WORKSPACE to the profile renderer for the hook path.
  • Documents the convention in AGENTS.md and docs/configuration-procedures.md.
  • MI300X: the MEC firmware check moves from the profile's default_bash_preamble to hooks/mi300x-amd/setup.sh. The hook fails when firmware is older than 177 instead of setting HSA_NO_SCRATCH_RECLAIM inside every container. The cluster's nodes report 192.

@github-actions

Copy link
Copy Markdown
Contributor

Thanks for the contribution!

  • Review: If this PR changes files owned by someone other than a repository admin or @SemiAnalysisAI/core, ask one eligible CODEOWNER to complete the latest PR_REVIEW_CHECKLIST.md before contacting a core maintainer on Slack. Follow the template exactly, including As a PR reviewer and CODEOWNER, I have reviewed this and have, so sign-off verification triggers.
  • PR verification: Sweeps only run on labeled PRs. Add full-sweep-fail-fast (strongly recommended); use full-sweep-enabled only when matrix jobs should continue after a failure.
  • After merging: PR authors must ensure all GitHub Actions jobs pass. Transient failures often pass on rerun; see how to rerun failed jobs.
中文

感谢你的贡献!

  • **审阅:**如果 PR 修改的文件归属于仓库管理员及 @SemiAnalysisAI/core 之外的 CODEOWNER,请先联系一位有资格的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md,再通过 Slack 联系核心维护者。必须严格遵循模板,并保留 As a PR reviewer and CODEOWNER, I have reviewed this and have,才能触发签核验证。
  • **PR 验证:**扫描仅在带有标签的 PR 上运行。强烈建议添加 full-sweep-fail-fast;仅当需要矩阵任务在失败后继续运行时才使用 full-sweep-enabled。
  • **合并后:**PR 作者必须确保所有 GitHub Actions 任务通过。临时性失败通常可以通过重新运行恢复;参见重新运行失败任务的说明。

@claude claude Bot left a comment

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.

I reviewed this PR and didn't find any bugs. Because it touches host-level setup that runs with sudo on production Slurm allocations (RDMA/QoS preflight and hugepage reclamation), a human look would still be worthwhile.

What was reviewed: the new RDMA/QoS/DCQCN preflight in check-rdma.sh (checked that nicctl failures now hard-fail instead of the base script's graceful skip); the hugepage reclaim logic in setup.sh (checked it only shrinks nr_hugepages to used+reserved and never restores the prior pool size); the new VRAM-drain gate's default 10% threshold for MI355X native-single-node runs; and the hard dependency of benchmark_lib.sh on sourcing common.sh via return 1 on failure.

Extended reasoning...

The PR refactors srt-slurm cluster setup hooks: extracts wait_for_amd_gpu_clean into a shared common.sh, adds a new sudo-invoked RDMA/QoS/DCQCN preflight and a hugepage-reclaim step for MI355X, and documents the hook convention in AGENTS.md and docs/configuration-procedures.md. It touches no auth/crypto surface but does run privileged host commands (sudo nicctl, writing /proc/sys/vm/nr_hugepages) on shared Slurm nodes, which is the security/operational-sensitive part. No findings were reported by the bug hunt, but the change is host-setup code with sudo and irreversible-looking side effects (hugepage pool shrink with no restore), so a human familiar with the cluster's operational risk should still confirm intent before merge.

This review covers commit 9d89131, which is no longer the latest commit on this pull request; later commits are not covered by it.

@cquil11

cquil11 commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator Author

Folded into #2542.

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

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant