[Power] feat: measure GPU power for single-node AgentX runs / 为单机 AgentX 运行接入 GPU 功耗测量 - #2601
[Power] feat: measure GPU power for single-node AgentX runs / 为单机 AgentX 运行接入 GPU 功耗测量#2601edwingao28 wants to merge 3 commits into
Conversation
中文：将 AIPerf profiling 生命周期与成功请求计数适配到严格功耗校验契约。
中文：在共享 AgentX 重放生命周期中采集单节点 GPU 功耗，并在异常退出时可靠清理监控进程。
|
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 关于重新运行失败任务的文档 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15a7b705d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case "${ENABLE_AGENTX_POWER:-1}" in | ||
| 1|true|TRUE|yes|YES) | ||
| if [ "${IS_MULTINODE:-false}" != "true" ]; then | ||
| agentx_power_enabled=1 | ||
| fi |
There was a problem hiding this comment.
Enabling 1 Hz GPU monitoring by default changes every single-node AgentX benchmark's execution environment and can affect the reported performance, but this commit leaves perf-changelog.yaml byte-for-byte unchanged. Append a new entry at the physical tail so consumers can distinguish results collected with this additional monitoring load.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
| trap '_stop_agentx_power_monitor' EXIT | ||
| trap '_stop_agentx_power_monitor; exit 130' INT |
There was a problem hiding this comment.
When the replay subprocess group receives SIGTERM, Bash can run this TERM handler and subsequently the installed EXIT handler without retaining the guard update between trap contexts, so stop_gpu_monitor is invoked twice. This makes the newly enabled CI test flaky: the targeted signal test failed in 8 of 12 local repetitions with two monitor-stop events. Clear the EXIT trap before performing signal cleanup, or otherwise make cleanup reliably single-shot.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the two nits flagged inline, this run also examined and ruled out three additional candidates: audit-sidecar loss in _record_adapter_failure when agg_result is missing, silent power_validation.json loss when the agg-result file is missing during a window-build failure, and whether a DST transition mid-replay could cause the fixed launch-time UTC offset to misinterpret the AIPerf window. None were confirmed as bugs.
Extended reasoning...
This run reviewed the AgentX power-measurement PR (window adapter in power_adapter.py, GPU-monitor lifecycle wiring in benchmark_lib.sh, and schema-version stamping across aggregate_power.py/aggregate_power_multinode.py/process_result.py). Only two nits were found (a duplicated failure-handling block in run_agentic_power, and a CI path-filter gap that misses transitive test dependencies), both maintainability issues rather than correctness bugs. Three additional candidate issues surfaced by finder agents — sidecar loss on missing agg_result, sidecar loss on missing agg-result during window-build failure, and DST-transition mishandling of the captured UTC offset — were investigated and refuted by verifier agents, so they are recorded here rather than re-explored in a future pass.
| def run_agentic_power( | ||
| *, | ||
| result_dir: Path, | ||
| agg_result: Path, | ||
| expected_num_gpus: int | None, | ||
| require_power: bool = False, | ||
| ) -> int: | ||
| """Validate AgentX power telemetry, failing only in strict mode.""" | ||
| window, reasons = build_power_window(result_dir) | ||
| if window is None: | ||
| try: | ||
| _record_adapter_failure( | ||
| result_dir=result_dir, | ||
| agg_result=agg_result, | ||
| expected_num_gpus=expected_num_gpus, | ||
| reasons=reasons, | ||
| ) | ||
| except (OSError, json.JSONDecodeError, ValueError) as exc: | ||
| print( | ||
| f"[agentx_power] Failed to record adapter failure: {exc}", | ||
| file=sys.stderr, | ||
| ) | ||
| print( | ||
| f"[agentx_power] Power-window adaptation failed: {', '.join(reasons)}", | ||
| file=sys.stderr, | ||
| ) | ||
| return 1 if require_power else 0 |
There was a problem hiding this comment.
🟡 run_agentic_power() has two near-identical ~12-line blocks (lines 175-191 and 198-214) that each call _record_adapter_failure() inside the same try/except, print the same failure/adaptation messages, and return the same 1 if require_power else 0. Extracting a single helper (e.g. _fail_adapter(result_dir, agg_result, expected_num_gpus, reasons, require_power) -> int) called from both branches would remove the duplication with no behavior change.
Extended reasoning...
What the duplication looks like
In utils/agentic/aggregation/power_adapter.py, run_agentic_power() contains two blocks with essentially identical control flow:
- Lines 175-191, triggered when
build_power_window()returnswindow is None. - Lines 198-214, triggered when writing
agentic_power_window.jsonraisesOSError.
Both blocks do exactly the same four things in the same order:
- Call
_record_adapter_failure(result_dir=..., agg_result=..., expected_num_gpus=..., reasons=...)inside atry/except (OSError, json.JSONDecodeError, ValueError). - On exception, print
[agentx_power] Failed to record adapter failure: {exc}to stderr. - Unconditionally print
[agentx_power] Power-window adaptation failed: {', '.join(reasons)}to stderr. - Return
1 if require_power else 0.
The only thing that differs between the two call sites is the source of the reasons list: in the first block it comes from build_power_window()'s return value, and in the second it's hardcoded to ['power_window_unwritable'].
Why this matters
This isn't a correctness bug — both code paths behave identically today, and the tests (test_run_agentic_power_records_window_write_failure_before_returning and test_run_agentic_power_records_adapter_failure_before_returning) cover both independently and pass. It's a maintainability issue: any future change to this failure-handling sequence (e.g. adding a new field to the sidecar payload, changing the log message format, or — as another reviewer noted — fixing the sidecar-loss defect where _record_adapter_failure's own exception handler doesn't write a fallback power_validation.json) has to be made in two places, and it's easy to update one copy and forget the other.
Suggested fix
Extract a small helper:
def _fail_adapter(
*,
result_dir: Path,
agg_result: Path,
expected_num_gpus: int | None,
reasons: list[str],
require_power: bool,
) -> int:
try:
_record_adapter_failure(
result_dir=result_dir,
agg_result=agg_result,
expected_num_gpus=expected_num_gpus,
reasons=reasons,
)
except (OSError, json.JSONDecodeError, ValueError) as exc:
print(f"[agentx_power] Failed to record adapter failure: {exc}", file=sys.stderr)
print(f"[agentx_power] Power-window adaptation failed: {', '.join(reasons)}", file=sys.stderr)
return 1 if require_power else 0and call it from both the window is None branch and the OSError branch in run_agentic_power(), passing the appropriate reasons list at each call site. This is a pure refactor with no behavioral change — existing tests should pass unmodified.
Proof of duplication (step-by-step)
- Read lines 165-191: on
window is None, the code enters atrycalling_record_adapter_failure(...), catches the same three exception types, prints the same two messages, returns the same expression. - Read lines 192-214: after
_write_json_atomic(window_path, window)raisesOSError,reasonsis reassigned to['power_window_unwritable'], and then the exact same four-step sequence (try/except/print/return) repeats verbatim. - Diff the two blocks textually — every line matches except the value bound to
reasonsbefore entering the block. - This satisfies the DRY criterion: identical logic, differing only by one input parameter, is a textbook candidate for extraction into a shared function.
| - 'runners/launch_gb300-nv.sh' | ||
| - 'utils/aggregate_power.py' | ||
| - 'utils/aggregate_power_multinode.py' | ||
| - 'utils/agentic/aggregation/power_adapter.py' | ||
| - 'utils/agentic/aggregation/test_power_adapter.py' | ||
| - 'utils/agentic/aggregation/test_power_lifecycle.py' | ||
| - 'utils/agentic/aggregation/process_agentic_result.py' | ||
| - 'utils/agentic/aggregation/test_process_agentic_result.py' | ||
| - 'utils/pytest.ini' | ||
| - 'utils/bench_serving/benchmark_serving.py' | ||
| - 'utils/process_result.py' | ||
| - 'utils/test_aggregate_power.py' |
There was a problem hiding this comment.
🟡 The pull_request path filter for this new CI job lists power_adapter.py, process_agentic_result.py, and their test files, but omits the modules those files import transitively: utils/agentic/aggregation/request_metrics.py (extract_per_record_ints, load_aggregate, load_records), plus aggregation_common.py, server_metrics.py, and server_log_metrics.py. A change to any of these transitive dependencies (e.g. tweaking extract_per_record_ints filtering in request_metrics.py) would silently skip the power/agentic test suite this PR just wired in. Consider adding those paths, or switching to a glob like 'utils/agentic/aggregation/**'.
Extended reasoning...
What the bug is: .github/workflows/test-process-result.yml gained six new path-filter entries in this PR to trigger the newly-wired agentic/power test suite on pull_request. The filter lists power_adapter.py, test_power_adapter.py, test_power_lifecycle.py, process_agentic_result.py, test_process_agentic_result.py, and pytest.ini — but it does not list the modules those production files import.
The code path: utils/agentic/aggregation/power_adapter.py (added by this PR) contains from .request_metrics import extract_per_record_ints, load_aggregate, load_records. utils/agentic/aggregation/process_agentic_result.py (an existing file whose tests are newly wired in here) imports from .aggregation_common, .request_metrics, .server_log_metrics, and .server_metrics; request_metrics.py itself pulls in trace_metadata.py transitively. None of request_metrics.py, aggregation_common.py, server_metrics.py, or server_log_metrics.py appear anywhere in the path filter (lines 14-25), and a repo-wide grep across .github/workflows confirms no other workflow covers them either.
Why nothing else catches this: the path filter is an allowlist gate on pull_request triggers — GitHub Actions simply won't start the job unless a changed file matches one of the listed globs. There's no fallback trigger (like a periodic or push-to-main run) configured in this workflow that would catch a miss here on a PR that only touches, say, request_metrics.py.
Impact: a PR that edits request_metrics.py — for example changing the filtering logic in extract_per_record_ints (used directly by the new power_adapter.build_power_window) or adjusting load_records — would not trigger test-process-result.yml at all. The 195+ tests covering the agentic/power suite this PR just wired in would silently not run on that PR, even though the change could break power_adapter.py's window derivation or process_agentic_result.py's aggregation.
Step-by-step proof:
- Suppose a follow-up PR changes only
utils/agentic/aggregation/request_metrics.py, e.g. tightening the token-count validation inextract_per_record_ints. - GitHub evaluates the
pull_request.pathsfilter intest-process-result.ymlagainst the changed file list:['utils/agentic/aggregation/request_metrics.py']. - None of the filter's globs (lines 14-25) match that path —
power_adapter.py,test_power_adapter.py, etc. are file-specific entries, not directory globs. - The workflow does not run for that PR; CI shows no failure because the job was never scheduled.
- If the
extract_per_record_intschange silently breakstest_power_adapter.py'stest_build_power_window_rejects_ambiguous_inputscase, that regression ships to main undetected until some later, unrelated PR happens to touch one of the explicitly-listed files and the job finally runs against stale-broken code.
Fix: add utils/agentic/aggregation/request_metrics.py, aggregation_common.py, server_metrics.py, and server_log_metrics.py to the path list, or replace the file-by-file enumeration with a directory glob such as utils/agentic/aggregation/** so future additions to that package don't require another manual path-filter update.
1 participant
What
Measured GPU power for single-node AgentX runs — the largest uninstrumented slice of the fleet (agentic dominates recent sweeps; fixed-seq single-node has been 100% instrumented for months, agentic 0%).
Two pieces:
1. Window adapter (
utils/agentic/aggregation/power_adapter.py)AgentX has no fixed benchmark window — requests stream through a replay. The adapter derives a formal power window from the AIPerf profile export and hands it to the existing
aggregate_power.pyintegrator, so agentic runs reuse the exact validation, integration, and fail-closed machinery fixed-seq runs already use.The subtle part: AIPerf exports naive local datetimes while SMI stamps host wall clock. The launch-time UTC offset is captured (
date +%z) at replay start and attached explicitly before normalizing; naive times without a valid captured offset are rejected (profile_timezone_offset_missing/invalid) rather than guessed.2. Lifecycle wiring (
benchmarks/benchmark_lib.sh)run_agentic_replay_and_write_outputsstarts/stops the sharedstart_gpu_monitoraround the replay. Default on (ENABLE_AGENTX_POWER, unset/empty → on), fail-closed on any unmatched value, multinode excluded (IS_MULTINODE=truerecipes never enter). Monitor stop is covered on every exit path (explicit stop + EXIT/INT/TERM traps + idempotency flag). No per-script changes — all 40+ agentic launch scripts inherit this through the shared library.Invalid/missing telemetry keeps the ordinary best-effort contract:
power_valid: 0, benchmark unaffected,REQUIRE_POWER=1to fail closed.Depends on
Stacked on #2599 (schema version stamp) — first commit here is that PR. Review the last two commits. Merge #2599 first.
CI note
The agentic aggregation suites were not previously wired into any workflow.
test-process-result.ymlnow runs them;utils/pytest.inigainspythonpath = ..because these tests import through the repo-root package path (utils.agentic....) while CI invokes pytest fromutils/. Both invocation modes verified.Verification
Shell wiring independently reviewed: unset-var handling, multinode guard, monitor-stop coverage on all exit paths, subshell locality of monitor globals,
expected_num_gpusconvention match.