Remove Trace Analysis Routes that Replicates TraceLens Features - #1368
Conversation
|
@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. |
CI E2E report — ❌ Timeout
|
|
|
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
left a comment
There was a problem hiding this comment.
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
profilestep [...]
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 keeps — test_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) == 0That 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_handlerspicks 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:
- Has a
custom-framework trace been run end to end through theagentroute? On what corpus? - For a model TraceLens does not support, what is the degraded behaviour after this PR — a graceful skip, or no candidates at all?
- What picks up the end-to-end coverage that
test_the_bypass_route_emits_the_sidecar_for_customprovided?
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:
bypassalready answers the same question — hot kernels with no LLM call — by reading the profiler trace directly. [...] Onlyagentandbypassremain.
A request still naming
analysis_route=deterministicfalls back toagentand carries a warning that namesbypass. 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.
|
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. |
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:
(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.
to trace analysis, plus route-selection branching threaded through the
dispatcher, CLI, env config, and docs.
Architecture
Before — three routes, runtime selection
After — main path
Verification
leftover route branches, flags, env vars, or conflict markers.
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).
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.)
integrity, and the completeness of the removal were each reviewed and signed
off.
Risk & rollout
subsumed by the primary path, with a graceful-skip failure mode rather than a
hard error even in the un-sampled edge case.
longer exist. Any caller still setting them should drop them; they now do
nothing (removed from docs and templates in this PR).
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