Skip to content

Remove Trace Analysis Routes that Replicates TraceLens Features - #1368

Merged
xiaofei-zheng merged 9 commits into
mainfrom
feat/tsrikris/hyperloom_tracelens_cleanup
Sep 9, 2026
Merged

Remove Trace Analysis Routes that Replicates TraceLens Features#1368
xiaofei-zheng merged 9 commits into
mainfrom
feat/tsrikris/hyperloom_tracelens_cleanup

Conversation

@tsrikris

@tsrikris tsrikris commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

The kernel agent previously carried three ways to turn a profile trace into
kernel candidates: the primary TraceLens-based path, plus two legacy
alternatives (a raw-trace "bypass" reader and a no-LLM "deterministic" route)
selected at runtime by an environment variable / CLI flag. This PR augments the deterministic path with in-built fallback handlers.


Why

The two legacy routes existed as fallbacks from an earlier design. Over time the
primary path absorbed everything they did:

  • Duplication and drift. Each route carried its own copy of shared rules
    (trace reading, source resolution, capture-sidecar classification, category
    mapping). The copies drifted independently — a class of latent bug where one
    route learned a fix the others did not.
  • Maintenance cost. Three code paths meant 3× the surface for every change
    to trace analysis, plus route-selection branching threaded through the
    dispatcher, CLI, env config, and docs.

Architecture

Before — three routes, runtime selection

                         profile trace (*.pt.trace.json.gz)
                                      │
                          ┌───────────┴───────────┐
                          │   route selector       │   env var / CLI flag
                          │  (ANALYSIS_ROUTE_*)     │
                          └─┬─────────┬─────────┬───┘
                            │         │         │
                   ┌────────▼──┐ ┌────▼─────┐ ┌─▼──────────────┐
                   │ PRIMARY   │ │ BYPASS   │ │ DETERMINISTIC  │
                   │ TraceLens │ │ raw-trace│ │ no-LLM route   │
                   │ pipeline  │ │ reader   │ │                │
                   └────┬──────┘ └────┬─────┘ └─┬──────────────┘
                        │             │         │
                        │      shared helpers (duplicated per route):
                        │      trace reader · source resolver ·
                        │      capture-shape classifier · category map
                        │             │         │
                        └─────────────┴────┬────┘
                                           ▼
                              kernel candidates + analysis.md

After — main path

                    profile trace (*.pt.trace.json.gz)
                                  │
                                  ▼
                    ┌─────────────────────────────┐
                    │   PRIMARY TraceLens path      │
                    │   (unconditional dispatch)    │
                    └──────────────┬───────────────┘
                                   │  imports shared helpers from
                                   │  single-owner modules:
                                   │   • trace reader
                                   │   • source index  (is_editable_source,
                                   │                     triton_def_line)
                                   │   • skill runner  (report parse/render)
                                   │   • capture-shape classifier
                                   ▼
                       kernel candidates + analysis.md

Verification

  • Lint: clean on all touched modules.
  • Deletion completeness: no surviving module imports any removed module; no
    leftover route branches, flags, env vars, or conflict markers.
  • Re-homed feature: exercised directly — the Triton def-line resolver
    returns correct results in all modes (exact match, strict name-match rejects
    unrelated symbols, single-def fallback), and the surviving path resolves it as
    a live callable (not silently disabled).
  • Focused test run: the suite subset that exercises the changed modules
    passes. (The only failures observed were an unrelated environment gap — a
    missing optional dependency in a feature this PR does not touch — reproducible
    on the base branch.)
  • Independent design review: the plan, the implementation, the primary-path
    integrity, and the completeness of the removal were each reviewed and signed
    off.

Risk & rollout

  • Capability risk: low. The one unique guard removed is empirically
    subsumed by the primary path, with a graceful-skip failure mode rather than a
    hard error even in the un-sampled edge case.
  • Behavioral change for callers: the analysis-route env var / CLI flag no
    longer exist. Any caller still setting them should drop them; they now do
    nothing (removed from docs and templates in this PR).
  • Cross-component dependency: the primary path relies on the upstream trace
    analyzer emitting its fallback report without the idle row. That contract is
    held by the pinned analyzer version.

Deprecation: xDiT diffusion roofline path slated for removal

The following three modules under src/hyperloom/agents/kernel/tools/ are marked for removal as the xDiT diffusion roofline path is incorrect and planned to be removed in a follow up PR

  • diffusion_roofline.py — workload-level roofline aggregation for diffusion/scriptable traces
  • _denoise_steps.py — per-denoise-step divisor helpers
  • diffusion_flops.py — per-architecture analytic FLOP / compute-ceiling estimator for xDiT models

@tsrikris tsrikris changed the title Remove legacy trace-analysis routes; consolidate on the primary path Remove Trace Analysis Routes that Replicates TraceLens Features Sep 1, 2026
@tsrikris

