Repository navigation
feat(roofline): CSV interface to decouple analytical roofline producers/consumers - #1729
abdubey-amd wants to merge 15 commits into
Conversation
…rs/consumers Introduce a CSV-based interface so an external program (e.g. MAIDAS) can author the analytical roofline data Hyperloom consumes, with zero MAIDAS code in Hyperloom. Modes via new CLI flags: --roofline-csv-dir DIR read externally-authored CSVs (MAIDAS mode; HL writes none) (unset) native mode: HL producers write the CSVs and read them --no-roofline-csv stock JSON/state path (byte-identical rollback) --roofline-csv-strict fail instead of fail-soft when an expected CSV is missing - NEW orchestrator/kernel/roofline_csv.py: stdlib-only leaf module — single source of the three CSV schemas (kernel_roofline / roofline_ceiling / gpu_arch_peaks), atomic writers (ceiling upserts by arm/op), fail-soft readers with numeric coercion + bound normalization, canonical_key (roofline_match_key parity), and a RooflineResolver facade. - CLI/state gating threads the flags through bootstrap into SharedState; roofline_csv_read_dir / roofline_csv_write_dir resolver methods. - Ceiling seam (in-process): compute_roofline_breakdown_from_state becomes a thin wrapper -- MAIDAS-read via _external_ceiling_breakdown (external CSV in place of native compute) else _compute_roofline_breakdown_native; native writes the composed arms via write_ceiling_arm at both assembly sites (baseline + per-cycle). Ceiling consumers are unchanged (they read the assembled snapshot). - Tests: test_roofline_csv.py (18) + test_roofline_ceiling_csv.py (6). Implements the ceiling path of CSV_INTERFACE_REFACTOR_PLAN.md; per-kernel (M2) and arch peak-swap (M3) cross-process wiring still to come. Co-Authored-By: Claude <noreply@anthropic.com>
Key each ceiling row by the config that determines its analytical values (v1|fw|prec|act|kv|tp|ep|tep|pp|dcp|pcp|conc|isl|osl) instead of the symbolic baseline/current_best, so nothing overwrites across optimizer rounds and a consumer reads exactly its own config's row. One shared build_ceiling_key builder (HL + MAIDAS bridge), default-deny server-args guard, external-store model/gpu validation, op rows arm-scoped. 287 kernel tests green. Local WIP checkpoint: also snapshots the in-progress roofline CSV-interface producers/consumers on this branch so a stray reset --hard cannot lose them. Not pushed. Co-Authored-By: Claude <noreply@anthropic.com>
Emit an INFO line on every roofline_ceiling.csv write reporting how many rows are new config keys (history preserved) vs overwrote a same-config row (idempotent), so the content-key migration is observable in a live optimizer run and a genuine cross-config overwrite is visible. Co-Authored-By: Claude <noreply@anthropic.com>
…ntrusted/wrong-store logging - decompose the ceiling content-key into per-determinant CSV columns and drop the packed arm string on both producer and consumer sides - fix the ep/tep determinant in _ceiling_config: framework-aware EP degree (vllm tp*dp vs sglang state.ep), tep derived from ep, with a default-deny untrust backstop when ep is requested but resolves <= 1 - log UNTRUSTED vs WRONG STORE separately, with detail, at every consumer site - add test_tracelens_analysis_roofline_csv.py for tool producer/consumer coverage Co-Authored-By: Claude <noreply@anthropic.com>
…via resolver Thread an optional RooflineResolver into the diffusion memory/compute ceiling helpers so external-CSV mode sources HBM bandwidth and achievable TFLOPS from gpu_arch_peaks.csv; native/disabled falls back to the vendor HW_SPECS tables, byte-identical to prior behavior. Co-Authored-By: Claude <noreply@anthropic.com>
| import sys | ||
| from pathlib import Path | ||
|
|
||
| import pytest |
… + factory (additive) Co-Authored-By: Claude <noreply@anthropic.com>
Brings the branch up to date with main (517 commits) so it is mergeable. Conflict resolution preserves the CSV-interface feature and adopts main's refactors: - roofline_ceiling.py / roofline_snapshot.py relocated to inference_optimizer/ (PR#1650); re-homed roofline_csv.py + roofline_provider.py there too (removes the inference_optimizer->orchestrator back-edge) and updated all importers. - collectors/roofline.py deleted on main (PR#1455); ported the 9 None-preserving analytical-magnitude passthrough fields into recorder/roofline_event.py:_kernel_roofline_row. - _build_trace_analyze_cmd relocated to actions/executors/trace_analyze.py; re-applied the --roofline-csv-dir/--roofline-csv-read-dir arg-passing there. - roofline_ceiling.py content conflicts: kept my resolver= peak-swap AND main's #1602 `_resolve_achievable_tflops(...) or _resolve_peak_tflops(...)` vendor fallback. - load_roofline_results: kept the CSV round-trip (publish + overlay) over main's early-return. - shared_state ceiling round-trip kept. - Updated one test for main's #1602 (MI355X achievable table removed -> vendor fallback). Validated: 170 roofline tests pass; full inference_optimizer/tests collects (14421) with parity to main; conc_sweep passes in full-dir run (the isolated-collection circular import is a pre-existing main quirk). Co-Authored-By: Claude <noreply@anthropic.com>
Add CsvRooflineProvider (read-through over an external MAIDAS-authored CSV dir with per-value native fallback, or strict-raise mirroring --roofline-csv-strict) and a transparent CsvWritingProvider decorator. Wire make_roofline_provider to return the external provider when --roofline-csv-dir is set, else pure native. Provider is not yet consumed by any runtime choke point, so a default/native run is unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
Route compute_roofline_breakdown_from_state through make_roofline_provider(state).ceiling(...) instead of the inline external-vs-native branch. The provider's ceiling() encapsulates the identical dispatch (external arm read, native fallback, strict-raise), so the change is byte-identical while making the provider the single seam for every ceiling read. No duplicate mechanism remains: the dispatch moves into the provider rather than being copied. Co-Authored-By: Claude <noreply@anthropic.com>
The provider is a read-only seam (external CSV vs native compute); CSV persistence is owned by the existing publish_*_csv producers, so the write-decorator had no consumer and no job. Remove it and align the module docstring with the read-only design. Co-Authored-By: Claude <noreply@anthropic.com>
…eaders --roofline-csv-strict is contractually scoped to the CONSUME-R ceiling and is enforced inside _external_ceiling_breakdown. Drop the provider's own strict branch (and the now-unused strict param) so CsvRooflineProvider.arch_peak / mem_bw / model_meta fall back leniently, matching their live seams; a strict guard is added deliberately, with a test, if a site needs it. Also correct the module docstring to say only the ceiling read is wired today. Co-Authored-By: Claude <noreply@anthropic.com>
The "ceiling wired today, other methods are the read surface" note appeared twice; state it once. Docstring only, no behavior change. Co-Authored-By: Claude <noreply@anthropic.com>
Routing the ceiling choke point through provider.ceiling() had loosened its static type to Optional (the protocol default), while the returned value is never None: native compute always yields a breakdown and the external reader falls back to it. Type ceiling as -> RooflineBreakdown on the protocol and both concrete providers so compute_roofline_breakdown_from_state's non-optional annotation holds again. Type-only; no runtime change. Co-Authored-By: Claude <noreply@anthropic.com>
…ak_convention)
gpu_arch_peaks.csv matrix_*_tflops was consumed as the achievable peak
regardless of what it held; an external author (MAIDAS) supplying theoretical
MFMA peaks was silently used as achievable, inflating the compute ceiling. Add
a matrix_peak_convention column ("achievable"|"theoretical"); the native
producer tags achievable, and RooflineResolver.arch_peak returns the value only
for an achievable/untagged row -> a theoretical row returns None so the caller
falls back to its native microbench-achievable table. mem_bw is unaffected
(convention-independent). Back-compat: a legacy row with no convention column
reads as achievable.
Co-Authored-By: Claude <noreply@anthropic.com>
…-interface # Conflicts: # src/hyperloom/agents/kernel/tools/tracelens_analysis.py
|
|
||
| def arch_peak(self, device: str, dtype: str) -> float | None: | ||
| """Max-achievable matrix TFLOP/s for ``device``/``dtype`` (None on miss).""" | ||
| ... |
|
|
||
| def mem_bw(self, device: str) -> float | None: | ||
| """HBM bandwidth GB/s for ``device`` (None on miss).""" | ||
| ... |
|
|
||
| def ceiling(self, state: Any, *, arm: str | None = None) -> "RooflineBreakdown": | ||
| """Composed tok/s ceiling (mem/cmp/peak + bound) for the run's ``arm`` (native floor guaranteed).""" | ||
| ... |
| self, state: Any, model_path: "str | Path", *, precision_hint: str = "" | ||
| ) -> "ModelMeta | None": | ||
| """Model memory sizes + geometry.""" | ||
| ... |
|
|
||
| def kernel(self, name: str) -> dict | None: | ||
| """Per-kernel analytical roofline row by kernel name (None when computed inline / absent).""" | ||
| ... |
| # Fail-soft: a write failure must not raise into the optimizer loop. | ||
| try: | ||
| os.unlink(tmp_name) | ||
| except OSError: |
| import sys | ||
| from pathlib import Path | ||
|
|
||
| import pytest |
| the native compute, so this call is byte-identical to the prior inline branch while routing | ||
| every ceiling read through the single provider choke point. | ||
| """ | ||
| from .roofline_provider import make_roofline_provider |
| # Fail-soft: a write failure must not raise into the optimizer loop. | ||
| try: | ||
| os.unlink(tmp_name) | ||
| except OSError: |
| if TYPE_CHECKING: # avoid import cycles at module load; these are only type hints | ||
| from pathlib import Path | ||
|
|
||
| from .roofline_ceiling import ModelMeta, RooflineBreakdown |
|
|
||
| def arch_peak(self, device: str, dtype: str) -> float | None: | ||
| """Max-achievable matrix TFLOP/s for ``device``/``dtype`` (None on miss).""" | ||
| ... |
| return float(vendor) if vendor and vendor > 0 else None | ||
|
|
||
| def mem_bw(self, device: str) -> float | None: | ||
| from .roofline_ceiling import HW_SPECS |
| return float(bw) if isinstance(bw, (int, float)) and bw > 0 else None | ||
|
|
||
| def ceiling(self, state: Any, *, arm: str | None = None) -> "RooflineBreakdown": | ||
| from .roofline_ceiling import _compute_roofline_breakdown_native |
| self, state: Any, model_path: "str | Path", *, precision_hint: str = "" | ||
| ) -> "ModelMeta | None": | ||
| # The pure-native model meta comes from the HF dir; the CSV-aware path is CsvRooflineProvider. | ||
| from .roofline_ceiling import load_model_meta |
| def ceiling(self, state: Any, *, arm: str | None = None) -> "RooflineBreakdown": | ||
| # _external_ceiling_breakdown reads the external ceiling arm and owns the strict-raise / | ||
| # trust guards; None means "not in the external CSV" -> native fallback (always yields). | ||
| from .roofline_ceiling import _external_ceiling_breakdown |
| ) -> "ModelMeta | None": | ||
| row = self._resolver.model_meta() | ||
| if row: | ||
| from .roofline_ceiling import _model_meta_from_row |
|
@abdubey-amd this PR conflicts with current main. Could you rebase it? Files that conflict: #1787 moved the kernel tools out of There has been no activity since 2026-09-30; if it is no longer planned, say so and we'll close it. Once it merges cleanly I'll review it. |
chaojhou
left a comment
There was a problem hiding this comment.
PR #1729 -- feat(roofline): CSV interface to decouple analytical roofline producers/consumers
What it does: lets an external roofline author (MAIDAS) supply Hyperloom's analytical roofline. --roofline-csv-dir DIR makes the ceiling, arch peaks, model meta and per-kernel rows come from CSVs in DIR. A RooflineProvider sits at the ceiling choke point, falling back to native compute when DIR has nothing usable. Native runs now write the same CSVs under <session_dir>/roofline_csv/ and read them back.
The branch cannot merge as it stands. It is 746 commits behind main and conflicts in 8 files:
agents/kernel/tools/tracelens_analysis.pywas deleted on main.- The
_bypass_*files moved toorchestrator/trace_analysis/(#1787). roofline_ceiling.py,roofline_snapshot.py,shared_state.pyandcli/parser.pywere rewritten (#2077, #2042).
The rebase is a re-implementation. The issues below are in the mechanism and carry over into it.
Blocking issues: 5
-
[free:wrong-store-meta-leak] The "native fallback" for a store authored for another model still uses that store's model geometry [verified]
Problem: when_external_store_mismatchrejects the external store (model_id differs),_external_ceiling_breakdownlogs "WRONG STORE ... -> native fallback" and returns None (src/hyperloom/inference_optimizer/roofline_ceiling.py:1406-1411). The fallback reaches_compute_roofline_breakdown_native, which callsresolve_model_meta(:1671). Becauseroofline_csv_diris set,resolve_model_metareads the same rejectedmodel_roofline_meta.csvwith no model_id check (:1001-1008).ExternalRooflineProvider.model_metadoes the same (roofline_provider.py:144-148). On an extract of the head, a native ceiling of 18104.50 tok/s becomes 2958.55 tok/s once the store holds another model's meta.
Impact: the run's ceiling, and with it saturation and reloop steering, is computed from the wrong model's layer count, KV geometry and weight bytes, while the log says it fell back to native.
Action: decide the store's validity once and have every external read obey it. When the store is rejected, nothing from it (ceiling, model meta, arch peaks) may be used. -
[free:partial-meta-zero-fill] A blank cell in an external model_roofline_meta.csv becomes 0 instead of falling back [verified]
Problem: the reader returns None for blank cells, as its contract says ("blank is None, not 0", roofline_csv.py:13-16)._model_meta_from_rowthen maps every non-number to 0 or 0.0 (src/hyperloom/inference_optimizer/roofline_ceiling.py:989-991).resolve_model_metareturns that meta whenever any row exists (:1005-1008), so it never falls back toload_model_meta. Withnum_layers,num_kv_headsandhead_dimblank, the KV bytes are 0 and the reproduced ceiling is 3.2e13 tok/s.
Impact: a partially filled external meta produces an absurd ceiling rather than the native one the description promises for blank values.
Action: treat a row missing any field the ceiling needs as absent and fall back toload_model_meta, or refuse it, but never coerce to 0. Pin it with a test. -
[free:snapshot-two-peak-sources] One roofline snapshot records two ceilings from different peak sources [verified]
Problem: in external mode withgpu_arch_peaks.csvand no ceiling row, the headline ceiling uses the external peaks through_arch_peak_resolver(roofline_ceiling.py:1700, :1709, :1726).attach_perfmodel_breakdownrecomputes the PerfModel withoutresolver=(src/hyperloom/inference_optimizer/roofline_snapshot.py:211). It then records the vendor-table peak inroofline_provenance(:220). Reproduced: the headline is 9052.25 tok/s andperfmodel_breakdownis 18104.50 tok/s, with a provenance naming a source the headline did not use.
Impact: consumers of the snapshot and the breakdown export see two ceilings, and a provenance that misattributes the external number.
Action: pass the same resolver to the PerfModel inattach_perfmodel_breakdownand derive the provenance from it, so a snapshot has one peak source. -
[free:strict-swallowed]
--roofline-csv-strictnever fails the run, because the PR wraps the ceiling in newexcept Exception: passblocks [verified]
Problem: strict mode raisesFileNotFoundErrorwhen the run's ceiling row is missing (roofline_ceiling.py:1415-1421). Both production callers now catch it in a newexcept Exception: pass(src/hyperloom/orchestrator/state/shared_state.py:2014, :2357).record_baseline_roofline_ceilingthen returns{}and the run continues with no ceiling. The flag neither fails nor falls back to native, contrary to its help text (parser.py:673-677). The PR adds three more broad catches (roofline_ceiling.py:1018, :1592, :1735); the one at :1735 turns a PerfModel failure into the legacy-formula ceiling. Neither the base nor current main has any of these handlers. This breaks AGENTS.md Don't grow the debt (no new broadexcept Exception).
Impact: an operator who asks for strict gets a silently ceiling-less run, and native ceiling failures that used to surface are now hidden.
Action: remove the five broad catches and let strict raise to the run boundary. Decide whether strict also covers the untrusted and wrong-store skips (:1399-1411), and make the help text match. -
[X2] The description covers 5 of 15 commits and leaves out operator-observable changes [verified]
Problem: the body does not mention:--roofline-csv-strictor--no-roofline-csv(src/hyperloom/inference_optimizer/cli/parser.py:673-686);- the env defaults
HYPERLOOM_ROOFLINE_CSV_DIRandHYPERLOOM_ROOFLINE_CSV_READ_DIR(tracelens_analysis.py:7383, :7393); - the
RooflineProviderlayer ormatrix_peak_convention; - that every default native run now writes
<session_dir>/roofline_csv/and logs a ceiling UPSERT line on each snapshot.
It says a run without an external CSV is "unchanged". The flag help says native mode writes "under reports/" (parser.py:669), but
roofline_csv_write_dirwrites<session_dir>/roofline_csv(shared_state.py:2188).kernel_roofline.csvis advertised as an external input (parser.py:665), but no production path reads it: trace_analyze never passes--roofline-json. Three comments citeCSV_INTERFACE_REFACTOR_PLAN.md, which is not in the tree. The PR template's Observable effect, Size/complexity and Breaking changes sections are missing, and no test-plan item is checked.
Impact: operators and the release notes get an incomplete account of new flags, env vars and session artifacts. An external author supplyingkernel_roofline.csvhas it silently ignored.
Action: for the re-implementation, write the description from the template and state every flag, env var, artifact and log line. Either wirekernel_roofline.csvinto a production read or drop it from the interface. Remove the references to the missing plan document.
deferred: free:rollback-valve -- --no-roofline-csv ("Rollback valve ... for A/B comparison and safety", parser.py:680-686) exists only to switch off a native write-then-read of the same value, and the two env defaults are a second route to --roofline-csv-dir. Both are against AGENTS.md Don't grow the debt. Drop the native round-trip, the flag and the env twins in the re-implementation.
CI: ruff fails at f8784e2 (gated, so it is not reviewed here). Every other check passed at that head, but the head is 746 commits behind main, so those results do not cover the code this would merge into.
Checked: roofline_csv.py, roofline_provider.py, roofline_ceiling.py (_external_store_mismatch, _external_ceiling_breakdown, resolve_model_meta, _model_meta_from_row, publish_and_read_ceiling_arm, _compute_roofline_breakdown_native, _arch_peak_resolver), roofline_snapshot.py attach_perfmodel_breakdown, shared_state.py record_baseline_roofline_ceiling and the snapshot path, cli/parser.py and bootstrap.py, tracelens/bypass producers and trace_analyze; main's versions of the touched files and #1787/#2077/#2042 | Ran: trial merge onto main (8 conflicts); on an extract of the head, native vs wrong-store vs partial-meta ceilings, strict with a missing row through record_baseline_roofline_ceiling, external arch peaks through the ceiling and the snapshot; broad-except counts at base, main and head; the PR's roofline test files at head (168 passed); rule checklist and two independent refutation passes per finding | Base: bd5a335 | Head: f8784e2
|
@chaojhou I would wait on this PR till we internally align across teams on the direction to be followed. TraceLens has also build a custom perf model interface that should be considered to bring up a unified interface. |
|
Closing this for now, as noted in the 2026-10-22 rebase request. There has been no reply or push since then, and the branch still conflicts with main: a trial merge against ba689ef conflicts in 32 files. Main has moved a long way under it. The kernel tools left Thank you for the work on the roofline CSV interface. If you'd like to continue, please re-port it onto current main and split it into smaller PRs, for example the CSV schema and reader/writer first, then the provider and its consumers. Reopen this or link the new PRs here. The review comments above say where each piece now belongs. |
|
Reopening: closing three days after the 2026-10-22 rebase request was too soon. This stays open for a month from that request, until 2026-11-22. If there is still no rebase or reply by then, it will be closed. The notes in the comment above still apply. |
Summary
Introduces a CSV interface that decouples Hyperloom's analytical-roofline producers from its consumers, so an external author (e.g. MAIDAS) can supply the roofline data instead of Hyperloom computing it natively. Backward-compatible: absent/blank CSVs fall back to native computation, so a run with no external CSV is unchanged.
5 commits on top of
main:roofline_ceiling.csvby config key — rows keyed by the config that determines them (fw/prec/act/kv/tp/ep/tep/pp/dcp/pcp/conc/isl/osl) rather than symbolic baseline/current_best.gpu_arch_peaks.csvvia the arch-peak resolver.Motivation
Lets MAIDAS (or any external roofline authority) feed Hyperloom's roofline CSVs (arch peaks, per-kernel, ceiling) at a stable interface, rather than Hyperloom recomputing them — the external ceiling is consumed in both the
--no-kerneland kernel paths via the existing saturation/reloop steering.Test plan
--roofline-csv-dirat a MAIDAS-authored dir -> assert the external ceiling/peaks are what the ceiling + snapshot actually use (CONSUME R / CONSUME M).🤖 Generated with Claude Code