Skip to content

[PowerX] tidy the #3781 power consumers and trim their driver test / [PowerX] 整理 #3781 功耗消费端并精简其 driver 测试 - #3798

Open
edwingao28 wants to merge 3 commits into
feat/powerx-amd-upstream-572from
chore/powerx-amd-572-quality
Open

edwingao28 wants to merge 3 commits into
feat/powerx-amd-upstream-572from
chore/powerx-amd-572-quality

Conversation

@edwingao28

Copy link
Copy Markdown
Collaborator

Summary

Stacked on #3781 with base feat/powerx-amd-upstream-572; merge into that branch, not main.

Three readability changes, no behavior change. Samples CSV version detection and row length derive from one SAMPLES_HEADERS table, so a future v4 is one entry. The AgentX adapter is named for native packages, run_native_agentic_power, with its GPU-topology decision in _power_topology instead of two interleaved branches. The single-node driver test runs one cluster per exporter kind; test_srt_config already covers the three AMD records.

Testing: full CI pytest command and ruff pass locally.

AI model disclosure
  • Model/version: claude-fable-5-1 (Claude Code)
  • Role: implementation and validation
PR checklist and change type

Type of Change

  • Bug fix
  • New feature
  • Configuration change
  • Documentation update
  • Other (please describe): readability refactor and test trim

Checklist

  • I have completed the AI model disclosure and kept it current
  • 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 inferencex-e2e/perf-changelog.yaml and have not edited historical entries
  • If this PR can affect benchmark performance or adds or modifies a recipe, it carries exactly one primary sweep label (a maintainer applies it on fork PRs): full-sweep-fail-fast (recommended), full-sweep-enabled, or non-canary-full-sweep-enabled. Optional modifiers all-evals, evals-only, and agentx-fast require a primary label; the last two block reuse while applied.
  • Before merging via reuse, an authorized maintainer (OWNER/MEMBER/COLLABORATOR) has commented /use <run_id> (or the legacy /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 primary sweep label will no longer automatically kick off new sweeps. Remove and re-add the primary sweep label to force a new sweep.
中文

基于 #3781,base 为 feat/powerx-amd-upstream-572;请合入该分支,而非 main。

三项可读性整理,不改变行为。samples CSV 的版本识别与行长度统一来自 SAMPLES_HEADERS 一张表,日后新增 v4 只需加一项。AgentX adapter 按原生功耗包命名为 run_native_agentic_power,GPU 拓扑判断集中在 _power_topology,不再分散在两段交错分支。单节点 driver 测试每种 exporter 只跑一个集群;三个 AMD 集群已由 test_srt_config 覆盖。

测试: 本地完整 CI pytest 命令与 ruff 均通过。

AI 模型:claude-fable-5-1(Claude Code),负责实现与验证。

@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.

LGTM — this is a pure refactor with no behavior change, and I verified the two logic extractions preserve original semantics.

  • Verified _power_topology in power_adapter.py returns outputs identical to the inlined original logic for both the single-node (expected_num_gpus set) and multinode (disagg/aggregate) branches, including the invalid-topology paths that route into the existing reasons failure flow.
  • Verified SAMPLES_HEADERS dict consolidation in multinode.py is a mechanical equivalent of the prior three-way if/elif and inline-dict-literal lookups.
  • Confirmed the dropped mi325x-amd/mi300x-amd parametrize cases in test_srt_driver.py are justified: both used the same custom exporter kind/port as the retained mi355x-amds case (so added no new exporter-kind coverage to that test), and test_srt_config.py still parametrizes over all three AMD clusters for config-rendering coverage.
  • The remaining two test files are mechanical rename-only updates (run_multinode_agentic_power → run_native_agentic_power) with no change to test bodies.
Extended reasoning...

The diff is a readability-only refactor across two source files (power_adapter.py, multinode.py) and three test files, touching power-telemetry validation logic with no auth/crypto/data-exposure surface. I manually traced both extracted helpers (_power_topology and SAMPLES_HEADERS) against the original inline logic line-by-line and confirmed behavioral equivalence, and checked that the trimmed test_srt_driver.py parametrize cases are still covered by test_srt_config.py as the author claims. No findings were reported by the bug hunt, the change is small and mechanical, and there are no outstanding reviewer objections in the timeline.

@functionstackx
functionstackx added this pull request to stack #3844 October 8, 2026 22:11
@edwingao28
edwingao28 force-pushed the chore/powerx-amd-572-quality branch from 7899c87 to fe41eaf Compare October 9, 2026 20:13
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

inferencex-e2e/infx diff: +67 -49

Generally most InferenceX submissions don't need to add changes to the infx package. On the off chance they do, generally the diff is small. Please prompt your agent to clean your PR to keep your diff to only necessary & minimal changes too. We have an increased code quality bar for changes to the infx package.

中文

大多数 InferenceX 提交通常不需要修改 infx 包。即使确实需要,改动通常也很小。请让你的 agent 清理 PR,使改动只包含必要且最小的内容。infx 包的改动有更高的代码质量要求。

中文:samples CSV 的版本识别与行校验改为共用同一张表头表。
The entry point serves single-node points too, and the GPU-topology decision
now lives in one function instead of two interleaved branches.

中文:入口函数同时服务单节点测试点,故按原生功耗包命名;GPU 拓扑判断集中到一个函数,不再分散在两段交错分支中。
中文:单节点原生功耗矩阵测试缩减为每种 exporter 一个集群;三个 AMD 集群渲染一致由 test_srt_config 覆盖。
@edwingao28
edwingao28 force-pushed the chore/powerx-amd-572-quality branch from fe41eaf to c84aec3 Compare October 9, 2026 20:15

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant