refactor: share AgentX matrix generation / 共用 AgentX 矩阵生成逻辑 - #2903
Conversation
共用 full-sweep 与 test-config 的 AgentX 矩阵构建逻辑,保留并发过滤、运行器顺序、卸载预算、实验名称、节点数及验证时机。将已有行为测试扩展到两个命令,并同步中英文架构说明。
|
Claude finished @adibarra's task in 3m 26s —— View job Review — PR #2903
LGTM - no blocking issues found. I verified the
The new shared test fixtures ( Note on local test execution: I could not run |
| return entries | ||
|
|
||
|
|
||
| def _agentic_entries( |
There was a problem hiding this comment.
📊 Line Count Report
- Total Lines: 1574
- Base Lines: 1681
- Change: 📉 -107 lines
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a fairs-sized refactor consolidating AgentX matrix-row generation for two CLI commands into one shared helper, a human look would still be worthwhile.
What was reviewed: traced the new _agentic_entries helper against both removed inline blocks in generate_full_sweep/generate_test_config_sweep for behavioral equivalence (point/runner ordering, min/max/exact concurrency filtering, DRAM-offload budget resolution, multinode kv-offload suffixes/node-count handling, validation timing before empty-filter short-circuit). Confirmed validate_agentic_matrix_entry's return value is the same dict object, so switching one call site to use its return value is a no-op. Checked that the new shared tests' expected concurrency sequences (e.g. step_size multiplicative range behavior in _concurrency_range) are computed independently rather than mirroring the implementation. Confirmed the added architecture.md/architecture_zh.md paragraphs accurately describe the code's validate-before-filter and ordering-preservation behavior.
Extended reasoning...
Overview
The PR refactors utils/matrix_logic/generate_sweep_configs.py to extract a shared _agentic_entries helper used by both generate_full_sweep and generate_test_config_sweep, replacing two nearly-identical ~120-line inline blocks. The corresponding test file is restructured with a new TestAgenticGeneration class and parametrized fixtures (agentic_mode, generate_agentic_sweep, agentic_config) covering both commands across single-node, aggregated-multinode, and disaggregated-multinode deployments. docs/architecture.md/docs/architecture_zh.md gain one synchronized paragraph documenting the new shared ownership boundary.
Security risks
None. This is internal build/config-generation tooling with no user input from untrusted sources, no auth, crypto, or network-facing code involved.
Level of scrutiny
I gave this a close manual trace given the size and behavior-preserving intent: compared the new _agentic_entries function line-by-line against both original inline blocks (single-node and multinode paths, concurrency filtering order, offload-budget computation using the base runner value rather than per-entry runner, exp-name construction, validation-before-empty-filter ordering) and found no discrepancy. I also verified the one call-site difference (using validate_agentic_matrix_entry's return value at both sites now, vs. only one site previously) is behaviorally inert since that function returns the same dict it receives. The new tests' expected values (e.g., _concurrency_range(3, 10, step) producing [3, 9, 10] for step=3) were independently recomputed rather than accepted at face value, and matched the multiplicative-range semantics in the actual implementation.
Other factors
The change is a large, multi-file refactor of core sweep-generation logic that many downstream benchmark configs depend on, restructures a sizeable test suite, and the PR itself is marked as a draft with a skipped dedicated CI matrix job. Even though my trace and the automated bug hunter found nothing, the size and centrality of this code (it determines what benchmark configurations get generated and run) makes a human sanity check worthwhile before merge, especially since the draft CI job for this path has not yet run.
将最新 main 合并到 AgentX 矩阵重构分支。
Description
full-sweepandtest-configduplicated AgentX worker defaults, offload budgeting, concurrency expansion, experiment naming, node counts, and row validation. Both now use one_agentic_entriesbuilder. Command selection and the existing CLI/recipe interfaces remain unchanged.The generator removes 107 lines. The shared tests cover both commands and single-node, aggregated, and disaggregated deployments, including ordering, duplicate points, range boundaries, command-specific filters, input preservation, and validation before filtered-empty output. English and Chinese architecture guides are synchronized.
Related Issue
Third internal cleanup in the series, following #2897 and #2901.
Type of Change
Validation
ac93c763band the refactored implementation.ac93c763b: 364 successful cases have byte-identical stdout and stderr; 76 rejections retain exit status and diagnostics after normalizing traceback locations/source excerpts. Controlled inputs and every current AgentX config are exercised. Existing catalog rejections are preserved, not claimed as successful generation.0bf325e89. The dedicated matrix CI job is skipped while this PR is a draft; its generator tests passed locally.git diff --checkpasses.The behavior tests ship in this PR. The one-off CLI comparison harness, raw outputs, frozen baseline, and mutation evidence are retained locally. These checks cover controlled cases, not arbitrary-input equivalence or GPU execution. No GPU sweep was run.
Checklist
perf-changelog.yamland have not edited historical entriesOWNER/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.No intended benchmark-performance or recipe change; no performance-changelog entry or sweep reuse is claimed. This is a draft for review.
中文说明
full-sweep与test-config重复实现了 AgentX worker 默认值、卸载预算、并发展开、实验命名、节点数和矩阵行验证。两个命令现在共用_agentic_entries构建逻辑,保留各命令的选择规则以及现有 CLI 和配方接口。生成器净减少 107 行。共用测试覆盖两个命令及单节点、聚合式和分离式多节点部署,包括运行器与并发点顺序、重复并发点、范围边界、命令特有的过滤规则、输入不被修改,以及并发过滤为空前仍需验证卸载预算的行为。中英文架构指南已同步。
这是继 #2897 和 #2901 之后的第三批内部清理,类型为保持行为不变的内部重构及文档更新。
验证结果:
ac93c763b和重构实现上均通过。ac93c763b执行 440 个真实 CLI 对比:364 个成功案例的 stdout 和 stderr 逐字节一致;76 个拒绝案例在归一化 traceback 位置及源码摘录后,退出码和诊断一致。覆盖受控输入和当前所有 AgentX 配置;已有配置目录中的拒绝案例保持原行为,不代表这些配置成功生成。0bf325e89上的 Test Changelog Gate 和 CodeQL 均通过。独立的矩阵 CI Job 在草稿状态下跳过,其生成器测试已在本地通过。git diff --check通过。行为测试包含在本 PR 中;一次性 CLI 对比脚本、原始输出、冻结基线及反例证据保留在本地。这些检查只证明所覆盖案例的一致性,不代表对任意输入或 GPU 执行的完整证明。未执行 GPU sweep。
本次没有预期的 benchmark 性能变化或配方改动,因此未新增性能变更日志,也不声明 sweep 复用。本 PR 为待审阅草稿。
Note
Medium Risk
Changes CI matrix JSON for all AgentX sweeps; risk is mitigated by extensive parity tests and CLI comparisons, but any subtle ordering or filter semantic drift would affect which benchmark jobs run.
Overview
Consolidates duplicated AgentX (agentic-coding) matrix expansion in
generate_sweep_configs.pyinto a single_agentic_entrieshelper used by bothfull-sweepandtest-config, mirroring the existing shared_fixed_sequence_entriespattern.The helper centralizes worker defaults, DRAM offload budgeting, concurrency expansion (list or range), experiment naming, multinode node counts, row ordering (runner-major for multinode, conc-major for single-node), and
validate_agentic_matrix_entry. Topology and offload validation run before concurrency filtering, so invalid deployments still fail even when filters would drop all points; min/max/--concfilters only remove existing points and do not synthesize a capped concurrency value (unlike some fixed-sequencemax_concbehavior).Callers still pass command-specific filters (
step_size,min_conc/max_concvsconc_filter). English and Chinese architecture docs add a short invariant paragraph under “Matrix derivation has one implementation.”Tests gain parametrized coverage for both commands and single/aggregated/disaggregated AgentX topologies, including ordering, duplicate conc points, range boundaries, and validation-on-filtered-empty cases.
Reviewed by Cursor Bugbot for commit 310afe2. Bugbot is set up for automated code reviews on this repo. Configure here.