Revert #2127: contains patches / 回滚 #2127：包含补丁 #2164
+2
−4,140
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The revert restores a known-broken
conc-listformat at run-sweep.yml:539/554 for thesweep-multi-node-agenticjob that #2127 had explicitly fixed.matrix.config.concis a list for multi_node agentic entries (utils/matrix_logic/generate_sweep_configs.py:670 assignsFields.CONC.value: conc_batch), soconc-list: '[${{ matrix.config.conc }}]'produces malformed JSON that fails downstreamfromJsonin benchmark-multinode-tmpl.yml:154 — the sister workflow e2e-tests.yml:227/242 already uses the correcttoJson(matrix.config.conc)+matrix.config.conc[0]pattern. Currently dormant (no perf-changelog entry references a multi_node agentic config), so this doesn't block the revert, but consider preserving the two-line workflow fix out of the revert scope since it's orthogonal to the #2155 engine-patch policy motivating the revert.Extended reasoning...
What the bug is
PR #2127 (commit 0bd3fa4) bundled a workflow fix alongside the AMD agentic recipe: it changed
.github/workflows/run-sweep.yml:539fromconc-list: '[${{ matrix.config.conc }}]'→conc-list: ${{ toJson(matrix.config.conc) }}and removed the pairedconc: ${{ matrix.config.conc }}at line 554. This revert restores those two broken lines verbatim.Why the restored pattern is broken
matrix.config.concis a list for multi_node agentic entries. Inutils/matrix_logic/generate_sweep_configs.py:670,Fields.CONC.valueis set toconc_batch, which comes fromchunk_multinode_agentic_concurrencies()(lines 128-131) — this function slicesconc_valuesinto sublists and returnslist[list[int]]. So every matrix element gets.concas a JSON array like[32]or[4096].The downstream template requires a valid JSON array string.
.github/workflows/benchmark-multinode-tmpl.yml:154doesCONC_LIST: ${{ join(fromJson(inputs.conc-list), ' ') }}— this needsinputs.conc-listto be parsable JSON of a flat array of scalars. Interpolating a list expression inside literal brackets ('[${{ [32] }}]') either produces nested'[[32]]'or GitHub Actions'Arraystringification — neither works withfromJson.The sister workflow already has the correct pattern.
.github/workflows/e2e-tests.yml:227usesconc-list: ${{ toJson(matrix.config.conc) }}and line 242 usesconc: ${{ matrix.config.conc[0] }}(extracting the scalar via[0]) for the identical input shape. Post-revert,run-sweep.ymlande2e-tests.ymlare inconsistent for the same matrix shape.Step-by-step proof of the failure mode
Take
dsv4-fp4-gb300-dynamo-vllm-agentic(defined inconfigs/nvidia-master.yaml:12520withmultinode: trueandagentic-codingscenario) and imagine a future changelog entry referencing it:generate_sweep_configs.pycallschunk_multinode_agentic_concurrencies([32, 64])→[[32], [64]].config.conc = [32]or[64](a list).sweep-multi-node-agenticjob evaluates'[${{ matrix.config.conc }}]'— GHA stringifies the array, yielding malformed content (nested'[[32]]'or the literal tokenArray).benchmark-multinode-tmpl.yml:154runsfromJson(inputs.conc-list)on the malformed string — parse error, job fails immediately.Why this is dormant on this PR
The revert also removes
dsv4-fp4-mi355x-sglang-disagg-agentic-hicachefromperf-changelog.yamlandconfigs/amd-master.yaml, which was the only recent changelog entry exercising thesweep-multi-node-agenticjob.grepconfirms no other perf-changelog entry currently references any multi_node agentic config, so the broken code path is not triggered by this PR's own CI.However,
configs/nvidia-master.yamlstill defines multi_node agentic configs (dsv4-fp4-gb300-dynamo-vllm-agenticline 12520,dsv4-fp4-gb200-dynamo-vllm-agentic-3p2d-tep8-tp8line 14119). The next PR that adds any of these toperf-changelog.yamlwill silently hit the brokenfromJsonpath.Fix
Preserve the two-line workflow fix out of the revert scope. The full delta is trivial and orthogonal to the inference-engine-patch policy (#2155) motivating this revert:
conc-list: '[${{ matrix.config.conc }}]'→conc-list: ${{ toJson(matrix.config.conc) }}conc: ${{ matrix.config.conc }}line (the multi-node template doesn't consume a scalarconcinput for agentic runs whenconc-listis provided).Severity: nit
Dormant on this PR — nothing currently drives the broken code path. But the regression undoes a validated bug fix that was bundled into #2127, and the fix is a 2-line diff completely unrelated to the engine-patch policy scope of this revert. Worth surfacing so the author can decide whether to preserve the workflow fix or file a fast follow-up.