Skip to content

feat(kernel): route mori EP dispatch/combine to the KernelForge vendor playbook - #1191

Merged
chaojhou merged 6 commits into
mainfrom
feature/zicanli/mori_dispatch_and_combine
Aug 19, 2026
Merged

feat(kernel): route mori EP dispatch/combine to the KernelForge vendor playbook#1191
chaojhou merged 6 commits into
mainfrom
feature/zicanli/mori_dispatch_and_combine

Conversation

@zili-amd

@zili-amd zili-amd commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

Mori's EP dispatch/combine kernels are pip-installed compiled binaries with no rewritable source, so classify_patchability() used to reject them as vendor_binary before any tuning could happen (KernelForge PR #88). This adds a vendor operator playbook: a registry that recognizes known vendor operators and routes them to a validated KernelForge task bundle instead of source rewriting.

Implemented:

  • vendor_operator_playbooks.json + _vendor_operator_playbooks.py — registry + matcher, disambiguates dispatch vs combine by trailing method segment, routes to examples/mori_ep_dispatch_combine/.
  • tracelens_analysis.pyclassify_patchability() checks the registry before rejecting as vendor_binary; groups dispatch+combine into one aggregate GPU% target.
  • forge_submit.py — new _submit_vendor_playbook() path with a session-scoped lock so dispatch and combine share one forge-loop session instead of running twice.
  • Fixed a result-dict field gap that made kernel_optimization.py report PARTIAL/no-speedup even when forge-loop had validated a real speedup internally.

Bugs found & fixed during review (4 rounds, 10 total, all with regression tests)

# Severity Issue Fix
1 High Reused (combine) role lost its optimized artifact (path mismatch) Aligned cli_workspace/output_dir; added a physical artifact copy as a safety net
2 High Exception after claiming the group lock left it stuck all session Wrapped post-claim work so failures always release the lock
3 Medium aggregate_gpu_pct computed but never used for gating Wired into all 3 gate call sites
4 Medium Registry comment overstated what kb_path does Corrected docs (metadata only)
5 Blocking Vendor-playbook artifact could reach apply_kernel_patch's legacy full-file-replace and overwrite the real installed operator Blocked vendor-playbook results from ever reaching auto-integrate
6 Blocking Failed results cached for the whole session, no retry Added a 10-min TTL on failure caching (successes stay cached permanently)
7 High A dead lock-holder (SIGKILL/OOM) blocked submissions for up to an hour Claims now carry {pid, timestamp, nonce} and are safely stolen once stale
8 High Dispatch and combine could double-count the same forge-loop speedup as two wins Only the role that actually ran forge-loop is counted
9 High _candidate_haystack() didn't check trace_launcher_file, so graph-captured kernels (the real production trace shape) never matched the registry at all Added the field + 2 regression tests reproducing the exact real-trace shape
10 Blocking (tech-lead review) _build_hot_kernel_summaries()'s key whitelist dropped patch_strategy/vendor_playbook_* fields, so untried_hot_reusable_kernels() silently degraded the aggregate/floor gate to bare gpu_pct on the real production path (hot_kernels_top15) — losing both the split-load pass-through and the playbook's own floor enforcement Added the 4 fields to the whitelist; 2 new regression tests through the real record_trace_analyze() entry point (existing tests bypassed the projection and couldn't catch this)

Testing

  • Unit: test_vendor_operator_playbook_mori.py 18/18 passing. Full agents/kernel/tests/ + inference_optimizer/tests/ suites checked before/after the latest fix: identical pre-existing, unrelated failure set both times (missing pytest-asyncio plugin) — no regressions.
  • Live e2e — isolated micro-workload (MI300X, dispatch/combine-bottlenecked): correctly routed to the vendor playbook; forge-loop found a validated 1.25x speedup (combine_zero_copy=True, tuned block/warp grid), correctness PASS against mori's own reference test-suite math, single-session dedup confirmed (combine reused dispatch's cached result in 0.1s). This is the run that proves mori is actually re-tuned by KernelForge, with a real measured win, end to end.
  • Live e2e — full production workload (8× MI300X, DeepSeek-Coder-V2-Lite, TP1/EP8/DP8, mori_high_throughput): proves the automatic half of the pipeline off a real serving trace, with zero manual intervention — TraceLens classified dispatch+combine as vendor_playbook on its own (42.7% aggregate GPU%), the orchestrator transitioned PRELUDE → KERNEL_AGENT, forge_submit resolved the target and spawned a real forge-loop subprocess (fresh git worktree, aiter-cache, claude-opus-5 implementer/supervisor), and the combine candidate correctly deduped onto the same session instead of spawning its own. forge-loop's baseline benchmark then hit a native abort inside mori's own compiled shmem library (ShmemStates::CheckStatusValid(), ×8 ranks) before a clean timing round could complete — so this run does not produce a second measured speedup, but the crash trace itself is further confirmation that mori's real operator construction path was reached (not mocked). Handled safely by the orchestrator (REVERT, crash_count: 0, no stuck lock) — see "Known follow-up" below.
    • Net read: detection → routing → dispatch → dedup is proven automatic on a real production trace; the actual tuning result (a measured speedup) is proven only on the isolated micro-workload above, not yet on a live production workload.
  • CI: all checks green, including ci-e2e (a transient CI-infra disk-quota fault on the first run was unrelated to this PR's code — cleared on retest).

Known follow-up (not blocking)

On the full production run, forge-loop's own baseline benchmark crashed with a native abort inside mori's shmem library (ShmemStates::CheckStatusValid()). Handled safely — REVERT, crash_count: 0, no stuck lock, no false-positive speedup — but this specific task bundle couldn't produce a measured speedup on that run.

Root cause is most likely resource exclusivity, not a missing rendezvous: mori's shmem is a GPU-initiated symmetric-memory heap shared across all ranks on a node, and driver.py already does its own complete standalone dist.init_process_group + shmem_torch_process_group_init — it doesn't depend on an external process group. The live vLLM server being optimized was still serving on the same 8 GPUs when forge-loop's driver tried to claim its own independent 8-rank mori heap on top of it; two competing mori-EP groups can't coexist on one 8-GPU box. A real fix means pausing/releasing the live server's GPU allocation around this class of kernel_opt attempt (or routing distributed collective kernels to a phase where the GPUs are guaranteed free) — an orchestrator-level coordination change, not a driver.py patch, and risky to rush (a botched pause/resume could hang a live production server, a worse outcome than today's safe no-op). Filed as follow-up rather than patched here; not a regression risk as-is since the pipeline already fails safe.

Test plan

  • Unit tests: pytest .../test_vendor_operator_playbook_mori.py -q (18/18)
  • Full kernel-agent + inference_optimizer regression suites (no new failures vs. main)
  • Live e2e, isolated micro-workload (dispatch+combine routed, single session, real speedup)
  • Live e2e, full production workload (automatic routing, dedup, graceful failure handling)
  • CI ci-e2e smoke check green
  • Follow-up: confirm the GPU-exclusivity hypothesis above, then either pause/release the live server's GPUs around distributed vendor-playbook attempts or route them to the Coordinator's collective lane, so routed EP kernels can produce a measured speedup on a live DP+EP serving workload

Made with Cursor

@zili-amd
zili-amd requested review from a team, devalshahamd and tsrikris as code owners August 14, 2026 20:06
@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown

CI E2E report — ✅ Succeeded

item value
result ✅ Succeeded
model Qwen/Qwen3-0.6B (dense)
resources 1× GPU, TP=1
PR branch feature/zicanli/mori_dispatch_and_combine
commit 1a637b8719b1fba16814506d1aba7a71199fe209
session_id 932c3cb2-b72f-4027-b8d7-ef05e69448d3
queue → dispatch 0s
run time 173m 45s
total 173m 45s

details

zili-amd added a commit that referenced this pull request Aug 15, 2026
…phaned lock, GPU% gating, kb_path docs)

A review pass on the mori dispatch/combine vendor-playbook path (#1191)
surfaced four real bugs:

- High: a reused (combine) result lost its optimized artifact.
  invoke_backend() unconditionally overwrites result["output_dir"] with
  the current attempt's own (empty) directory after submit() returns, and
  cli_workspace pointed at the git worktree while optimized_versions/ was
  actually written under output_dir -- so _candidate_artifact_paths()
  found nothing for the reused role and make_proposal() returned PARTIAL
  despite a real, validated speedup. Fix: align cli_workspace with
  output_dir (matching the convention the rest of forge_submit.py already
  uses), and physically stage a copy of the artifact under the reused
  attempt's own output_dir as a second, independent safety net.

- High: an exception after claiming a vendor-playbook group's lock
  (e.g. subprocess.CalledProcessError from _copy_vendor_task_bundle's git
  calls, previously only caught as OSError, or anything else unexpected)
  left claimed.lock on disk with no result.json ever written, hanging
  every future submission for that group for the rest of the session.
  Fix: wrap all post-claim work in try/except so any exception always
  writes a failure result.json.

- Medium: aggregate_gpu_pct was stamped on grouped candidates but never
  consulted by the hot-kernel gate, so a split load (e.g. dispatch=7%,
  combine=5%) could be dropped as below_min_gpu_pct on both members
  despite clearing the floor combined; the registry's min_gpu_pct_floor
  was likewise unused. Fix: add effective_hot_kernel_gpu_pct()/
  effective_hot_kernel_min_gpu_pct() helpers and wire them into all three
  gate call sites (untried_hot_reusable_kernels(), the task_group and
  legacy per-kernel passes in _batch_kernel_candidates()).

- Medium: the registry's _comment claimed forge_submit.py "includes
  kb_path in the fellow's context", but the code only ever sets
  KERNELFORGE_INCLUDE_MORI_KB=1. Traced into KernelForge itself:
  that env var is a boolean-only ablation switch with no hook to accept
  an external path, so kb_path was never actually passable. Fix: correct
  the comment instead of adding a no-op pass-through.

Adds regression tests for all four: build_verification()/make_proposal()
now runs on the *reused* result (not just the winner) and asserts KEEP;
a new test simulates a post-claim exception and asserts submit() never
raises and always writes a result; two new gate tests cover the 7%+5%
aggregate scenario and the floor-still-applies case. Verified each new
test actually fails without its corresponding fix, and that the full
agents/kernel/tests + inference_optimizer/tests suites have identical
pre-existing failure sets before/after (unrelated missing pytest-asyncio
plugin and a subprocess PYTHONPATH gap in the sandbox, not this change).

Co-authored-by: Cursor <cursoragent@cursor.com>
zili-amd added a commit that referenced this pull request Aug 17, 2026
…rwrite, session-locked retries, orphaned claims, double-counted benefit)

