Skip to content

feat(agentx): support cross-GPU reference comparisons - #1484

Merged
ZhengGong-amd merged 11 commits into
mainfrom
feature/leixin/agentx-compare-gpu
Sep 14, 2026
Merged

ZhengGong-amd merged 11 commits into
mainfrom
feature/leixin/agentx-compare-gpu

Conversation

@zengleixin-amd

Copy link
Copy Markdown
Contributor

Summary

  • Match AgentX reference rows by workload type instead of fixed ISL/OSL and fetch derived P90 metrics by benchmark ID.
  • Compute exact external-comparison P90 from request records while preserving internal grading semantics.
  • Compare same-concurrency, same-precision evidence using accepted measurement provenance and conservative per-GPU total throughput normalization.
  • Surface consistent advisory and final-report comparisons, including missing-data reasons and prompt/report switch separation.

Verification

  • 378 focused comparison, mapper, client, analyzer, advisory, report and executor tests passed before the latest main merge.
  • Syntax, Ruff lint/format and diff checks passed for the feature changes.
  • GEAK promotion and source-propagation fixes were checked with actual-method AST reproductions.
  • Full Linux Coordinator/GEAK regression remains unverified: local Windows lacks fcntl and the previous Linux validation Pod is unavailable.
  • Live derived-metrics API and GPU end-to-end validation have not been run.
  • Latest main was merged without textual conflicts; CI is required to validate the merged revision.

Scope

  • No changes to internal Objective/KEEP/REVERT policy or CI configuration.
  • Single-node pure-TP comparison scope; unsupported partition/topology normalization is reported unavailable.
  • Synchronous advisory HTTP offloading and low-priority cleanup remain deferred.
  • Temporary state and handoff documents are excluded.

🤖 Generated with Claude Code

Zeng and others added 7 commits September 11, 2026 09:46
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Align the AgentX comparison implementation with the current main branch.

Co-Authored-By: Claude <noreply@anthropic.com>


def test_agentx_analysis_joins_p90_before_selecting_one_real_reference(tmp_path, monkeypatch):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta

@pytest.mark.parametrize("derived", [{"1": None}, {}, None])
def test_agentx_analysis_preserves_total_when_p90_is_unavailable(tmp_path, monkeypatch, derived):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta


def test_agentx_p90_selection_is_stable_and_never_prefers_missing_metric(tmp_path, monkeypatch):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta

@pytest.mark.parametrize("benchmark_id", [None, "invalid", 0, True])
def test_agentx_rows_without_joinable_id_keep_only_throughput(tmp_path, monkeypatch, benchmark_id):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta

@pytest.mark.parametrize("precision", ["mxfp4", "nvfp4"])
def test_agentx_precision_alias_is_explicitly_an_upstream_bucket(tmp_path, monkeypatch, precision):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta
],
)
def test_agentx_no_match_diagnostics_do_not_request_derived_metrics(tmp_path, monkeypatch, rows, reason):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta


def test_agentx_no_target_persists_marker_without_network(tmp_path, monkeypatch):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta


def test_synthetic_analysis_never_fetches_agentic_p90(tmp_path, monkeypatch):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta


def test_agentx_analysis_joins_p90_before_selecting_one_real_reference(tmp_path, monkeypatch):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta

@pytest.mark.parametrize("derived", [{"1": None}, {}, None])
def test_agentx_analysis_preserves_total_when_p90_is_unavailable(tmp_path, monkeypatch, derived):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta


def test_agentx_p90_selection_is_stable_and_never_prefers_missing_metric(tmp_path, monkeypatch):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta

@pytest.mark.parametrize("benchmark_id", [None, "invalid", 0, True])
def test_agentx_rows_without_joinable_id_keep_only_throughput(tmp_path, monkeypatch, benchmark_id):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta

@pytest.mark.parametrize("precision", ["mxfp4", "nvfp4"])
def test_agentx_precision_alias_is_explicitly_an_upstream_bucket(tmp_path, monkeypatch, precision):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta
],
)
def test_agentx_no_match_diagnostics_do_not_request_derived_metrics(tmp_path, monkeypatch, rows, reason):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta


def test_agentx_no_target_persists_marker_without_network(tmp_path, monkeypatch):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta


def test_synthetic_analysis_never_fetches_agentic_p90(tmp_path, monkeypatch):
import hyperloom.inference_optimizer.baseline_comparison.target_analyzer as ta
@ZhengGong-amd

Copy link
Copy Markdown
Collaborator

