refactor: centralize results in infx and speed up tests / 集中结果处理并加速测试 - #2940
Conversation
Extract composable result builders, component metadata parsing, and power metric transformations while preserving legacy CLI and artifact behavior. Expand independent behavior checks and shorten tests with controlled waits and bounded parallel execution. 将可组合的结果构建函数、组件元数据解析和功耗指标转换集中到 infx, 保留现有 CLI 和产物行为。补充独立的行为测试,并通过可控等待和 有限并行执行缩短测试时间。
|
Claude finished @adibarra's task in 5m 29s —— View job Review of PR #2940
LGTM - no blocking issues found. I traced the old Note: I could not execute the test suite in this environment (Bash approval restrictions on |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a large refactor of core result/power aggregation code plus a CI concurrency change, a human look would still be worthwhile.
What was reviewed:
- The new
infx.resultspackage (fixed_sequence, metadata, power) and howaggregate_power.py,aggregate_power_multinode.py,process_result.py, andprocess_agentic_result.pynow route through its shared metadata/power-metric logic. - The switch to 4
pytest-xdistworkers intest-changelog-gate.yml— checkedutils/evals/test_run_eval_dispatch.py's HOME/env-mutation tests for cross-worker interference; each test scopes its own temp state, so no shared-state race was evident. aggregate_power_multinode.py's new top-levelfrom infx.results.power import (...)lacks thesys.pathguardaggregate_power.pyhas for direct execution; it works today only becauseutils/pytest.inisetspythonpath = ..and no current caller invokes the script directly outside pytest.
Extended reasoning...
Overview
This PR is a substantial internal refactor: it extracts result-processing logic (fixed-sequence result building, component metadata validation, power-metric replacement) that was duplicated across utils/process_result.py, utils/agentic/aggregation/process_agentic_result.py, and the power aggregation scripts into a new infx/results package (fixed_sequence.py, metadata.py, power.py), then routes the existing CLI entry points through the shared implementation. It also changes CI test execution to use pytest-xdist (4 workers) for the changelog-gate workflow, and updates docs and several test files to match. Twenty files changed across production code, tests, workflows, and docs — a wide but conceptually cohesive change.
Security risks
No injection, auth, or data-exposure surface is touched — this is internal benchmark result/telemetry post-processing and test infrastructure. The main risk class here is correctness/behavioral regression (e.g., silently changing exit codes, JSON schema, or power validity semantics that downstream tooling depends on) and CI-concurrency-induced test flakiness, not classic security vulnerabilities.
Level of scrutiny
This warrants a moderate-to-high level of scrutiny: it is a large, multi-file refactor of code that governs benchmark power/energy metric integrity (REQUIRE_POWER, power_valid, energy accounting) and it changes how CI parallelizes tests, both of which have already produced one plausible latent issue found during review — aggregate_power_multinode.py gained a top-level infx.results.power import with no fallback sys.path guard, unlike its sibling aggregate_power.py, which does have one for direct-script execution. Under the test suite this is masked by utils/pytest.ini's pythonpath = .., and no current caller runs the script directly outside pytest, so it isn't exercised as a live bug today — but it is an inconsistency worth a maintainer's eyes given the stated goal of preserving direct-execution CLI compatibility.
Other factors
The PR description cites extensive local validation (1,223 tests, 531 CLI comparisons, actionlint) but this is unverified self-reported testing from the author, not independent evidence. Given the scope of the refactor, the power/metadata semantics involved, and the one inconsistency noted above, I'm deferring rather than approving even though the automated bug hunt reported no confirmed bugs.
Description / 说明
Result processors duplicated component metadata validation and power-metric replacement, while fixed-sequence conversion was only available through a script. This change exposes composable functions in
infx.resultsand routes existing processors through the shared rules.结果处理器重复实现了组件元数据校验和功耗指标替换,固定序列长度的结果转换也只能通过脚本调用。本次将可组合的函数集中到
infx.results,让现有处理器复用相同规则。infx.results.fixed_sequence.build_resultaccepts explicit, read-only inputs; CLI adapters own environment and artifact I/O. Shared metadata parsing preserves each caller's validation policy, and power transformations accept the metric family and schema version.Existing commands, environment variables, output JSON, failure diagnostics, exit codes, and best-effort power fallback remain compatible on the tested cases. Recipe maintainers need no changes or new runtime dependencies.
Test fixtures control clocks and wait for observable readiness while retaining real process termination, HTTP requests, and artifact writes. The changelog CI job uses four
pytest-xdistworkers; parameter collection is deterministic.infx.results.fixed_sequence.build_result接收显式只读输入,CLI 适配层负责环境变量和产物读写。共享元数据解析保留各调用方的校验策略,功耗转换由调用方传入指标集合和模式版本。在已验证用例中,现有命令、环境变量、输出 JSON、失败诊断、退出码和尽力完成的功耗处理回退行为保持兼容。配方维护者无需调整,也没有新增运行时依赖。
测试 fixture 控制时钟,并根据可观察到的就绪状态结束等待,同时保留真实的进程终止、HTTP 请求和产物写入。changelog CI 任务使用四个
pytest-xdistworker,参数化用例收集顺序保持确定。Related Issue / 关联事项
Continues the internal package cleanup from #2911. / 延续 #2911 的内部包结构整理。
Type of Change / 变更类型
Validation / 验证
Rebased onto
8260f95de; final commitf6f726a7107eae3cd5482ad240ae40b308847b20:python -m pytest utils/ runners/ experimental/CollectiveX/tests/ -n 4— 1,223 passed, 207 subtests passed, zero skips, 13.93s locally.Compatibility baseline
b51c65807: the same 254 behavior tests passed before and after extraction; two additional tests cover the new importable APIs. The relevant pre-refactor production files are unchanged between that baseline and the rebased main.531 CLI comparisons: 81 captured test invocations, 446 boundary cases, and four direct power CLI checks. Exit status, stdout, JSON artifacts, and diagnostics matched after normalizing temporary paths and traceback frames; exception types, messages, and chaining were retained.
Independent expected values cover arithmetic, rounding, malformed input rejection, metadata policies, stale power removal, and internal-error fallback. Negative controls confirm malformed aggregate coercion and missing process termination are detected; six concurrent repeat runs passed the 12 lifecycle checks.
Before the rebase added upstream tests, the same 1,192-test suite went from 43.91s to 10.39s locally through controlled waits and four workers. This is a local execution comparison, not hosted CI duration.
actionlintpasses for both changed workflows;git diff --checkpasses.已变基到
8260f95de;最终提交f6f726a7107eae3cd5482ad240ae40b308847b20运行上述完整命令,本地 1,223 个测试及 207 个子测试通过,无跳过,耗时 13.93s。兼容性基线为
b51c65807:提取前后相同的 254 个行为测试均通过,另有两个测试覆盖新的可导入 API。该基线与变基后的 main 之间,相关的重构前生产代码没有变化。531 次 CLI 对照包括 81 次测试调用、446 个边界用例和四项直接调用功耗 CLI 的检查。规范化临时路径和 traceback 帧后,退出状态、stdout、JSON 产物和诊断一致;保留了异常类型、消息及异常链。
使用独立预期值验证计算、舍入、无效输入拒绝、元数据策略、旧功耗字段清理和内部错误回退。负向对照确认测试能发现错误的聚合数据强制转换及缺失的进程终止;六次并发重复运行中的 12 项生命周期检查全部通过。
在变基引入上游新测试前,同一组 1,192 个测试通过可控等待和四个 worker,本地耗时从 43.91s 降至 10.39s。这是本地执行对比,并非托管 CI 的总耗时。
两个修改后的工作流均通过
actionlint,git diff --check通过。Checklist / 检查清单
Note
Medium Risk
Touches benchmark aggregate JSON and power-metric shaping on fixed-sequence and AgentX paths; behavior is intended to stay CLI-compatible but regressions would affect CI ingest contracts.
Overview
Introduces
infx.resultsso fixed-sequence aggregation, component metadata parsing, and power-metric replacement are importable library functions instead of duplicated script logic.utils/process_result.pybecomes a thin CLI shim toinfx.results.fixed_sequence, whileaggregate_power,aggregate_power_multinode, and AgentX aggregation route throughwith_power_metricsandparse_component_metadata.Tests and CI add
pytest-xdist(-n 4in the changelog-gate job), mock clocks / readiness-based waits to avoid hung workers, and expand coverage for builders, power scrubbing, and multinode power fallback.test-process-result.ymlnow triggers oninfx/results/**. Architecture and testing docs (EN/ZH) document the new extension points and parallel local runs.Reviewed by Cursor Bugbot for commit f6f726a. Bugbot is set up for automated code reviews on this repo. Configure here.