Skip to content

feat(kb): record the roofline on every recipe - #1217

Open
rpoornac wants to merge 2 commits into
mainfrom
feat/rpoornac/kb-roofline
Open

feat(kb): record the roofline on every recipe#1217
rpoornac wants to merge 2 commits into
mainfrom
feat/rpoornac/kb-roofline

Conversation

@rpoornac

Copy link
Copy Markdown
Collaborator

A recipe only kept what beat the incumbent, so a session that improved nothing left behind session rows and no measurement. The distance to the roofline is worth keeping either way: it says whether the run was near its ceiling or nowhere close, and which side bound it.

Project the latest roofline snapshot onto the recipe through extras, so the write sits outside the has_validated_win gate that guards best_config. Extras merge with the prior row, so a later session without a roofline preserves the earlier one rather than erasing it.

Local mode only. The remote publisher still filters roofline actions out of its new-KEEP test, and nothing reads the stored value back yet.

A recipe only kept what beat the incumbent, so a session that improved
nothing left behind session rows and no measurement. The distance to the
roofline is worth keeping either way: it says whether the run was near
its ceiling or nowhere close, and which side bound it.

Project the latest roofline snapshot onto the recipe through extras, so
the write sits outside the has_validated_win gate that guards
best_config. Extras merge with the prior row, so a later session without
a roofline preserves the earlier one rather than erasing it.

Local mode only. The remote publisher still filters roofline actions out
of its new-KEEP test, and nothing reads the stored value back yet.

Co-authored-by: Cursor <cursoragent@cursor.com>
@rpoornac
rpoornac requested a review from a team as a code owner August 18, 2026 00:40
@github-actions

github-actions Bot commented Aug 18, 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 feat/rpoornac/kb-roofline
commit 98c2ec31202e235021640a4599955c019a6b4791
session_id 9f1b3678-8c42-40af-8cc0-acbb5f0e1f83
queue → dispatch 0s
run time 164m 20s
total 164m 20s

details

@rpoornac
rpoornac requested a review from xiaofei-zheng August 18, 2026 01:13
@xiaofei-zheng
xiaofei-zheng requested review from lishuoshuo-amd and removed request for ZhengGong-amd and meinali-566 August 18, 2026 01:51
@ZhengGong-amd

Copy link
Copy Markdown
Collaborator

Reviewed. No blockers — tests pass (7 new + 529 in the related subset), ruff is unchanged vs the merge base, and the extras routing does exactly what the description claims. A few things worth fixing:

1. snapshot_count is inconsistent with "project the latest snapshot" (medium)

Every other key comes from snaps[-1]; this one comes from snaps. It also gets overwritten per session, so a recipe ends up carrying "how many snapshots the session that last wrote this row took" — which reads like "this roofline aggregates N samples" but isn't. snapshot_id already identifies which snapshot was projected. Suggest dropping it, or renaming it and documenting in the docstring that it describes the writing session, not the recipe. It's currently undocumented.

2. _project_perfmodel_breakdown can emit a truncation marker with no data (low)

build_recipe_roofline([{"ts": "t", "perfmodel_breakdown": {"ops": [{} for _ in range(69)]}}])
# -> {'ts': 't', 'perfmodel_breakdown': {'ops_truncated_from': 69}, 'snapshot_count': 1}

If every op projects to empty, ops is omitted but ops_truncated_from is still written — it reads as "69 rows, capped to 64" when it's actually 0. build_recipe_roofline has the if not out: return {} guard for exactly this; the inner function doesn't. One-line fix: move the assignment inside if projected_ops:.

3. No pointer to where the field came from (low)

roofline isn't in Recipe.from_dict's well_known set, has no doc entry, and no reader. All fine given the deliberate write-only scope, but someone finding this block in a recipe JSON later has only grep to go on. A line in the build_recipe_roofline docstring naming the writer and the "no reader yet, local mode only" status would cover it.

4. Optional: guard the allowlists against drift

The three key tuples restate field names from build_roofline_snapshot / attach_perfmodel_breakdown ~300 lines up. The allowlist is the right call (it's what keeps kernel_roofline_path out of the recipe), but nothing flags a new snapshot field that never gets projected. A static test asserting snapshot_keys - allowlist == <explicit deliberately-excluded set> would make that a red test instead of a silent omission.


Things the PR got right that I'd rather not see regress: is None or == "" instead of a falsy check (so comm_pct=0.0 survives as a real measurement), the allowlist keeping the session-local kernel_roofline_path out, truncation leaving a trace rather than being silent, and no try/except anywhere in a pure function. The docstrings explaining why snapshots[-1] and why {} rather than an empty shell are also doing real work.

snapshot_count was read off the snapshot list while every other field
came from snapshots[-1], and each session overwrote it, so a recipe
carried how many snapshots the last writing session happened to take —
which reads as if the roofline aggregated them. snapshot_id already
names the snapshot that was projected, so drop the count rather than
rename it.

ops_truncated_from was written whenever the op list exceeded the cap,
including when every op projected to nothing and no ops array was
stored, leaving a marker that claimed rows the recipe never held. Write
it only alongside a non-empty ops, which lets an all-empty breakdown
fall out entirely.

Name the writer and the local-mode, write-only status in the docstring
so the block found in a recipe JSON points back at what produced it, and
pin the projection allowlists against the fields build_roofline_snapshot
and the PerfModel breakdown dataclasses emit, so a new snapshot field
fails a test instead of being silently dropped.

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

Copy link
Copy Markdown
Contributor

Thanks for the PR — the implementation looks fine.

I don't think this requirement is worth merging as-is: roofline is already captured and used within each session, nothing reads recipe["roofline"] back, and the write path is local-only while production uses remote KB. Persisting this adds storage without changing runtime behavior.

I'd recommend closing this PR. If there's a concrete read-side use case later, we can revisit with a full design.

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