A second review of the mori dispatch/combine vendor-playbook path (#1191)
surfaced four more real bugs on top of the round-2 fixes, plus confirmed
four smaller findings already partially addressed:

- Blocking: a vendor-playbook KEEP's best_artifact_path is a copy of a
  KernelForge task-bundle config file, not a rewrite of the real installed
  operator -- apply_kernel_patch's legacy full-file-replace strategy would
  happily overwrite the real site-packages module with it once routed
  through NEEDS_REVIEW->LLM-initiated integrate, or automatically once a
  future fix lets vendor-playbook reach KEEP. Fix: stamp
  vendor_playbook_deploy_blocked on the ledger entry in record_kernel_opt(),
  refuse to queue it in _queue_kernel_keep(), and add a second,
  independent refusal in integrate_handler() (via
  _fill_integrate_defaults_from_state()) so an LLM-initiated integrate
  naming the kernel_id directly is caught too.

- Blocking: the vendor-playbook lock/result cache is scoped to the whole
  session, so every failure branch (FORGE_PATH unset, bundle copy failure,
  post-claim exception) permanently retired the group for the rest of the
  session with no way to retry after a transient fault. Fix: cached
  failures now expire after _VENDOR_PLAYBOOK_FAILURE_CACHE_TTL_S (10min);
  cached successes remain permanent, preserving the intended one-session
  dedup for a genuinely concurrent dispatch+combine pair.

- High: claimed.lock carried no owner/timestamp, so a holder killed by
  SIGKILL/OOM/node-restart left every subsequent submission polling the
  full wait deadline (timeout_s + 300s) before failing -- up to an hour
  burned per submission for a 60-minute-budget attempt. Fix: claim markers
  now carry {pid, claimed_at, nonce}; a claim older than its own budget
  (plus grace) -- or backing a failure result that has aged out of the TTL
  above -- is treated as abandoned and safely stolen via a nonce-verified
  atomic replace.

- High: dispatch and combine are separate kernel_ids that both carry the
  identical mean_case_speedup/best_ms from one shared forge-loop session,
  so a benefit collector summing per-kernel-id would double-count one real
  measurement as two. Fix: the role that actually ran forge-loop is
  stamped vendor_playbook_independently_counted=True; a reused sibling is
  stamped False.

- The vendor-playbook path reused one forge-loop run but never wrote
  optimization_report.md, so correctness_passed stayed False and
  make_proposal() could never return KEEP even when SNR validation had
  already passed inside forge-loop. Fix: call the same _write_report()
  the ordinary per-file path uses, where cli_workspace == output_dir.

- _role_haystack() only applied the trailing-symbol-segment fix to `name`,
  not `operation` -- this repo's own convention
  (_task_group_contract.logical_operator_name, _bypass_report.py) is to
  populate `operation` with the same fully-qualified Class::method string,
  which would silently reintroduce the dispatch/combine ambiguity the
  round-2 fix already claimed to close. Fix: apply the same segment
  extraction to `operation`.

- aggregate_gpu_pct collided with an existing task_group-level field name
  (tracelens_skill_runner.py, _bypass_report.py); reading it unconditionally
  off a candidate row would silently change every ordinary group's gating
  behavior if a future change ever flattened task_group data onto candidate
  rows. Fix: renamed to vendor_playbook_aggregate_gpu_pct everywhere it's
  stamped and read.

- resolve_kernel_anchor_path() returned a bare relative path when
  FORGE_PATH was unset, which a later Path(...).resolve() would silently
  reinterpret against whatever the apply-stage process's CWD happened to
  be. Fix: always return an absolute path, anchored under $FORGE_PATH when
  set or a fixed synthetic root otherwise.

Adds regression tests for all of the above: two new SharedState tests plus
four new request_handlers tests for the deploy-blocking guard; six new
tests in test_vendor_operator_playbook_mori.py covering the failure-TTL
retry, claim stealing/non-stealing, the optimization_report.md write, and
the independently-counted dedup flag; one new role-haystack-ambiguity test
and one new absolute-path test. Full agents/kernel/tests/ (1595 passed) and
the touched inference_optimizer/tests/ files pass with the same
pre-existing pytest-asyncio-plugin-missing and subprocess PYTHONPATH gaps
already present on the base branch.

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread src/hyperloom/agents/kernel/tools/backends/forge_submit.py Fixed
zili-amd and others added 3 commits August 17, 2026 12:09
…r playbook

Mori's dispatch/combine kernels are pip-installed compiled binaries with no
rewritable device source, so classify_patchability() previously rejected
them as vendor_binary before any tuning could happen. Add a registry-based
vendor operator playbook (vendor_operator_playbooks.json +
_vendor_operator_playbooks.py) that matches these kernels by
class/operation name, disambiguating dispatch vs combine by trailing method
segment instead of substring match (both appear in
`mori::EpDispatchCombineOp::{dispatch,combine}`), and routes them to the
validated KernelForge examples/mori_ep_dispatch_combine/ task bundle
(KernelForge PR #88) instead of source rewriting.

classify_patchability() checks the playbook registry before the
vendor_binary rejection, and _finalize_candidates() aggregates GPU% across
grouped candidates so dispatch and combine are recognized as one logical
optimization target. forge_submit.py adds a _submit_vendor_playbook() path
that copies+git-inits the task bundle and uses a session-scoped lock/cache
so dispatch and combine share exactly one forge-loop session instead of
running it twice.

Fixes a result-dict field gap (total_improved, pristine_baseline_ms,
search_start_ms, etc.) that caused kernel_optimization.py's verification
pipeline to report PARTIAL/"no measurable speedup found" even when
forge-loop had validated a real speedup internally -- confirmed against a
live MI300X run that achieved a genuine 1.25x speedup.

Co-authored-by: Cursor <cursoragent@cursor.com>
…phaned lock, GPU% gating, kb_path docs)

A review pass on the mori dispatch/combine vendor-playbook path (#1191)
surfaced four real bugs:

- High: a reused (combine) result lost its optimized artifact.
  invoke_backend() unconditionally overwrites result["output_dir"] with
  the current attempt's own (empty) directory after submit() returns, and
  cli_workspace pointed at the git worktree while optimized_versions/ was
  actually written under output_dir -- so _candidate_artifact_paths()
  found nothing for the reused role and make_proposal() returned PARTIAL
  despite a real, validated speedup. Fix: align cli_workspace with
  output_dir (matching the convention the rest of forge_submit.py already
  uses), and physically stage a copy of the artifact under the reused
  attempt's own output_dir as a second, independent safety net.

- High: an exception after claiming a vendor-playbook group's lock
  (e.g. subprocess.CalledProcessError from _copy_vendor_task_bundle's git
  calls, previously only caught as OSError, or anything else unexpected)
  left claimed.lock on disk with no result.json ever written, hanging
  every future submission for that group for the rest of the session.
  Fix: wrap all post-claim work in try/except so any exception always
  writes a failure result.json.

- Medium: aggregate_gpu_pct was stamped on grouped candidates but never
  consulted by the hot-kernel gate, so a split load (e.g. dispatch=7%,
  combine=5%) could be dropped as below_min_gpu_pct on both members
  despite clearing the floor combined; the registry's min_gpu_pct_floor
  was likewise unused. Fix: add effective_hot_kernel_gpu_pct()/
  effective_hot_kernel_min_gpu_pct() helpers and wire them into all three
  gate call sites (untried_hot_reusable_kernels(), the task_group and
  legacy per-kernel passes in _batch_kernel_candidates()).

- Medium: the registry's _comment claimed forge_submit.py "includes
  kb_path in the fellow's context", but the code only ever sets
  KERNELFORGE_INCLUDE_MORI_KB=1. Traced into KernelForge itself:
  that env var is a boolean-only ablation switch with no hook to accept
  an external path, so kb_path was never actually passable. Fix: correct
  the comment instead of adding a no-op pass-through.

Adds regression tests for all four: build_verification()/make_proposal()
now runs on the *reused* result (not just the winner) and asserts KEEP;
a new test simulates a post-claim exception and asserts submit() never
raises and always writes a result; two new gate tests cover the 7%+5%
aggregate scenario and the floor-still-applies case. Verified each new
test actually fails without its corresponding fix, and that the full
agents/kernel/tests + inference_optimizer/tests suites have identical
pre-existing failure sets before/after (unrelated missing pytest-asyncio
plugin and a subprocess PYTHONPATH gap in the sandbox, not this change).

Co-authored-by: Cursor <cursoragent@cursor.com>
…rwrite, session-locked retries, orphaned claims, double-counted benefit)

A second review of the mori dispatch/combine vendor-playbook path (#1191)
surfaced four more real bugs on top of the round-2 fixes, plus confirmed
four smaller findings already partially addressed:

- Blocking: a vendor-playbook KEEP's best_artifact_path is a copy of a
  KernelForge task-bundle config file, not a rewrite of the real installed
  operator -- apply_kernel_patch's legacy full-file-replace strategy would
  happily overwrite the real site-packages module with it once routed
  through NEEDS_REVIEW->LLM-initiated integrate, or automatically once a
  future fix lets vendor-playbook reach KEEP. Fix: stamp
  vendor_playbook_deploy_blocked on the ledger entry in record_kernel_opt(),
  refuse to queue it in _queue_kernel_keep(), and add a second,
  independent refusal in integrate_handler() (via
  _fill_integrate_defaults_from_state()) so an LLM-initiated integrate
  naming the kernel_id directly is caught too.

- Blocking: the vendor-playbook lock/result cache is scoped to the whole
  session, so every failure branch (FORGE_PATH unset, bundle copy failure,
  post-claim exception) permanently retired the group for the rest of the
  session with no way to retry after a transient fault. Fix: cached
  failures now expire after _VENDOR_PLAYBOOK_FAILURE_CACHE_TTL_S (10min);
  cached successes remain permanent, preserving the intended one-session
  dedup for a genuinely concurrent dispatch+combine pair.

- High: claimed.lock carried no owner/timestamp, so a holder killed by
  SIGKILL/OOM/node-restart left every subsequent submission polling the
  full wait deadline (timeout_s + 300s) before failing -- up to an hour
  burned per submission for a 60-minute-budget attempt. Fix: claim markers
  now carry {pid, claimed_at, nonce}; a claim older than its own budget
  (plus grace) -- or backing a failure result that has aged out of the TTL
  above -- is treated as abandoned and safely stolen via a nonce-verified
  atomic replace.

- High: dispatch and combine are separate kernel_ids that both carry the
  identical mean_case_speedup/best_ms from one shared forge-loop session,
  so a benefit collector summing per-kernel-id would double-count one real
  measurement as two. Fix: the role that actually ran forge-loop is
  stamped vendor_playbook_independently_counted=True; a reused sibling is
  stamped False.

- The vendor-playbook path reused one forge-loop run but never wrote
  optimization_report.md, so correctness_passed stayed False and
  make_proposal() could never return KEEP even when SNR validation had
  already passed inside forge-loop. Fix: call the same _write_report()
  the ordinary per-file path uses, where cli_workspace == output_dir.

- _role_haystack() only applied the trailing-symbol-segment fix to `name`,
  not `operation` -- this repo's own convention
  (_task_group_contract.logical_operator_name, _bypass_report.py) is to
  populate `operation` with the same fully-qualified Class::method string,
  which would silently reintroduce the dispatch/combine ambiguity the
  round-2 fix already claimed to close. Fix: apply the same segment
  extraction to `operation`.

- aggregate_gpu_pct collided with an existing task_group-level field name
  (tracelens_skill_runner.py, _bypass_report.py); reading it unconditionally
  off a candidate row would silently change every ordinary group's gating
  behavior if a future change ever flattened task_group data onto candidate
  rows. Fix: renamed to vendor_playbook_aggregate_gpu_pct everywhere it's
  stamped and read.

- resolve_kernel_anchor_path() returned a bare relative path when
  FORGE_PATH was unset, which a later Path(...).resolve() would silently
  reinterpret against whatever the apply-stage process's CWD happened to
  be. Fix: always return an absolute path, anchored under $FORGE_PATH when
  set or a fixed synthetic root otherwise.

Adds regression tests for all of the above: two new SharedState tests plus
four new request_handlers tests for the deploy-blocking guard; six new
tests in test_vendor_operator_playbook_mori.py covering the failure-TTL
retry, claim stealing/non-stealing, the optimization_report.md write, and
the independently-counted dedup flag; one new role-haystack-ambiguity test
and one new absolute-path test. Full agents/kernel/tests/ (1595 passed) and
the touched inference_optimizer/tests/ files pass with the same
pre-existing pytest-asyncio-plugin-missing and subprocess PYTHONPATH gaps
already present on the base branch.

Co-authored-by: Cursor <cursoragent@cursor.com>
@zili-amd
zili-amd force-pushed the feature/zicanli/mori_dispatch_and_combine branch from 745f599 to 125d853 Compare August 17, 2026 21:04
Comment thread src/hyperloom/agents/kernel/tools/backends/forge_submit.py Fixed
A live MI300X end-to-end run (DeepSeek-V2-Lite, TP=1/EP=8/DP=8,
mori_high_throughput backend) surfaced a fifth routing gap beyond the
four review-round-2 fixes: TraceLens reconstructs a graph-captured
launch as a "Synthetic Op" (e.g. `vllm::moe_forward_shared->
EpDispatchIntraNodeKernel_bf16 (Synthetic Op)`) with no surviving
module chain, so `library`/`source_file`/`kernel_repo` all resolve
empty on the real candidate. The only field that still carries the
mori identity marker is `trace_launcher_file` (the Python frame that
launched the op, e.g. `.../site-packages/mori/jit/hip_driver.py`),
which `_candidate_haystack()` did not check -- so `match_vendor_operator_playbook()`
silently returned None and both kernels fell through to
"source file not resolved" instead of routing to the playbook, even
though TraceLens correctly flagged them as the #1 and #3 bottlenecks
(35.8% of GPU time combined).

Add trace_launcher_file to the haystack, plus two regression tests
reproducing the exact real-trace candidate shape.

Co-authored-by: Cursor <cursoragent@cursor.com>
@zili-amd zili-amd added the retest Re-run E2E smoke without a new commit (runs once) label Aug 18, 2026
@github-actions github-actions Bot removed the retest Re-run E2E smoke without a new commit (runs once) label Aug 18, 2026
github-code-quality[bot] and github-advanced-security[bot] (CodeQL) both
flagged the bare `except OSError: pass` around the scratch temp-file
unlink in _steal_stale_claim() as an empty except with no explanation.
The behavior is intentional (the unlink is best-effort cleanup of a file
that has, by that point, already been atomically replaced onto
claim_path or never fully written -- it must never raise out of a
claim-stealing attempt) -- add a comment documenting why, no behavior
change.

Co-authored-by: Cursor <cursoragent@cursor.com>
@chaojhou

Copy link
Copy Markdown
Collaborator

Reviewed the full diff. The design holds up and the nine listed findings are genuinely closed in the code — the deploy refusal is defense-in-depth in both places that matter (_queue_kernel_keep() for auto-enqueue, _fill_integrate_defaults_from_state()integrate_handler() for an LLM-named kernel_id), the claim/steal protocol is sound (O_CREAT|O_EXCL, nonce-verified os.replace, failure-cache TTL and claim staleness aging out together), and submit()'s route check is safe because kernel_optimization.py loads candidates off candidates_path as full dicts, so patch_strategy really does survive to the branch.

One thing to fix before merge.

The aggregate/floor gate is a no-op on the production path in untried_hot_reusable_kernels()

SharedState._build_hot_kernel_summaries() builds summary_entry as an explicit key whitelist (shared_state.py, ~3199-3224), and that is what becomes last_trace_analyze["hot_kernels_top15"]. None of vendor_playbook_aggregate_gpu_pct, vendor_playbook_min_gpu_pct_floor, vendor_playbook_group_id or patch_strategy is in it. untried_hot_reusable_kernels() reads info.get("hot_kernels_top15") or info.get("hot_kernels"), and in production hot_kernels_top15 is always populated, so effective_hot_kernel_gpu_pct() / effective_hot_kernel_min_gpu_pct() degrade to bare gpu_pct / min_gpu_pct at that site.

Of the three gate call sites, the two in _batch_kernel_candidates are fine — that function reads the candidates artifact off candidates_path, which carries the full dicts. Only this one is affected, and it loses both directions of the intended behavior:

  • the split-load case the change exists for (7% + 5% clearing a 10% floor together) is still dropped, so a mori group whose members are each under threshold never shows up as owed work; and
  • the playbook's own min_gpu_pct_floor: 10.0 is not enforced either, so with a loosened HYPERLOOM_KERNEL_OPT_MIN_GPU_PCT (a small fixture at 1.0, say) the group is reported untried and can burn a whole forge-loop session below its own floor. That is the unsafe direction.

The two new tests can't catch this because _set_trace() assigns state.last_trace_analyze directly and bypasses the projection. Fix is adding those keys to summary_entry, plus one regression test that goes through record_trace_analyze() instead of hand-building the dict.

Two small things while you're in that test: test_untried_hot_kernels_vendor_playbook_group_gated_on_aggregate only asserts assert untried, which passes even if just one member clears the gate — worth asserting the specific kernel ids. And its comment says the two rows "share source_file+name", but the names differ (::dispatch vs ::combine), which is precisely what keeps both members in the list rather than collapsing them in the identity dedup.

Also worth letting ci-e2e/run land on 873a610 before merging.

…uction

Tech-lead review finding on PR #1191: SharedState._build_hot_kernel_
summaries() projects each hot kernel into an explicit key whitelist
(summary_entry) that becomes last_trace_analyze["hot_kernels_top15"].
That whitelist never carried patch_strategy, vendor_playbook_group_id,
vendor_playbook_aggregate_gpu_pct, or vendor_playbook_min_gpu_pct_floor.

untried_hot_reusable_kernels() reads hot_kernels_top15 in preference to
raw hot_kernels, and hot_kernels_top15 is always populated in production,
so effective_hot_kernel_gpu_pct()/effective_hot_kernel_min_gpu_pct()
silently degraded to bare gpu_pct/min_gpu_pct at that call site -- the
only one of the three gate call sites affected (the two in
_batch_kernel_candidates read full candidate dicts off candidates_path
instead, so they were never broken). This broke both directions of the
intended behavior: a split-load vendor-playbook group (e.g. dispatch=7%,
combine=5%) clearing a 10% floor together was still dropped as
below-threshold on both members, and the playbook's own
min_gpu_pct_floor was not enforced either, so a loosened
HYPERLOOM_KERNEL_OPT_MIN_GPU_PCT could let a below-floor group burn a
whole forge-loop session.

Fix: add the four fields to summary_entry. Existing coverage
(test_untried_hot_kernels_vendor_playbook_group_gated_on_aggregate/
_floor_still_applies) could not catch this because their _set_trace()
helper assigns last_trace_analyze directly and never populates
hot_kernels_top15, so they fell through to the unprojected raw
hot_kernels list. Added two new regression tests that go through the
real record_trace_analyze() entry point instead (one per direction);
both fail with a KeyError without this fix and pass with it. Also
strengthened the existing aggregate test's weak `assert untried`
(which passed even if only one member cleared the gate) to assert the
specific kernel id set, and fixed its comment, which incorrectly
claimed the two rows share (source_file, name) -- they do not; the
differing names (::dispatch vs ::combine) are exactly what keeps them
distinct instead of collapsing under the identity-dedup fallback.

Verified: the two new tests fail with KeyError without the
shared_state.py fix and pass with it. Full agents/kernel/tests/ +
inference_optimizer/tests/ suites show the identical 1612 pre-existing,
unrelated failures (missing pytest-asyncio plugin) both before and
after this change -- no regressions.

Co-authored-by: Cursor <cursoragent@cursor.com>
@zili-amd

Copy link
Copy Markdown
Collaborator Author

Fixed in 1a637b8 — thanks for catching this, good find.

Confirmed your read exactly: _build_hot_kernel_summaries()'s summary_entry whitelist never carried patch_strategy/vendor_playbook_group_id/vendor_playbook_aggregate_gpu_pct/vendor_playbook_min_gpu_pct_floor, so untried_hot_reusable_kernels() silently degraded to bare gpu_pct/min_gpu_pct on the real hot_kernels_top15 production path (the two _batch_kernel_candidates call sites were unaffected, as you noted, since those read full dicts off candidates_path).

  • Added the four fields to summary_entry.
  • Added two new regression tests through the real record_trace_analyze() entry point (one per direction: split-load pass-through, and floor-still-enforced) — verified both fail with a KeyError without the fix and pass with it.
  • Fixed test_untried_hot_kernels_vendor_playbook_group_gated_on_aggregate's weak assert untried to assert the specific kernel-id set (turns out both k010 and k011 surface without task_groups metadata, since neither the group-key nor identity dedup collapses them — verified empirically before asserting).
  • Fixed the comment claiming the two rows share source_file+name — they do not; the differing names (::dispatch vs ::combine) are exactly what keeps them distinct.

Ran the full agents/kernel/tests/ + inference_optimizer/tests/ suites before/after: identical 1612 pre-existing, unrelated failures (missing pytest-asyncio plugin) both times — no regressions.

Also waiting on ci-e2e/run to land on the new head before merge, per your note.

@chaojhou chaojhou 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.

Verified the fix in 1a637b871. Both halves are there: _build_hot_kernel_summaries()'s summary_entry now carries patch_strategy / vendor_playbook_group_id / vendor_playbook_aggregate_gpu_pct / vendor_playbook_min_gpu_pct_floor, and the two new tests go through the real record_trace_analyze() entry point — one for the pass direction (7% + 5% clearing the 10% default as 12%) and one for the unsafe direction (the playbook's 10.0 floor still blocking under a loosened min_gpu_pct=1.0) — each asserting the projection actually carries the fields, which is exactly the assertion that fails without the change. The weak assert untried is now assert set(untried) == {"k010", "k011"} and the misleading comment about the rows sharing (source_file, name) is corrected.

Also checked the new keys don't disturb the surrounding logic: the kernel_roofline inclusion test just below only inspects its own key tuple (bound_type, arithmetic_intensity, ...), so a None-valued vendor field on an ordinary kernel doesn't change which rows land in kernel_roofline_top15, and breakdown/collectors/kernels.py re-projects explicit keys, so the extra keys are inert downstream.

On the Read the Docs failure — agreed, unrelated to this PR:

  • the same commit passes the repo's own Sphinx build (Actions Docs run 32183961767, head_sha 1a637b8719b1…, build (sphinx html) green in 47s running sphinx-build -b html docs docs/_build/html);
  • the fix commit touches only shared_state.py and one test file, and no docs file changed anywhere in the PR;
  • RTD succeeded on the four earlier commits of this same branch (125d853, 2bfba7c, 873a610), so neither the docs tree nor .readthedocs.yaml is broken here;
  • RTD is failing intermittently on unrelated open PRs too (#1222, #1216) while a dozen others pass; and
  • the failures return in 34-62s versus 78-101s for the successes, i.e. it is failing early (clone/env/install or a queue rejection), not at the end of a real documentation build.

A re-run of RTD build 4332364 should clear it; worth a glance at that build's "Installing" step if it recurs.

Approving. The four non-blocking items from my earlier pass (recommend_backends() still returning [] for a vendor_binary-typed playbook candidate, vendor_playbook_independently_counted having no production reader, both roles consuming a top-N slot, and the breadth of the "mori" substring marker now that trace_launcher_file is in the haystack) are fine to leave for follow-ups. The one worth filing alongside the GPU-exclusivity item is the deployability gap: with integrate refused, a vendor-playbook KEEP is measured but never shipped, so the tuned launch config has no route to a live server yet.

@chaojhou
chaojhou merged commit 0022869 into main Aug 19, 2026
27 of 28 checks passed
@chaojhou
chaojhou deleted the feature/zicanli/mori_dispatch_and_combine branch August 19, 2026 00:37
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