Closing the agentx_not_supported short-circuit is the right goal, and matching
agentic rows is real work this repo needs. But most of this diff recomputes
numbers the pipeline already produces and already grades on. Please redraw
rather than patch: the business code should land around 150-200 lines, not 761,
and local_measurement.py should not exist.

0. Blocking: verify the upstream contract before anything else.
benchmark_type, agentic_traces, derived-agentic-metrics and
p90_e2e_norm_intvty appear nowhere on main, and the PR description says the
live endpoint was never called. Also settle whether an agentic row's
metrics.tput_per_gpu is output or total tokens — the synthetic path reads that
same field as output, you reinterpret it as total, and on an agentic corpus the
two differ by roughly 1 + ISL/OSL. Paste one real /benchmarks row and one
real derived response into the PR. Until then the 283 passing tests only prove
the fixtures agree with themselves. Related: drop the new
benchmark_type in (None, "single_turn") filter on the synthetic branch — it
can only subtract rows from a path that works today.

1. One definition of e2e_norm_intvty_p90. mapping.py:102 already emits
it as pct(m, "e2e_output_token_throughput", "p10"), and its comment states
that this is 1/P90(E2EL/OSL) per MODELS.md:78 — the identical quantity your
comparison_metrics() recomputes from profile_export.jsonl. It already flows
to measurement and to current_best["e2e_norm_intvty_p90"]; your own
load_local_measurement reads it to cross-check. Delete the 87 lines in
map_aiperf.py and read current_best. Right now one JSON carries this key
twice from two sources, the session grades on one and the report prints the
other. If you believe the turn eligibility differs, fix the one definition in
agentx/mapping.py — don't park a second one beside it, least of all in a file
whose docstring calls itself a thin shim over that module.

2. One source of GPU count. state.tp — same as the synthetic path, same as
dispatcher._resolve_serving_tp() and serving_tp_for_policy(), both of which
exist to answer exactly "how many cards is this server holding". _recipe_scope
re-derives it from a YAML digest, eight parallelism envs, a framework allowlist
and a shlex.split over two server-arg strings. Either state.tp is
trustworthy (then it is trustworthy here too) or it isn't (then synthetic
per-GPU normalization is wrong and that's the PR to write). There is no third
option keyed on benchmark_mode. With this and #1 gone, the local side is two
lines: current_best["total_throughput"] / state.tp and
current_best["e2e_norm_intvty_p90"].

3. Trust the accepted measurement. current_best["tput"],
measurement["tput"] and the raw output_throughput are filled by one
_merge_raw_result call from one file; re-verifying them against each other
guards a state that would already have corrupted KEEP/REVERT. Same for the
before/after fstat on a file aiperf finished writing before the CLI started,
and for source_sha256, which nothing reads. Drop all three, and the reason
codes that only those checks can emit.

4. One agentx predicate. agentx_active(state) — you use it correctly in
target_analysis.py, then hand-inline is_agentx_mode(...) or env_bool("HYPERLOOM_AGENTX") again in report.py:935 and
research_hints.py:374. Three copies of a rule whose docstring exists to
explain why both signals are needed.

5. One competitor-target assembly, one gap vocabulary.
_external_baseline_with_comparison rebuilds a target dict in memory from
external_baseline, bypassing _coerce_per_conc; have the report read
competitor_target.json like the advisory does. And primary_gap must stay
"latency" / "throughput"match_variants_to_priors tests
primary_gap == "latency" (research_hints.py:543), so your new
"interactivity" silently loses the variant-ordering hint on exactly the
sessions this PR targets.

6. Reuse the numeric helpers, drop the escape hatch. perf_metric._positive
is line-for-line your _positive; coerce.optional_positive_int covers
_positive_int; _to_num is already in research_hints. Also use isinstance
rather than type(x) in (int, float) — that idiom is why mypy gained three new
errors at inferencex_client.py:193. Separately, for_report=True exists only
to bypass target_advisory_enabled; if the report legitimately ignores that
switch, make the caller decide, don't add a parameter that routes around it.

