refactor: deduplicate benchmark internals / 重构：去除基准测试内部重复逻辑 - #2897
Conversation
Share fixed-sequence matrix construction and Slurm result collection. Extract the unchanged Kimi archive verifier, remove duplicate helpers, and cover shared runner utilities in CI while preserving existing interfaces. 中文：共享固定序列长度的矩阵构建和 Slurm 结果收集逻辑，提取内容不变的 Kimi 归档验证代码，删除重复辅助函数，并将共享运行器工具测试纳入 CI。 保持现有调用接口、配方格式和结果契约不变。
|
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 关于重新运行失败任务的文档 |
|
Claude finished @adibarra's task in 4m 51s —— View job PR Review
LGTM - no blocking issues found. Verified behavior preservation across all three refactor areas: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return expanded_entries | ||
|
|
||
|
|
||
| def _concurrency_range(start: int, end: int, step: int) -> list[int]: |
There was a problem hiding this comment.
📊 Line Count Report
- Total Lines: 1681
- Base Lines: 1815
- Change: 📉 -134 lines
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a large, three-part internal refactor touching core benchmark-matrix generation and Slurm launcher result-collection logic, a human look would still be worthwhile.
What was reviewed: verified copy_fixed_sequence_results() in runners/slurm_utils.sh is a byte-for-byte match of the previously duplicated inline block across all five launchers (including the two launchers that already sourced slurm_utils.sh and so needed no new source line); confirmed utils/evals/_kimi_verifier_archive.py is an exact copy of the removed Bash heredoc body; traced the new _fixed_sequence_entries()/_concurrency_range() helpers in generate_sweep_configs.py through both generate_full_sweep and generate_test_config_sweep (multi-node vs single-node, EP/max-ep mutation, ordering) and found them equivalent to the removed inline blocks; and confirmed the "write_command deleted but still used" candidate issue was actually a duplicate definition removed, not a full deletion (the original file defined write_command() twice).
Extended reasoning...
Overview
This PR is a behavior-preserving dedup refactor spanning three areas: (1) extracting a duplicated result-collection block from five Slurm launcher scripts into runners/slurm_utils.sh::copy_fixed_sequence_results(), with new tests; (2) moving the Kimi-Vendor-Verifier archive download/verify/extract Python heredoc out of benchmark_lib.sh into a standalone utils/evals/_kimi_verifier_archive.py, plus a helper rename; (3) deduplicating fixed-sequence matrix-row construction and concurrency-range expansion in utils/matrix_logic/generate_sweep_configs.py into shared helpers _fixed_sequence_entries() and _concurrency_range(), and reusing an existing freeze_config_value() instead of a duplicate _freeze_matrix_value().
Security risks
The only security-relevant code is the Kimi-Vendor-Verifier archive handling (SHA256 pinning, tarfile member-path sanitization, size limits) — this was moved verbatim, not modified, and I diffed the new file against the removed heredoc line-by-line and confirmed they match exactly aside from an added module docstring. No new injection, auth, or data-exposure surface is introduced elsewhere; the launcher and matrix-logic changes are pure control-flow extraction with no new external inputs.
Level of scrutiny
This warrants a closer-than-average look because it touches core benchmark-matrix generation (generate_sweep_configs.py) that determines what actually gets scheduled across the fleet, and five production Slurm launchers whose result-collection behavior directly affects which files land in CI artifacts. I traced the refactored matrix-construction paths (multi-node vs single-node, ep/max-ep mutation ordering, point-major vs runner-minor iteration order) against the removed inline code and did not find a discrepancy, and the five launcher diffs are mechanically identical extractions. However, the PR author's own description flags that full-sweep/eval validation is still pending, and this is exactly the kind of large multi-area refactor where a human sanity check on the CI run and the "25 before/after CLI comparisons" claim adds real value beyond what static diff review can confirm.
Other factors
Test coverage for the new slurm_utils.sh function is solid (filename/content preservation, empty-discovery, and errexit-preservation behavior, using real subprocess execution rather than mocks). No CODEOWNERS file was found restricting these paths. The bug-hunting pass reported no findings and exited via dry_streak (not a truncated budget), which supports the zero-findings read, but the combined size and criticality of the change is enough that I'm deferring rather than approving outright.
1 participant
Description / 说明
This first self-contained PR in the internal cleanup series removes duplicated matrix and result-collection logic while preserving existing caller and recipe interfaces.
这是内部清理系列的首个独立 PR：去除矩阵生成和结果收集中的重复逻辑，保持现有调用方式和 recipe 接口不变。
Production code shrinks by 304 lines; the total change removes 218 lines net. Recipes, images, schemas and CLI options are unchanged.
生产代码减少 304 行，计入测试等改动后净减少 218 行。recipe、镜像、schema 和 CLI 选项均未改变。
Validation / 验证
Unfiltered all-scenario generation still fails on both the baseline and this change because
cluster:b300-nvhardware metadata is missing. No GPU smoke, full sweep or inference-engine eval was run; merge validation remains pending.基线与本次改动在不筛选的全场景生成中均因缺少
cluster:b300-nv硬件元数据而失败，该既有问题未改变。本次未执行 GPU smoke、完整 sweep 或推理引擎 eval；合并前验证尚未完成。Related Issue / 关联 Issue
None. / 无。
Type of Change / 改动类型
Checklist / 检查清单
perf-changelog.yamlis untouched. / 性能 changelog：本次为保持行为不变的内部重构，不涉及 recipe 或基准测试性能行为调整；perf-changelog.yaml未改动，因此不适用。/reuse-sweep-runafter a green full sweep with passing evals. Not completed for this draft. / 通过复用路径合并前，须在完整 sweep 和 eval 均通过后，由有权限的维护者发布/reuse-sweep-run；此草稿尚未完成该步骤。Note
Low Risk
Behavior-preserving refactor of matrix generation and artifact staging; Kimi verifier logic is relocated, not rewritten. Risk is mainly regression in result filename patterns or sweep matrix output if shared helpers diverge from the old copies.
Overview
This PR deduplicates internal benchmark and CI plumbing without changing recipes, CLI options, or outward matrix contracts.
Sweep matrix generation centralizes fixed-sequence-length row building in
_fixed_sequence_entriesand concurrency expansion in_concurrency_range, reuses existingfreeze_config_valuefor multinode grouping keys, and routes full-sweep and test-config paths through the shared helpers.Slurm launchers (
launch_b200-nscale-compat,launch_b300-dsxe,launch_gb300-nv,launch_h100-dgxc-slurm,launch_h200-dgxc-slurm) sourcerunners/slurm_utils.shand callcopy_fixed_sequence_resultsinstead of inlined find/copy loops, with newrunners/test_slurm_utils.pycoverage.Kimi vendor verifier preparation moves from an inline heredoc in
benchmark_lib.shtoutils/evals/_kimi_verifier_archive.py(same download/validate/extract logic). MiniMax full-eval failure reporting uses the shared_write_minimax_vendor_integration_errorhelper.Changelog-gate CI watches the new runner/eval helper paths and runs
runners/test_slurm_utils.pyin the pytest job; eval dispatch tests setcwdfor the archive script path.Reviewed by Cursor Bugbot for commit 507393b. Bugbot is set up for automated code reviews on this repo. Configure here.