Fix/kernel op identity - #425
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Prefer main behavior where overlapping workflow logic has diverged.
… depth Verifying the discovery contract against the 88-run archive turned up six ways it could be bypassed or misled. The selection contract trusted candidate.depth, an integer supplied by the same agent whose choice is under review. An extractor could append a candidate of its own carrying depth:999, nominate it, and the gate returned ok even though the architect had declared a genuinely deeper launcher. kernel_selection.py already derives deeper_live_candidates from the profiler's host-span nesting; the gate now reads that. The declared chain stays as a second, independent way to fail -- it can add a rejection but can never buy a pass. A depth that is absent or not a finite number now removes the candidate instead of ranking it as zero, where every NaN comparison was silently false. seam_trace could not resolve a dotted attr, so Class.method seams dropped out of the probe -- and an omitted candidate is exactly what the coverage check exists to catch. Marker installs also had to move after the capture hooks: importing a target module during a marker install aliases the un-captured function, so the oracle and the probed object were not the same callable. The profiler re-emits each marker on the device timeline, where an outer seam's short span routinely lands inside an inner one's; reading those projections inverted the call nesting and refused the correct launcher. A gpu_kernel head naming no device symbol at all reached extraction with the contract switched off: requiredDeviceKernel returned '', which the gate reports as "not a profiled GPU-head extraction". The vacuous pass is right for a non-kernel head, so admission is what stops it now. Finally, a bare `mismatch` in the correctness regex was tested first and swallowed signature_mismatch, sending a seam defect to the corrective that hunts data_ptr over-fits rather than the one that matches the live dispatch signature. Co-authored-by: Cursor <cursoragent@cursor.com>
…e installed but never observed
main's baseline redesign wins where the two branches disagree: the kernel
track's denominator is now the frozen baseline_overlay/ environment plus the
one declared candidate_bind, not a baseline_callable, and baseline_callable
survives only on the op track. The selection contract (seam_candidates,
selection_validation, the machine verdict gate) rides on top of that unchanged.
Two things the textual merge could not see:
- overlay_setup's add-marker never got main's --from, so a probe-only overlay
would have been seeded from the pristine install rather than the live stack.
- the branch's two new test files were never registered in the L0 job.
Co-authored-by: Cursor <cursoragent@cursor.com>
No conflicts: main's work this round (learned-KB cards and their lint/index gates, the magpie launcher's recipe-env handling, and the lane-level use_learned_kb default) lands in regions the selection contract does not touch. Checked rather than assumed: every kernel-lane invocation still routes through laneArgs, so main's new lane-defaults contract holds with the selection changes present; and the capture flow's EXTRA_ENV/OVERLAY_PYTHONPATH still reach the server on both the native and the rewritten magpie path. Co-authored-by: Cursor <cursoragent@cursor.com>
Both red L0 checks came from this branch's own new code.
Coverage sat at 96.78% against a 97% gate: kernel_selection.py, seam_trace.py
and the parse_profile.py additions landed with 82 uncovered statements between
them, and --annotate -- the entry point that turns a Top-N doc into routable
head candidates, exit code included -- had no CLI test at all. All three are now
at 98-100% and the total is 98.43%.
CodeQL flagged canonicalDeviceKernel's single greedy pass over `<.*>`. It really
is lossy: the pass spans from the first '<' to the last '>', so `k<a>(t<b>)`
loses the '(' that ends the name, and a nested `k<pair<a,b>>` leaves a delimiter
behind. Balanced groups are now removed innermost-first until the symbol stops
changing. canonical_kernel_name in kernel_selection.py is the same function on
the Python side and had the same defect, so both move together -- a drift there
would let a kernel pass one side of the contract and be refused by the other.
Co-authored-by: Cursor <cursoragent@cursor.com>
iraj465
left a comment
There was a problem hiding this comment.
Reviewed at c040738 (15 files, +2616/−37 vs main @ c0433ba). The 319 tests
in the five suites pass locally. The design is right: replacing a static
no_rebind_seam guess with installed-marker + External-id/correlation evidence
is the correct shape of fix for #405, and installed_but_never_live_candidates
being reported rather than read as "not deeper" is the honest call.
One blocking finding. canonical_kernel_name is wrong on real ROCm profile
names, and the tests do not catch it because every canonicalization case in
test_kernel_selection.py is namespaced (void vllm::..., void ns::...),
where the :: split rescues it.
Two mechanical causes, both in the same three lines:
text.split("(", 1)[0]truncates at the first(. For
void (anonymous namespace)::clamp_position_kernel<long>(...)that is the(
of(anonymous namespace), not the argument list, so the token becomesvoid.re.sub(r"[^a-z0-9_]+", "", ...)deletes separators instead of splitting on
them, so a namespace-freevoid wvSplitKrc_<...>becomesvoidwvsplitkrc_.
(?:^|_)then cannot anchor, and the head never matches its own kernel.
Measured over every profile_topN.json in /shared_nfs/hyperloom-claw/
(127 profiles, 3003 head rows, 679 distinct kernel names) — for each row I
compared the identity the gate would use (short_name, via
requiredDeviceKernel) against that row's own trace name:
| current | reusing parse_profile.short_name |
|
|---|---|---|
| head fails to match its own kernel | 1486 / 3003 (49.5%) | 120 / 3003 (4.0%) |
distinct degenerate tokens ("" or <6 chars) |
8 | 7 |
| kernels sharing a degenerate token | 23 | 19 |
Both directions are wrong, and the false positive is the worse one:
- False negative.
paged_attention_ll4mi_QKV_mfma4_kernelvs
void paged_attention_ll4mi_QKV_mfma4_kernel<__hip_bfloat16, ...>→
voidpaged_attention_ll4mi_qkv_mfma4_kernel→ no match →
device_kernel_not_under_target→ afterBASELINE_EXTRACT_RETRIESthe new
code setssmoke:'fail',selection_failed:true. A correct seam is killed.
31 distinct names havevoidfused on this way; 54 of 127 profiles carry at
least one degenerate row. - False positive. 9 distinct kernels all reduce to the token
void, and
kernel_matchesreturnswant == gotbefore thelen(want) >= 6guard
applies. Soclamp_position_kernelandelementwise_kernel_with_index
mutually certify. That is the gate grantingmatched_kernel_calls > 0and
ok: truefor a kernel the seam did not launch — the exact credit this PR
exists to withhold.
Suggested fix, in the repo's own idiom rather than a new special case:
parse_profile.short_name() already strips ^void\s+, takes [\w:]+ (which
stops at both ( and <), and drops the namespace — and norm_key() already
folds on top of it. Building canonical_kernel_name on short_name gets the
table above, deletes _strip_template_args entirely, and leaves one
canonicalizer in the repo instead of two. Two additions still needed:
strip a leading (anonymous namespace):: (most of the residual 120), and move
the length guard so it also covers the equality branch.
Second, smaller: the JS and Python matchers are not the pair the comment
claims. _strip_template_args says it "must stay identical to
canonicalDeviceKernel in e2e_workflow.js". The canonicalizers do agree, but the
matchers built on them do not — kernelIdentitiesMatch tests containment in
both directions, kernel_matches only want in got:
(x.length >= 6 && rx(x).test(y)) || (y.length >= 6 && rx(y).test(x)) // JSlen(want) >= 6 and re.search(..., got) # PYThe asymmetry may well be deliberate (the JS compares two declared symbols, the
Python compares a declared symbol to trace events, where the reverse direction
would be unsafe). If so, please scope the comment to the canonicalization only —
as written it tells the next maintainer the two are interchangeable. A shared
fixture of real symbol pairs, asserted on both sides, would pin whichever
contract you intend.
Three minor points, none blocking:
seam_trace._trace_path: withGEAK_SELECTION_TRACE_UNIQUE=0it returns
templateun-formatted, so a template containing{pid}/{rank}yields a
file literally named...{pid}.... It also drops the pid/rank suffix that
kernel_selection.mainmatches on (\.pid-N(?:\.|$)), so every rank races one
path and the per-process pairing fails tocapture_process_trace_missing.
Returning the formattedpaththere is the one-line version.kernel_selection.mainpicks amax(attempts, ...)for
selected_meta_path, then overwrites nearly every field with an aggregate
across all attempts and reassignsselected_pathsto all paths (L409). The
emittedcapture_meta_filetherefore names one process while the counts next
to it are sums. Either drop themaxor label the field
best_process_meta_file.capture_shapes._wrappernow emitsGEAK_TARGET::<target>as well as
seam_trace.install. When the capture target is also a marker candidate the
same span name nests inside itself, sotarget_marker_callsdouble-counts.
Only compared against> 0today, so it is a reporting nit.
_start_profile has one narrow leak: _PROFILE.update(active=True, ...) runs
before _record_install_markers(), so if that raises, the except sets
done=True while leaving active=True and the profiler entered —
_finish_profile then returns early and never exits it. Moving the update after
the marker write closes it.
Happy to re-review once the canonicalizer lands; the rest of the contract reads
well and I could not fault the nesting/causality logic.
Note: no
noderuntime is available in my environment, so the JS above is
reviewed by reading only; every Python number in this comment came from
executing the branch's own code over the live campaign.
|
Thanks for the detailed review and corpus measurement. I reproduced the reported failure mode and addressed all blocking, secondary, and minor findings. What changedKernel identity canonicalizationThe Python and JavaScript canonicalizers now follow the same contract:
I did not canonicalize solely through Python/JavaScript parityA shared fixture now contains 15 symbols taken verbatim from ROCm captures, including anonymous namespaces, namespace-free symbols, elided templates, pipeline annotations, and memcpy rows. It is asserted by:
The JavaScript test extracts the implementation directly from Other review findings
Corpus validationI repeated the review methodology over every current
All 161 residual rows contain a historical Re-deriving The template-aware matcher also reduces the distinct-symbol collision surface: 158 accepted distinct-symbol pairs versus 942 for a For cross-language parity, I compared Python and JavaScript over:
All review findings are now addressed. Please re-review when convenient. |
iraj465
left a comment
There was a problem hiding this comment.
Approving. The blocking canonicalization defect is fixed, and I re-measured it against the live campaign rather than taking the tests' word for it.
A/B on /shared_nfs/hyperloom-claw/ — 3202 profile rows, 20260820
| metric | c040738 (reviewed) |
2253f32 (head) |
|---|---|---|
| head fails to match its own kernel | 1486 / 3003 (49.5%) | 0 / 3202 (0.0%) |
'void' collision bucket |
9 kernels | 0 |
| degenerate tokens | 8 | 0 |
M(clamp_position_kernel, elementwise_kernel_with_index) |
True (false positive) |
False |
| the three cited false negatives | all False |
all True |
The 0% is the number that matters, and it is only 0% because the fix landed on both halves. Measuring the new matcher against the stored short_name still leaves 172 residuals (5.4%) — but every one of those is an artifact the old parse_profile.short_name produced: 'StoreKVCacheParams)', 'FastTopKParams, int*, int const*, long)', 'result_type*)', i.e. the trailing parameter type where the kernel name should be. The (anonymous namespace) strip in parse_profile.py fixes them at the producer. Replaying producer + matcher together, the residual is exactly zero.
What I checked
- Full suite:
1076 passed, 2 failed, 4 skipped, 66 subtests. Both failures are intest_bench_e2e_teardown_lookup.py, which this PR does not touch; I checked outorigin/mainand ran that file alone —2 failed, 2 passed. Pre-existing, out of scope. - Parity fixture — this answers my second finding.
kernel_symbols.jsoncarries 20 canonical symbols (15 lifted verbatim from ROCm captures) and 17 match verdicts, 10 of them negative, so it pins the false-positive direction too. The Python half (TestTheSharedFixtureHoldsOnThisSide) passes: 3 tests / 37 subtests.ci-l0-checks.ymlwires the node half into the existingnode-regressionjob. - The parity test extracts the block from
e2e_workflow.jsby source offset rather than reimplementing it. That is the right call — a copy would pass while the shipped code drifted. - All four minor points addressed:
_trace_pathunderGEAK_SELECTION_TRACE_UNIQUE=0keeps the per-process suffix;_start_profilenow closes a profiler that fails after__enter__;_record_install_markers()has a single call site; no vestigial attempts clamp remains. _template_argumentsdiscrimination for the 20-wayat::native::vectorized_elementwise_kernelcollision was not something I asked for and is a real improvement — the base token alone cannot separate those.
Caveats on this review, stated plainly
- I could not execute the JS. There is no
node,deno,bun, orqjsin this environment.canonicalDeviceKernelandkernelIdentitiesMatchwere reviewed by reading only. I walked them step-for-step againstcanonical_kernel_name/kernel_matchesand they agree in order and in every transform. CI'snode-regressionjob is what actually proves it. - Non-blocking, for a follow-up: Python's
[\w:]+is Unicode-aware and JS's is ASCII-only, so a non-ASCII identifier character would canonicalize differently. No ROCm symbol will hit this, and the fixture cannot catch it. Worth one ASCII-only probe in the Python regex if you touch this again.
Neither caveat blocks the merge.
Verify kernel selection against the running server instead of trusting the extraction
Refs #405
Problem
A run picks a GPU kernel out of a profile, then binds an authored replacement at a Python
callable in the live server. Those are two different identities and the orchestrator only ever
checked one. A profile row names a device symbol; a deployable seam is a
module:attrthatactually launches it. Nothing checked that the selected callable was on the live path at all,
let alone that it was the innermost one that launches the profiled kernel.
That is how the 20260802 attention run banked a 4.47x isolated speedup that moved the server 0%:
the selected callable was an outer wrapper and the kernel it "replaced" was launched further
down the chain. No static gate can catch this. Of the 210 archived extractions in the 88-run
corpus, 188 carry a syntactically perfect
module:attrtarget — the run that worked (20260720)and the run that did not (20260802) are indistinguishable on paper. They differ only in what
actually executes.
What this changes
Selection now has to produce runtime evidence, and the gate fails closed without it.
scripts/parse_profile.py— every Top-N row declares what it is (entity_kind:gpu_kernel/memory_op/dispatcher_op/python_launcher/unresolved) together withthe evidence the decision rests on. Rows confirmed to be dispatcher aggregates are expanded
into the device kernels they launched, using External-id edges, so an aggregate can no longer
hide the kernel underneath it.
scripts/seam_trace.py(new) — installs profiler markers on the declared seam candidates,including dotted class-method specs such as
Runner.run. Only pure Python callables arewrapped; JIT/native objects exposing
fn/cache/warmup/run/__torch_dispatch__are refused rather than corrupted. Traces are exported per PID/rank.
scripts/kernel_selection.py(new) — reads the trace and returns a machine verdict: did theselected callable run, did the profiled kernel launch inside its span, and is there a live
candidate nested deeper. Launch causality is established from External-id and launch-correlation
edges on the marker's own thread inside the marker span, never from timestamp overlap.
e2e_workflow.js—kernelSelectionVerifiedgates completion on that verdict. "Deepest" issettled by the profiler's observed nesting; a self-reported
depthcan add a rejection but cannever buy a pass. A head naming no device symbol is refused at admission. When retries are
exhausted the run is forced to
smoke:'fail'withselection_failed:true.Evidence
1. Production end-to-end run: the descent happens, on a live server
Full
--mode e2erun ofQwen3.5-122B-A10B-FP8on SGLang (BACKEND=sglang TP=1 GPU=0),driven by the orchestrator rather than by hand.
seam_trace.pyin that run is byte-identical tothis branch. The run produced machine verdicts for two different operator families.
MoE — the seam actually descended off the wrapper it used to sit on. Earlier attempts on this
head targeted the
triton_utils.fused_moealias. This run marked all six declared candidates,including the class-method candidate
TritonRunnerCore.run, and the verdict resolved to thedirect
fused_moe_kernel[grid]launcher — a target that appears in none of the previouslyattempted callables:
target_callablesglang.kernels.ops.moe.fused_moe_triton_kernels:invoke_fused_moe_kerneldevice_kernelfused_moe_kernelok/deepest_verifiedtarget_marker_callsmatched_kernel_callscorrelated_external_idscandidate_targets_testeddeeper_live_candidates/missing_candidate_markers[]/[]Four of the six declared candidates were live and the verdict still landed on the innermost one,
with nothing observed nested below it. That is the 0802 failure mode resolved in the opposite
direction, in production, on an operator family unrelated to attention.
GEMM — three seam specifications, three certified verdicts. For the CK kernel
kernel_gemm_xdl_cshuffle_v3_multi_d_blockscale_b_preshuffle:ok/deepest_verifiedaiter.ops.gemm_op_a8w8:gemm_a8w8_blockscale_bpreshufflesglang...fp8_utils:gemm_a8w8_blockscale_bpreshuffleaiter.ops.gemm_op_a8w8:gemm_a8w8_blockscale_bpreshuffle_ckEntity classification ran on the production profile.
profile_topN.jsonfrom that run carriesentity_kind_contract: v1with all 25 of 25 Top-N rows resolved togpu_kernel, over 13,922kernel launches and 72 distinct kernels — the head track is fed by rows that declared what they
are, not by rows assumed to be kernels.
2. Controlled hardware validation: the wrapper is refused, the launcher is certified
A second, deliberately adversarial check on one MI355X,
Qwen3.5-122B-A10B-FP8served by vLLMv0.24.0 (V1 engine, in-process).
head_dim=256forces the Triton paged-attention path — the exactcall chain the 0802 run mis-selected. 60 traces over prefill and decode; capture meta records the
target called 36 times across 3 cases covering both regimes.
rocm_attn:chunked_prefill_paged_decode, kernelkernel_paged_attention_2d(decode)ok:true,deepest_verified:true, 72 live marker spans, 36 matched launches, 447 External-id / 714 launch-correlation edges, no deeper live candidate_fwd_kernel(prefill)ok:true,deepest_verified:true, 12 matched launchesattention:unified_attention_with_outputok:false,failed=[capture_target_mismatch, device_kernel_not_under_target, target_marker_missing], 0 matched launchesThe selection that produced the 4.47x paper win cannot reach extraction. The launcher that
actually issues the kernel is certified, backed by 48 individual launch edges.
Attribution does not depend on a single correlation channel. Kernel rows in that trace divide
cleanly: all 73
at::native::*rows carried both anExternal idand acorrelation, while all48 Triton rows — 36
kernel_paged_attention_2dplus 12_fwd_kernel— carried acorrelationand no
External id. Binding attribution to either edge covers both shapes. The correlation edgeis held to the same strictness as the External-id one: the host launch event must sit on the
marker's own thread, inside the marker span. An edge falling outside the span is refused, and
merged multi-process traces prefix
correlationso ids from different ranks cannot collide. Bothproperties are pinned by
test_launch_correlation_outside_the_marker_span_is_not_enoughandtest_correlation_ids_do_not_collide_across_merged_call_traces.3. Regression suites
python3 -m pytest -q e2e_workflow/scripts/tests— 890 passed, 1 skipped on a clean tree.test_kernel_selection.pyalone carries 19 cases pinning each rejection branch, including:test_0720_live_inner_launcher_is_selection_success— the live inner launcher passes.test_0802_outer_wrapper_fails_when_0720_launcher_is_marked— the wrapper is refused when adeeper live candidate is observed.
test_triton_kernel_without_external_id_is_linked_by_launch_correlation— a device row carryingonly
correlationis still attributed to the seam that launched it.test_the_observed_nesting_is_what_settles_deepest_not_the_declared_depth— a candidateappended with
depth:999cannot outrank what the profiler observed.test_device_projected_annotation_does_not_invert_call_nesting— the profiler re-emits everymarker on the device timeline, where an outer seam's short span lands inside an inner one's;
only host spans establish nesting, and the correct launcher still passes.
test_every_declared_probe_candidate_must_have_an_installed_markerandtest_a_probed_candidate_that_never_fired_is_reported_not_silently_dropped— the candidate sethas to be complete, and "never probed" is reported apart from "probed and never observed".
test_cli_requires_deepest_selection_on_every_capture_process— one TP rank disagreeing sinksthe verdict.
4. Replay against the 88-Hyperloom-run archive
An out-of-repo harness runs the real predicates —
e2e_workflow.jssource slices executedthrough QuickJS, and the shipped Python scripts driven as subprocesses — over 210 archived
meta.jsonfrom 88 Hyperloom production runs. 260 passed / 1130 subtests. On that real archived data:kernelSelectionVerifiedpassing on archived evidencemodule:attr) targets refusedprepareHeadSelectionEntity classification was run with the shipped
parse_profile.py --annotateover every archivedround whose torch trace survived:
dispatcher_opresolves to 0, because confirmed dispatcheraggregates are replaced by the device kernels they launched — precisely the descent step the 0802
head needed and the old behaviour discarded.