7. Whatever survives gets documented and typed. _agentx_gap is 60 lines,
four untyped params, nine exits, no docstring. target_analyzer's module
docstring still says rows are matched on isl/osl and that analyze is the
only function the executor calls (it now also imports _persist and
to_inferencex_name — promote them or don't). analyze() and gap_analysis()
gained parameters with no Args: entries; _dedup_by_conc lost its. Collapse
the ~30 reason strings to the handful that remain, put them in a Literal, and
log the failures — local_measurement.py currently has zero log calls, so
recipe_mismatch reaches the user as a bare token with nothing behind it. Sync
SKILL.md:659 and optimization-loop.md; no docs changed in this PR.

The comparison is worth having. What should merge is "we can now match agentic
reference rows", not a parallel measurement stack next to the one that decides
KEEP.

Zeng and others added 3 commits September 11, 2026 15:14
Establish an in-progress JSON snapshot before upstream work, preserve it on terminal write failure, and treat Markdown as a best-effort projection. Propagate storage errors without claiming a successful refresh.

Co-Authored-By: Claude <noreply@anthropic.com>
Remove the new derived-P90 type-check errors while retaining the existing finite-positive numeric response contract.

Co-Authored-By: Claude <noreply@anthropic.com>
Render controlled per-axis explanations in advisory and final reports, retain usable metric gaps, and keep unknown reason text out of prompts.

Co-Authored-By: Claude <noreply@anthropic.com>
@ZhengGong-amd

Copy link
Copy Markdown
Collaborator

The three new commits move in the opposite direction from the review. Nothing
blocking was addressed, and the diff grew by another 450 lines.

To be direct: the review asked you to delete code, not to explain it better.

# Asked Status
0 Verify benchmark_type / agentic_traces / derived-agentic-metrics / p90_e2e_norm_intvty against the live API; revert the new single_turn filter untouched (inferencex_client.py:231)
1 Delete comparison_metrics(), read current_best["e2e_norm_intvty_p90"] untouched
2 GPU count from state.tp; delete _recipe_scope / local_measurement.py untouched
3 Drop the identity re-checks, the fstat TOCTOU guard, the unread source_sha256 untouched (map_aiperf.py:164,176)
4 One agentx predicate: agentx_active(state) untouched (report.py:935, research_hints.py:393)
5 Report reads competitor_target.json; primary_gap stays "latency" untouched
6 Reuse perf_metric._positive / coerce.optional_positive_int; drop for_report 1 of 4 isinstance sites; helpers and flag still there
7 Collapse the ~30 reason codes into a Literal, log the failures inverted

Item 7 is the clearest sign the intent didn't land. The ask was to remove reason
codes. _AGENTX_AXIS_REASON_TEXT keeps all of them and adds a 16-entry prose
table that now has to be maintained in lockstep with the emitters — plus a
catch-all string for codes missing from it, which hides the drift. Same pattern
in the other two commits: an in_progress placeholder summary with a new
status/reason pair, and except OSError: raise wedged in front of the broad
handler. Three new layers, zero removed.

CI being green is not evidence here. The two real risks are (a) four unverified
external API symbols and (b) two parallel computations of one metric. The
fixtures encode assumption (a) and set both values of (b) equal, so no test can
fail on either.

Gate for the next push — these three, in order, before anything else:

  1. Paste one real /benchmarks agentic row and one real derived-metrics
    response into this PR, and state whether that row's tput_per_gpu is output
    or total tokens.
  2. Delete comparison_metrics() from map_aiperf.py. mapping.py:102 already
    emits the same quantity — pct(m, "e2e_output_token_throughput", "p10") is
    1/P90(E2EL/OSL), by its own comment. If eligibility genuinely differs, fix
    that one line; do not keep two.
  3. Delete _recipe_scope. GPU count is state.tp, the same source the
    synthetic path uses.

After those three, the local side is two lines, local_measurement.py is gone,
and most of items 3/6/7 disappear on their own. Production code should land
under 200 lines; it is currently 827. If any of the three looks impossible,
reply with why rather than adding a guard — that's the discussion worth having.

Remove request-record recomputation, local measurement revalidation, comparison-only provenance, and the extra analysis lifecycle. Reuse accepted metrics and serving TP, share the persisted competitor target, and retain latency/throughput advisory categories.

Constrain remaining reason codes, add contextual diagnostics, and align documentation and tests with the simplified contract.

Co-Authored-By: Claude <noreply@anthropic.com>
Comment on lines +248 to +249
'{"metrics": {"request_latency": 1000, "time_to_first_token": 10, '
'"input_sequence_length": 128, "output_sequence_length": 10}}\n',
Comment on lines +248 to +249
'{"metrics": {"request_latency": 1000, "time_to_first_token": 10, '
'"input_sequence_length": 128, "output_sequence_length": 10}}\n',
@zengleixin-amd
zengleixin-amd marked this pull request as ready for review September 14, 2026 03:08
@zengleixin-amd
zengleixin-amd requested a review from a team as a code owner September 14, 2026 03:08

@ZhengGong-amd ZhengGong-amd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@ZhengGong-amd
ZhengGong-amd merged commit f6a2296 into main Sep 14, 2026
32 checks passed
@ZhengGong-amd
ZhengGong-amd deleted the feature/leixin/agentx-compare-gpu branch September 14, 2026 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants