fix(evals): preserve nested lm-eval artifacts for sweep summaries - #2920
fix(evals): preserve nested lm-eval artifacts for sweep summaries#2920Ankur-singh wants to merge 1 commit into
Conversation
|
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 关于重新运行失败任务的文档 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes shared eval-artifact staging logic used by lm-eval, SWE-bench, and the vendor eval frameworks, a human look would still be worthwhile.
What was reviewed:
- The new recursive
find-based directory walk and multi-sourcestage_eval_artifactscall inappend_lm_eval_summary, plus the batched-eval early-return path (unaffected — it returns before the new loop runs). - The new regression test — it exercises the real bash function via subprocess, checks both positive staging of allowlisted files and negative exclusion of
debug.log, and confirms temp-dir cleanup. - The bilingual docs update — English and
_zhadditions are equivalent. - Noted but not blocking: two nested subdirectories producing same-named artifacts would silently overwrite via the flat
cp -f, and the walk is unbounded for large trees (e.g. SWE-bench output); neither is exercised by the new test.
Extended reasoning...
Overview
The diff is a focused fix to append_lm_eval_summary() in benchmarks/benchmark_lib.sh, which now enumerates all subdirectories of EVAL_RESULT_DIR via find -type d and passes them as additional sources to the existing stage_eval_artifacts() helper (which already accepted multiple sources and only copies an allowlisted set of filenames). This addresses a real gap: single-concurrency lm-eval writes results*.json/sample*.jsonl into a model-named subdirectory rather than the top-level output dir, so those artifacts were previously deleted unstaged. A new pytest test exercises the real bash function against a nested directory layout, verifying both positive staging and negative exclusion of a non-allowlisted file, and that the temp dir is removed. Both English and Chinese docs gained a matching one-sentence explanation.
Security risks
None identified. This is a benchmark artifact-staging path operating on local temp directories with no user-controlled paths, no shell injection surface beyond what already existed (glob expansion against a controlled allowlist), and no changes to auth, crypto, or permissions.
Level of scrutiny
The diff itself is small and self-contained, and the regression test directly targets the bug described. However, append_lm_eval_summary is shared code invoked from several distinct call sites (lm-eval batched sweeps, SWE-bench agentic eval-only runs, kimi-vendor/minimax-vendor/bfcl failure paths), so a change to its artifact-collection behavior has broad blast radius across eval frameworks that this PR's single test does not fully cover (e.g. the SWE-bench agentic path, where EVAL_RESULT_DIR can be a much larger, deeper directory tree than the lm-eval case the test covers).
Other factors
Two edge cases are visible from reading the code but are not covered by the new test and were not flagged as bugs by the automated hunt: (1) stage_eval_artifacts copies matches flatly per source directory, so two nested subdirectories producing identically-named artifacts (e.g. two results_<timestamp>.json) would silently overwrite one another; (2) the find walk is unconditional and unbounded in depth, so for eval trees with many subdirectories (e.g. SWE-bench per-instance directories) it will enumerate and glob-check every one, which is a scope/perf question rather than a correctness bug. Given the shared nature of this function and these untested edges, a human review pass adds value even though no concrete bug was found.
1 participant
Description
Single-concurrency lm-eval can write
results*.jsonandsample*.jsonlunder a model-named subdirectory ofEVAL_RESULT_DIR.append_lm_eval_summarypreviously staged only files immediately insideEVAL_RESULT_DIRand then deleted that temporary directory.This left affected per-config artifacts with
meta_env.jsonbut without the raw result and sample files. Becausecollect-evalsneeds the result JSON to extract scores, the sweep produced no eval rows: the Eval Summary table was missing andagg_eval_all.jsonwas empty even though lm-eval had written results. The temporary-directory cleanup also made those unstaged results unavailable for later diagnosis.The fix enumerates directories inside the dedicated
EVAL_RESULT_DIRand passes each one through the existing shallow, allowlistedstage_eval_artifactshelper before cleanup. Nested result and sample files are flattened into the workspace root for the workflow's existing upload patterns. The shared staging helper remains shallow, so callers that inspect broad directories do not recursively collect unrelated files.A regression test exercises the real summary function with nested result and sample files. It verifies that both files survive, an unrelated log remains excluded, and the temporary result directory is removed only after staging. The English and Chinese eval procedure guides now document this behavior.
Related Issue
N/A
Type of Change
Validation
uv run --with pytest --with pyyaml pytest -q utils/evals/test_run_eval_dispatch.py— 105 passedbash -n benchmarks/benchmark_lib.shgit diff --check origin/main...HEADChecklist
perf-changelog.yamland have not edited historical entries (not applicable: this changes post-eval artifact collection only)OWNER/MEMBER/COLLABORATOR) has commented/reuse-sweep-runon this PR. Do this only once there is a final full sweep that is all green with evals passing, since after this comment the sweep label will no longer automatically kick off new sweeps. Remove and re-add the label to force one.Note
Low Risk
Post-eval artifact staging only; no inference or scoring logic changes. Risk is limited to whether directory enumeration could stage from unexpected paths under EVAL_RESULT_DIR.
Overview
Fixes single-concurrency lm-eval runs where
results*.jsonandsample*.jsonlland under a model-named subdirectory ofEVAL_RESULT_DIR.append_lm_eval_summarynow discovers every directory under that temp tree and passes each to the existing allowlistedstage_eval_artifactshelper (still shallow per directory), so nested outputs are flattened to the workspace root before the temp dir is removed.Without this, sweeps could upload
meta_env.jsonbut miss raw results, leavingcollect-evalswith no scores and an empty Eval Summary /agg_eval_all.json. A regression test covers nested staging, exclusion of non-allowlisted files, and cleanup timing; English and Chinese eval procedure docs note the behavior.Reviewed by Cursor Bugbot for commit 822299a. Bugbot is set up for automated code reviews on this repo. Configure here.