Skip to content

refactor: share AgentX matrix generation / 共用 AgentX 矩阵生成逻辑 - #2903

Merged
adibarra merged 2 commits into
mainfrom
refactor/shared-agentic-matrix
Sep 9, 2026
Merged

refactor: share AgentX matrix generation / 共用 AgentX 矩阵生成逻辑#2903
adibarra merged 2 commits into
mainfrom
refactor/shared-agentic-matrix

Conversation

@adibarra

@adibarra adibarra commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Description

full-sweep and test-config duplicated AgentX worker defaults, offload budgeting, concurrency expansion, experiment naming, node counts, and row validation. Both now use one _agentic_entries builder. 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

  • Bug fix
  • New feature
  • Configuration change
  • Documentation update
  • Other: behavior-preserving internal refactor

Validation

  • Full local suite: 1,130 passed + 219 subtests, zero skips, 48.93 seconds.
  • The exact same 160 generator tests pass on frozen ac93c763b and the refactored implementation.
  • 440 actual CLI comparisons against 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.
  • Two isolated negative controls are rejected by the tests: changing multi-node runner/point order and deferring offload validation until after empty concurrency filtering.
  • Test Changelog Gate and CodeQL pass on 0bf325e89. The dedicated matrix CI job is skipped while this PR is a draft; its generator tests passed locally. git diff --check passes.

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

  • I have tested my changes locally
  • I have updated documentation if necessary
  • For every change that can affect benchmark performance and every recipe addition or modification, I have appended a new entry to the physical end of perf-changelog.yaml and have not edited historical entries
  • Before merging via reuse, an authorized maintainer (OWNER/MEMBER/COLLABORATOR) has commented /reuse-sweep-run on 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-sweeptest-config 重复实现了 AgentX worker 默认值、卸载预算、并发展开、实验命名、节点数和矩阵行验证。两个命令现在共用 _agentic_entries 构建逻辑,保留各命令的选择规则以及现有 CLI 和配方接口。

生成器净减少 107 行。共用测试覆盖两个命令及单节点、聚合式和分离式多节点部署,包括运行器与并发点顺序、重复并发点、范围边界、命令特有的过滤规则、输入不被修改,以及并发过滤为空前仍需验证卸载预算的行为。中英文架构指南已同步。

这是继 #2897#2901 之后的第三批内部清理,类型为保持行为不变的内部重构及文档更新。

验证结果:

  • 完整本地测试 1,130 个通过,另有 219 个子测试通过,零跳过,耗时 48.93 秒
  • 同一组 160 个生成器测试在冻结的 ac93c763b 和重构实现上均通过。
  • 相对 ac93c763b 执行 440 个真实 CLI 对比:364 个成功案例的 stdout 和 stderr 逐字节一致;76 个拒绝案例在归一化 traceback 位置及源码摘录后,退出码和诊断一致。覆盖受控输入和当前所有 AgentX 配置;已有配置目录中的拒绝案例保持原行为,不代表这些配置成功生成。
  • 两个隔离反例均被测试发现:改变多节点运行器与并发点的顺序,以及将卸载验证推迟到并发过滤为空之后。
  • 0bf325e89 上的 Test Changelog GateCodeQL 均通过。独立的矩阵 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.py into a single _agentic_entries helper used by both full-sweep and test-config, mirroring the existing shared _fixed_sequence_entries pattern.

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/--conc filters only remove existing points and do not synthesize a capped concurrency value (unlike some fixed-sequence max_conc behavior).

Callers still pass command-specific filters (step_size, min_conc/max_conc vs conc_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.

共用 full-sweep 与 test-config 的 AgentX 矩阵构建逻辑,保留并发过滤、运行器顺序、卸载预算、实验名称、节点数及验证时机。将已有行为测试扩展到两个命令,并同步中英文架构说明。
@adibarra
adibarra marked this pull request as ready for review September 9, 2026 01:10
@adibarra
adibarra requested a review from a team September 9, 2026 01:10
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @adibarra's task in 3m 26s —— View job


Review — PR #2903

  • Gather context (AGENTS.md, PR diff vs origin/main)
  • Review generate_sweep_configs.py refactor for behavior changes/bugs
  • Review shared test changes
  • Run generator tests locally — blocked: pytest is not in my allowed tools; verified statically instead (see note below)
  • Post Line Count Report inline comment
  • Post review summary

LGTM - no blocking issues found.

I verified the _agentic_entries extraction line-by-line against both removed loops on origin/main and found no behavioral divergence:

  • Dict insertion order (hence JSON key order) matches the old entries exactly for single-node, aggregated, and disaggregated paths, including the conditional disagg / kv-offload-backend placement.
  • validate_agentic_matrix_entry returns the same dict it receives (utils/matrix_logic/validation.py:433), so entries.append(validate_agentic_matrix_entry(entry)) is identical to the old full-sweep pattern of validate-then-append-raw.
  • Point ordering is preserved: multi-node stays runner-major, single-node stays conc-major; duplicate conc-list values are preserved.
  • Topology (multinode_worker_pair) and the offload budget (agentic_dram_offload_gb) are still resolved before concurrency filtering, so invalid deployments still fail even when all points are filtered out — matching the old behavior in both commands, and now covered by a parametrized test.
  • Command semantics are preserved: full-sweep gets step_size/min_conc/max_conc (with the old agentic behavior of no min_conc <= 0 special case, unlike fixed-seq-len), test-config keeps hardcoded step 2 and only the exact --conc filter. The offload budget still uses the config's base runner label, not the expanded node name.
  • No dangling references to the removed locals (is_multinode, image, disagg, …) remain in generate_test_config_sweep.

The new shared test fixtures (agentic_mode × agentic_config) run each assertion across both commands and all three deployment shapes, use controlled inputs, and don't freeze recipe inventories — consistent with the AGENTS.md test-quality policy. Docs changes in docs/architecture.md and docs/architecture_zh.md are synchronized and accurate.

Note on local test execution: I could not run pytest in this environment because the command requires approval under the current --allowedTools. If you want future reviews to include a local test run, allow Bash(python -m pytest:*) in the workflow config. Mitigating this: the Test Changelog Gate and CodeQL passed on 0bf325e89, and the dedicated matrix CI job should now run since the PR left draft state.

return entries


def _agentic_entries(

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.

📊 Line Count Report

  • Total Lines: 1574
  • Base Lines: 1681
  • Change: 📉 -107 lines

@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'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 矩阵重构分支。
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

1 participant