tsrikris commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

@xiaofei-zheng @chaojhou Note that this PR clears out redundant data paths in the TraceLens component of Hyperloom with the fallback path built into the agent: AMD-AGI/TraceLens#970. Pls take a look, we are planning to un-draft this tomorrow.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

CI E2E report — ❌ Timeout

item value
result ❌ Timeout
model Qwen/Qwen3-0.6B (dense)
resources 1× GPU, TP=1
PR branch feat/tsrikris/hyperloom_tracelens_cleanup
commit 901bb4d0ca8306ef61a1b94eded9fa78090baea6
session_id 7ec1503c-f893-4a95-9b24-f49069f06274
queue → dispatch -81236s
run time 1379m 37s
total 25m 41s
reason Timed out — the run never reached a terminal state in time (task stuck, or the GPU stayed queued too long).
detail not terminal after 13200s

details

@tsrikris

tsrikris commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author
  1. 6fac696 — Cleanup Pt 4 (merge of main + branch cleanup)
    Synced main into the branch and finished the route removal at the edges: CHANGELOG'd the breaking removal of the bypass/deterministic routes, stripped their sections from docs/reference/environment-variables.md and
    .env.template, and cleaned trailing references across docs and framework files.

  2. f5b4d76 — Synch Main 9/3 (merge of 77 main commits)
    Resolved the merge cleanly while preserving main's new features on top of our deletion:

  • 3 bypass modify/delete conflicts kept as deletions; 1 content conflict in _build_trace_analyze_cmd; 1 latent NameError where main's new analysis_meta block referenced route-branching we'd deleted (fixed to constants,
    preserving main's new SBD-V6 roofline feature).
  • Reconciled a live main feature — trace_selfcert.py — that imported our deleted _bypass_trace_reader. Restored just its analysis surface as a new deduped module _trace_analysis_reader.py (I/O primitives kept in the single
    _trace_reader.py owner), repointed selfcert, restored its 53-test suite.
  • Ported the one useful behavior from the parallel chore/shafeng/drop-deterministic branch: a route-safety guard that hard-fails an explicit non-agent route (invalid_analysis_route) instead of silently charging an LLM.
  1. 0358d9b — Reconcile with main 9/3 (post-merge cleanup — 10 files, +53 / −230)
    Two review passes after the merge:
  • Dead-code collapse: removed the unreachable tool == "bypass" branch and its now-single-valued param in _analysis_steady_state; hardcoded the constant route/tool = "tracelens" in _build_analysis_meta (export envelope keys
    unchanged). Reworded "this branch" docstrings to durable prose; updated the SBD-V6 roofline test.
  • Residue sweep: tracelens_route_hint "bypass" → "roofline" (observability-only field); de-referenced the dead route in 3 docstrings; deleted 2 orphaned files (_kernel_category.py + its test — only importer was itself; live
    logic is the separate derive_kernel_category).

@tsrikris
tsrikris marked this pull request as ready for review September 3, 2026 19:28
@tsrikris

tsrikris commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Repo updated with latest TraceLens version that supports in-built fallback handling: AMD-AGI/TraceLens#1004

@ZhengGong-amd @xiaofei-zheng @fengshaoyi-amd Pls take a look, reconcile any gaps and merge during your timezone. This PR aims to start cleaning up the unnecessary bloat around TraceLens in Hyperloom

CC: @chaojhou

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

Blocking: removing the bypass route drops the only TraceLens-free analysis path, and custom framework depends on it.

The deterministic removal I have no objection to. The bypass removal I think is not safe, and separately it collides with #1397, which is removing deterministic in parallel.


1. bypass is the only path that does not depend on TraceLens parsing the trace

bypass_trace_analysis.py's own module docstring:

Independent (TraceLens-free) trace analysis backend for the bypass route. [...] It replaces the TraceLens agent / TraceLens deterministic scripts entirely: it never imports or shells out to TraceLens. It reads the torch-profiler Kineto trace produced by the profile step [...]

It reads raw Kineto directly and is framework-agnostic. The agent route is not: it depends on TraceLens being able to produce perf CSVs from the trace first.

The clearest evidence for that is in a test this PR keepstest_custom_no_diffusion_ceiling.py:

def _stub_trace_derived_report(monkeypatch):
    """Stand in for the TraceLens aggregation, which needs real perf CSVs."""

So custom on the TraceLens route has only ever been exercised with the TraceLens aggregation stubbed out, because it cannot be driven end to end in a unit test.

2. custom is exactly the framework where that assumption is least safe

From framework_registry.py:

# is deliberately neutral: only the operator's own report knows whether the
# number it produced counts frames, images or anything else.
"custom": FrameworkSpec(name="custom", kind=SCRIPTABLE, repo_url=None, ...)

custom runs an operator-supplied entrypoint against an operator-supplied model. Hyperloom knows nothing about the workload ahead of the run — by design. In practice we have models on this path that TraceLens does not support (hy model being the concrete one). For those, bypass is not a legacy fallback, it is the only route that produces candidates at all.

3. The end-to-end coverage for custom is deleted, not migrated

This PR removes:

def test_the_bypass_route_emits_the_sidecar_for_custom(self, tmp_path, capsys, monkeypatch):
    """Trace-derived on that route too, so a name check would wrongly skip it."""
    ...
    assert bta.main(argv) == 0

That was a real end-to-end run. The surviving custom tests all stub the TraceLens aggregation. After this PR, custom has no end-to-end coverage on any route.

Worth re-reading the docstring of the test class that gets deleted with it:

TestTheTwoRoutesAgree: bypass and TraceLens are two spellings of one feature (request_handlers picks between them), so a framework must not be scriptable on one and not the other

The design intent was that the two routes are interchangeable. Removing one upgrades that into "TraceLens is sufficient for every framework," which is the claim that needs evidence.

4. The empirical evidence in the description does not cover this

Verified empirically against a corpus of real graph-collapsed traces: the primary path flags every case the old guard would have

That corpus validates the idle guard specifically. It does not say anything about whether TraceLens can parse traces from arbitrary operator-supplied workloads. Those are two different questions, and the guard result is being used to support a conclusion about the second.

Concretely, could you share:

  1. Has a custom-framework trace been run end to end through the agent route? On what corpus?
  2. For a model TraceLens does not support, what is the degraded behaviour after this PR — a graceful skip, or no candidates at all?
  3. What picks up the end-to-end coverage that test_the_bypass_route_emits_the_sidecar_for_custom provided?

5. Overlap with #1397

#1397 (Drop the deterministic trace-analysis route) touches 19 files; 18 of them are also touched here. Biggest overlaps: tracelens_analysis.py (+13/−953 there vs −1072 here), test_tracelens_csv.py (+34/−775 vs −965), request_handlers.py (+36/−52 vs −183). CHANGELOG and .env.template conflict on both sides.

More importantly the two PRs disagree on bypass. #1397 is pure modification, zero deletions, and keeps bypass deliberately:

bypass already answers the same question — hot kernels with no LLM call — by reading the profiler trace directly. [...] Only agent and bypass remain.

A request still naming analysis_route=deterministic falls back to agent and carries a warning that names bypass. Falling back silently would spend an LLM session — the opposite of what a caller asking for a no-LLM route wants.

If #1397 lands first and this PR lands after, that warning points at a route that no longer exists, and the exact scenario #1397 set out to prevent (a caller who asked for no-LLM being forced into an LLM session) becomes the only possible outcome. If this PR lands first, #1397's premise disappears and 18 of its 19 files are wasted.

There is also a tighter coupling worth noting: #1397 states "_analysis_md.py is kept; bypass is now its only caller" — this PR deletes _analysis_md.py. And both PRs rewrite the record_kernel_discovery call contract (#1397 drops tool= so it defaults to source; this PR changes the same call site). That is an easy one to resolve wrongly in a merge.

Suggestion

Let #1397 land first — it is smaller, uncontroversial, and already scoped to exactly the part everyone agrees on. Then rebase this PR on top with the bypass removal dropped.

That leaves this PR as the helper re-homing (is_editable_source, triton_def_line into the source index, report parse/render into the skill runner, trace-reading primitives into the trace reader) plus the TraceLens SHA bump. Those are good de-duplication and stand on their own merits regardless of the routing question — but they should probably be their own PR rather than riding along with a route removal.

If you do want to make the case for removing bypass later, it would be much easier to review as a standalone PR with the custom-framework evidence attached, rather than inside a 12k-line deletion.


I have a set of line-level findings on the current diff as well (one of them: the new _trace_reader.stream_events is byte-identical to the pre-c6cbccbff implementation, so it reverts that performance fix — measured ~5x slower on a 32MB trace, 0.28s to 1.46s, and _trace_analysis_reader.py now imports it). Holding those until the rebase question is settled, since a good chunk of them land in code that would be rewritten anyway.

@tsrikris

tsrikris commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

The TraceLens Deterministic fallback aims to handle any scenario where a bad trace falls through despite all the efforts in trace splitting, etc. The PR was shared along with the design doc previously: AMD-AGI/TraceLens#970. The flow is designed to fall through to a purely kernel name and time based prioritization (no shapes, etc. available) in case the trace quality is lacking; and Hyperloom will be able to read out the missing entries.

@xiaofei-zheng
xiaofei-zheng merged commit 8729917 into main Sep 9, 2026
32 of 34 checks passed
@xiaofei-zheng
xiaofei-zheng deleted the feat/tsrikris/hyperloom_tracelens_cleanup branch September 9, 2026 07:12
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.

2 participants