[AMD] exclude mia1-p01-g09 from AMD multi-node Slurm jobs#2283
[AMD] exclude mia1-p01-g09 from AMD multi-node Slurm jobs#2283seungrokj wants to merge 2 commits into
Conversation
…jobs Add two additional problematic nodes to the default SLURM_EXCLUDE_NODES list in the AMD multi-node submit script. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
1 similar comment
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
seungrokj
changed the title
|
cc. @hongxiayang |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — trivial, mechanical change (adding two hostnames to a default exclude list); the one finding is a repo-wide PR-metadata nit unrelated to the code correctness.
Extended reasoning...
Overview
Single-line change to benchmarks/multi_node/amd_utils/submit.sh: appends mia1-p01-g09 and mia1-p01-g14 to the default value of SLURM_EXCLUDE_NODES (a comma-separated string used to build the --exclude sbatch argument). This only changes the default — any caller that already sets SLURM_EXCLUDE_NODES in the environment is unaffected, and the mechanism for consuming the list (splitting on commas into --exclude) is unchanged.
Security risks
None. This is a benchmarking/ops script that composes a Slurm sbatch exclude list; there is no user input, auth, or secret handling involved.
Level of scrutiny
Low. This is a one-line data change (extending a hostname list) in a non-critical benchmark orchestration script, following the exact existing pattern (comma-separated hostnames already excluded for the same reason — broken Docker sockets). No control-flow or logic changes.
Other factors
The only finding from the bug-hunting pass is a nit that the PR title/description lacks the repo-mandated Chinese translation section — this is a metadata/process issue, not a code defect, and doesn't bear on correctness of the fix. The reviewed candidate issue about leaving other documented bad nodes (g19/g31/g37) unexcluded was already ruled out by the verifier pass as out of scope for this fix. Given the change is small, self-contained, and mechanical, I'm approving despite the nit.
| # Optional: exclude specific nodes (e.g. nodes with broken Docker sockets). | ||
| # Set SLURM_EXCLUDE_NODES env var to a comma-separated list of hostnames. | ||
| EXCLUDE_OPT=() | ||
| SLURM_EXCLUDE_NODES="${SLURM_EXCLUDE_NODES:-mia1-p01-g11,mia1-p01-g12,mia1-p01-g15}" | ||
| SLURM_EXCLUDE_NODES="${SLURM_EXCLUDE_NODES:-mia1-p01-g11,mia1-p01-g12,mia1-p01-g15,mia1-p01-g09,mia1-p01-g14}" | ||
| if [[ -n "${SLURM_EXCLUDE_NODES:-}" ]]; then | ||
| EXCLUDE_OPT=(--exclude "$SLURM_EXCLUDE_NODES") | ||
| fi |
There was a problem hiding this comment.
🟡 This PR's title and description are English-only, but AGENTS.md mandates bilingual PR titles/descriptions (<English> / <中文标题> format plus a ## 中文说明 section). This is a pre-existing, repo-mandated documentation requirement unrelated to the actual code fix, which is correct.
Extended reasoning...
AGENTS.md (line 7) explicitly states: "PR and GitHub-issue titles & descriptions must be bilingual — include a Simplified Chinese version in addition to English. Title format: <English title> / <中文标题>. In the PR/issue body, follow the English content with its Chinese translation (e.g. a ## 中文说明 section mirroring the summary...)." This applies to every PR and every issue in the repository, per that same section.
This PR's title is "fix: exclude mia1-p01-g09 and mia1-p01-g14 from AMD multi-node Slurm jobs" — English only, with no / <中文标题> suffix. The PR body contains only ## Summary and ## Test plan sections in English; there is no accompanying ## 中文说明 section translating the summary into Simplified Chinese. Both requirements from AGENTS.md are unmet.
This is not a subjective style nit invented by the reviewer — it's an explicit, verifiable, repo-mandated checklist item that applies uniformly to all PRs, independent of the code change itself. The underlying code fix (adding mia1-p01-g09 and mia1-p01-g14 to the default SLURM_EXCLUDE_NODES list on line 169 of benchmarks/multi_node/amd_utils/submit.sh) is correct and not in question — the issue is purely with the PR metadata that accompanies it.
Step-by-step proof:
- Open AGENTS.md and read the callout on line 7: it requires
<English title> / <中文标题>for the title, and a## 中文说明section (or equivalent trailing Chinese paragraph) in the body. - Read this PR's actual title: "fix: exclude mia1-p01-g09 and mia1-p01-g14 from AMD multi-node Slurm jobs" — no
/ <中文标题>segment is present. - Read this PR's actual body: it contains
## Summary(English bullet) and## Test plan(English checklist) only — no## 中文说明or any Chinese text anywhere. - Therefore the PR fails both the title-format and body-translation requirements mandated by AGENTS.md.
Impact and fix: There's no runtime or functional impact — the code change itself (excluding the two broken nodes from Slurm scheduling) is unaffected and correct. The fix is simply for the PR author to append / <中文标题> to the title (e.g. "fix: exclude mia1-p01-g09 and mia1-p01-g14 from AMD multi-node Slurm jobs / 修复:在 AMD 多节点 Slurm 任务中排除 mia1-p01-g09 和 mia1-p01-g14") and add a ## 中文说明 section mirroring the Summary/Test plan in Simplified Chinese before merging.
seungrokj
changed the title
Node mia1-p01-g14 is healthy again, remove it from the default exclude list to expand the available runner pool. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Evidence of failing PD on these nodes
https://github.com/SemiAnalysisAI/InferenceX/actions/runs/29683845216/job/88211152187?pr=2247
Summary
mia1-p01-g09andmia1-p01-g14to the defaultSLURM_EXCLUDE_NODESlist in the AMD multi-node submit scriptTest plan
🤖 Generated with Claude Code