Skip to content

[PowerX] scope required power to selected 8k1k scenarios / 按所选 8k1k 场景要求功耗 - #3050

Merged
edwingao28 merged 2 commits into
mainfrom
feat/powerx-scoped-required-power
Sep 12, 2026
Merged

[PowerX] scope required power to selected 8k1k scenarios / 按所选 8k1k 场景要求功耗#3050
edwingao28 merged 2 commits into
mainfrom
feat/powerx-scoped-required-power

Conversation

@edwingao28

@edwingao28 edwingao28 commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

Description

Add an opt-in measured-power requirement to individual fixed 8192/1024 scenarios and propagate it through standard sweeps and E2E jobs. Existing recipes, AgentX and eval-only behavior remain unchanged.

Testing: Matrix CI passed at 671b0c1e5; two alias regressions failed before the fix, all four focused cases pass. No GPU run or recipe activation; runtime qualification and CODEOWNER approval remain required.

中文

为独立固定 8192/1024 场景增加可选的实测功耗要求,并贯通标准 sweep 和 E2E 作业。现有配方、AgentX 和 eval-only 行为保持不变。

测试: 671b0c1e5 的矩阵 CI 通过;两个字段别名回归用例在修复前失败,修复后四个针对性用例全部通过。未运行 GPU 或启用配方;仍需各 runtime 资格验证与 CODEOWNER 审批。

Related Issue

Shared prerequisite for #3027 runtime splits.

#3027 runtime 拆分的共享前置。

Type of Change

  • Bug fix
  • New feature
  • Configuration change
  • Documentation update
  • Other (please describe)

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.

Note

Low Risk
CI matrix and generator changes only; eval-only and non-8k1k paths are explicitly excluded, with no auth or data-handling impact.

Overview
Adds an opt-in require-power flag on fixed-sequence master YAML (validated and emitted only for 8192/1024 rows) so benchmark matrix jobs can request validated measured power without changing AgentX or default behavior.

The generator copies the flag onto throughput matrix rows, strips it from eval-only paths (smoke, subset/all eval modes), and rejects require-power on other sequence lengths. Pydantic schemas accept the field on sequence configs and matrix entries.

CI wiring: run-sweep and manual E2E throughput jobs pass require-power when the matrix row sets it and the job is not eval-only; E2E also honors the workflow input or per-row flag. Docs (EN/ZH) describe rollout constraints. Tests cover single/multi-node and both key spellings.

Reviewed by Cursor Bugbot for commit 671b0c1. Bugbot is set up for automated code reviews on this repo. Configure here.

为固定序列场景增加默认关闭的功耗要求,并贯通矩阵及工作流;保持 AgentX 和评估隔离。
@edwingao28
edwingao28 marked this pull request as ready for review September 12, 2026 09:25
@edwingao28
edwingao28 requested a review from a team September 12, 2026 09:25
@claude

claude Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Claude finished @edwingao28's task in 3m 9s —— View job


Review of PR #3050

  • Read PR diff and surrounding code
  • Trace require-power through generate.py, validation.py, workflows
  • Run matrix tests (blocked: the sandbox denied pytest; relying on the author's reported 317 passing)
  • Line count report for infx/matrix/generate.py
  • Post review

LGTM - no blocking issues found.

Verified: the flag is emitted only from _fixed_sequence_entries, so AgentX rows never carry it; it is stripped on all three eval-only paths (smoke_entries, select_matrix_evals subset/all); trim_conc and the eval markers copy rows with {**entry} so the key survives; and in run-sweep.yml the YAML anchors defined on the 1k1k jobs propagate the new input to the 8k1k jobs, so canary, single-node and multi-node throughput jobs all receive it. Both benchmark templates already declare a boolean require-power input, so the reusable-workflow call is valid. No master config or perf-changelog.yaml changes, so no changelog entry is required.

One non-blocking observation: the 8192/1024 restriction is enforced in _fixed_sequence_entries at generation time, not in SingleNodeSeqLenConfig/MultiNodeSeqLenConfig, so a misplaced require-power: true passes master-config validation and only fails when that scenario is generated. Fine for an opt-in prerequisite PR, but worth a model_validator before recipes start using it.

Comment thread infx/matrix/generate.py
isl, osl = sequence[Fields.ISL.value], sequence[Fields.OSL.value]
require_power = sequence.get(Fields.REQUIRE_POWER.value, False)
if require_power and (isl, osl) != (8192, 1024):
raise ValueError("require-power rollout supports only fixed-sequence 8192/1024")

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: 1594
  • Base Lines: 1587
  • Change: 📈 +7 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.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread infx/matrix/validation.py
Comment on lines 632 to +635

isl: int
osl: int
require_power: bool = Field(default=False, alias=Fields.REQUIRE_POWER.value, strict=True)

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.

🟡 (optional) SingleNodeSeqLenConfig/MultiNodeSeqLenConfig set populate_by_name=True, so a master config authored with require_power: true (snake_case) passes schema validation silently, but infx/matrix/generate.py:812 reads the raw yaml dict via sequence.get('require-power', False) (hyphen only) since validate_master_config returns the original dict, not the pydantic-normalized one. A misspelled key silently disables the power-validation requirement with no error anywhere. Fix: reject the non-alias spelling for this field (or normalize the validated model back into the dict actually consumed by generation) so config validation and the generator agree on one spelling for require-power and its multinode sibling at line 646.

Extended reasoning...

A config author writes require-power style keys everywhere else (matches Fields enum's kebab-case convention) but types require_power: true for this new field, following Python naming instead. SingleNodeSeqLenConfig(**entry) accepts it via populate_by_name and validate_master_config (infx/matrix/validation.py:957) returns the original master_configs dict unchanged (not the validated model). generate.py's _fixed_sequence_entries then does sequence.get(Fields.REQUIRE_POWER.value, False) i.e. sequence.get('require-power', False), which misses the snake_case key and returns False. The generated matrix entry has no require-power, so no ValueError, no REQUIRE_POWER env var, and CI never enforces the measured-power qualification the author believed they had enabled -- a silent no-op for a safety/qualification gate, with no diagnostic anywhere in the pipeline.

Verification: nit. The mechanism is real and reachable. validation.py:631 and :642 set ConfigDict(extra='forbid', populate_by_name=True) on SingleNodeSeqLenConfig/MultiNodeSeqLenConfig, and the new field is require_power: bool = Field(default=False, alias='require-power', strict=True) (lines 635, 646). With populate_by_name=True, pydantic accepts the snake_case field name require_power as valid input;…

矩阵生成同时识别校验允许的字段名与别名,避免静默丢失功耗门槛。
@edwingao28
edwingao28 merged commit 9c6b26e into main Sep 12, 2026
9 checks passed
@edwingao28
edwingao28 deleted the feat/powerx-scoped-required-power branch September 12, 2026 19:53
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