Repository navigation
refactor(enablement, measurement): delete the dead and repeated code in seven over-gate functions - #2076
Conversation
verify_applied coerced every field of the report kernelforge's
parse_log_file returns: `or {}` / `or 0` / `or []` fallbacks, int() and
str() casts, and a separate list for the consulted tables. parse_log
builds that report on every path (missing or unreadable logs included)
with integer hit/miss counts and sorted string table lists, so the
coercions never changed a value; they are gone, and the consulted and
merged table names feed one set directly.
The runtime table names were stripped, emptied-out and str()-cast, then
re-defaulted with `canonical or set()`. The only production caller
passes values of _AITER_ENV_TO_TABLE, which are fixed, clean file
names, so they are now compared as given. The arrival check's two
outer conditions are one `if`, and the "# 1." / "# 2." step numbering
and a comment restating the branch above it are deleted.
Verdicts, details and the order of checks are unchanged.
Co-authored-by: Cursor <cursoragent@cursor.com>
_derive_checkpoint_weight_facts grouped the offending parameter names
into layer-index-stripped families, then answered "which layers carry
this family in the checkpoint" by turning every family back into a
regex and rescanning every checkpoint key with it. A key matches that
regex exactly when the same folding maps it to the same family, so the
checkpoint keys now go through the one grouping rule
(_layers_by_family) and each family looks its present layers up. The
nested _family/_layer_idx helpers, the single-key {"missing_layers": ...}
wrapper and the per-family regex construction are deleted.
Also deleted:
- the function-local `import glob/json/re` and the second, aliased
import of Path the module already has;
- the model-path normalisation in front of resolve_local_model_dir
(getattr default, str/strip, the fallback to the unresolved path and
the second is_dir check): the resolver already strips, expands and
checks the directory, and SharedState declares model_path. The
resolver is imported from its owner, hyperloom.common.model_paths;
- the `if not lines` return, unreachable once `offending` is non-empty,
and a comment promising a single-file key listing that never existed.
The try now covers only the model resolution and the index read, the
only statements that raise OSError/ValueError. The FACTS text, the
trigger and every "" return are unchanged.
Co-authored-by: Cursor <cursoragent@cursor.com>
snapshot_round followed every copy with the same envelope, seven times: `if _copy(src, dest): archive.record(role, dest)`. No caller of _copy used its result for anything but that record, so the copy helper now records the copy that landed (_archive_copy) and each call site is one line. The server-log tail keeps its own copier and joins its empty-path check and its record into one `if`. The workspace sweep for unapplied patches was three nested loops over two directories and two patterns; it is now one loop per directory over `sorted(*.patch) + sorted(*.diff)`, the same files in the same order, so the archive records land in the order they did. A refused copy still marks its name seen, and every size limit, destination and role is unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Delete the always-empty stop_reason / stop_set path and the redundant advanced flag: every producer that sets advanced=True also sets status="advanced". Extract the archive + setting-script write into _archive_round so maybe_rearm_enablement stays under the line budget. Co-authored-by: Cursor <cursoragent@cursor.com>
… vLLM Delete the duplicated disk/toolchain/workspace/constraints envelopes from run_aiter_build and run_vllm_source_build into one _TargetedRecipe owner. run_sgl_kernel_build is left untouched (#2050). Public recipe entry points become thin runners; install stays on the recipe classes. Co-authored-by: Cursor <cursoragent@cursor.com>
…the gate All six enablement targets fell within every gate threshold they violated; drop their baseline rows (and the cleared targeted_build duplicated-lines entry). Co-authored-by: Cursor <cursoragent@cursor.com>
fengshaoyi-amd
added
the
size-exception
Co-authored-by: Cursor <cursoragent@cursor.com>
…in the breakdown recorder AuthoringRound.row marked a round advanced when its status was advanced or when the result carried advanced=True. The lane already decides from the status alone, and the only producer (integrate_patch's advanced result) writes advanced=True only together with status=advanced, so the second rule never changed a row. The row keeps its persisted advanced field. Co-authored-by: Cursor <cursoragent@cursor.com>
…g it on the recipe preflight() and prepare() now return the failed BuildResult, or None, and both run methods use the returned value; the recipe no longer carries a result attribute as a second result channel. Co-authored-by: Cursor <cursoragent@cursor.com>
Brings back the comments that say what the code cannot: why vLLM's build needs CUDA_HOME and 20 GB of disk, that pip install -e is where CMake compiles, why a torch ABI Python mismatch is only an advisory, what the load probe checks, and why model_path is resolved before the sharded-index read (now the docstring of the helper that does that read). Comments that only narrated the code stay deleted. Co-authored-by: Cursor <cursoragent@cursor.com>
Code metrics gate: PASSEDJudged: the 7 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.
Module length warning: over 800 lines, within the 1200-line failure limit (not failing): 3
Already over at the base (backlog, not this change's): 1
88 findings in files this change does not touchOutside this change (informational): 88
...and 63 more; the job log lists every row.
Thresholds and sources: |
ZhengGong-amd
left a comment
ZhengGong-amd
left a comment
There was a problem hiding this comment.
Please reconsider one point before merge.
run_sgl_kernel_build(src/hyperloom/orchestrator/enablement/runtime/targeted_build.py:548-722) is still a third copy of the preamble that the new_TargetedRecipe(:269-403) now owns:_run/_fail, disk preflight, toolchain + torch ABI,gpu_archcheck, workspace, constraints,installed_versions, and even the deadif venv_dir else ""branch (:707) this PR removed from vLLM. The reason given (#2050 rewriting it in parallel) no longer holds: #2050 is already in the merge base. AGENTS.md Finish the replacement.
Action: add_SglKernelBuild(_TargetedRecipe)keeping only the sgl-specific install/probe steps, makerun_sgl_kernel_buildconstruct and run it, and drop its baseline entry. A local prototype of this passes the runtime tests and removes another ~99 lines. Consider making thegpu_archrequirement a base-class switch shared by vLLM and sgl.
…blement-pr Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…blement-pr Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the review.
|
xiaofei-zheng
left a comment
xiaofei-zheng
left a comment
There was a problem hiding this comment.
What this PR does
Six functions under src/hyperloom/orchestrator/ sat over the code-metrics gate and carried waiver rows in scripts/code_metrics_baseline.txt. This PR brings each under the limits by deleting what it did twice or what no caller used — the always-empty stop_reason and the dead stall branch that read it, the advanced flag read beside the status its only producer writes with it, the per-family regex rebuilt from the family rule it inverts, the coercion of a Forge report Forge already produces in that shape, seven copy-then-record envelopes in the round archive — and by lifting the shared ROCm build preamble of all three targeted-build recipes into a _TargetedRecipe base with one subclass per recipe. 12 baseline rows go, none is added.
Blocking issues: 1
-
The description still describes the pre-review head: it says
run_sgl_kernel_buildis untouched, while the head rewrites it.The commit
refactor(enablement): share targeted build setup— the 12th on the branch, pushed after the CHANGES_REQUESTED review — replaced the inline sgl recipe with_SglKernelBuild(_TargetedRecipe)plus a 7-line runner (src/hyperloom/orchestrator/enablement/runtime/targeted_build.py:549-631) and removed its 159-linefunction-linesbaseline row. Four statements in the description contradict that head:- "
run_sgl_kernel_buildis not touched: #2050 rewrote it in parallel and has since landed." - "
run_sgl_kernel_buildis byte-identical to base." - "
run_sgl_kernel_build(:548) is byte-identical to base (:485at base)." - "What is left is outside this work, all on length:
run_sgl_kernel_build, …"
The totals went stale with them:
- "Production code: +583 / −662 (net −79) across 6 files" — the compare API against the merge base reports +649 / −828 (net −179) over those same six files.
- "1,245 changed production lines against the merge-base" — it is 1,477 by the body's own definition (added plus deleted lines of non-test
.pyundersrc/andscripts/). - "The code-metrics baseline loses 11 entries" — the diff deletes 12 rows; the gate's own comment reports −4 cognitive, −1 cyclomatic, −6 function-length.
The per-commit table stops at
30ea99087, the gate-numbers table carries no row for the sgl recipe, and the citations in Why the deletions are safe still point at the old head: the driver dispatch istargeted_build.py:822, not:917-922, and the exports are:870, not:969/:971. The trailing Review response section states the correct fact but leaves all of the above standing, so the two halves of the description disagree with each other.Impact: the release cut aggregates these descriptions. A reader is told the third recipe was left alone when it was in fact rewritten, and is given production-line and baseline-entry counts that do not match the diff.
Action: update the description against the head commit — the opening paragraph, the commit table, the gate-numbers table, the three totals, and the Targeted builds bullet with its line citations — and fold the Review response text into them instead of leaving it as a contradicting appendix.
- "
No other blocking issue: the code changes themselves check out.
Checked: targeted_build.py — all three recipes step by step against base (disk → toolchain → torch ABI → gpu_arch → workspace → constraints → install → probes, installed_versions key order, run_timeout 3600/5400); lane.py — maybe_rearm_enablement, _archive_round, _is_foreign_stall, _answered_round_outcome, _record_enablement_round, _MOVED_ROUND_OUTCOMES, and stop_set at base (lane.py:540 is its only assignment, read at :610/:623, so the removed elif stop_reason branch was dead), with OUTCOME_STALLED keeping its live writer at lane.py:125; both production callers of maybe_rearm_enablement (phases/framework.py:1265-1271 returns for a non-dict, loop/intent_router.py:840 passes a literal dict), so dropping the isinstance guard is safe; integrate_patch.py:3388,3409 — the only writer of "advanced": True always writes "status": "advanced" beside it, and the readers are enablement_event.row:310 → :648 → reporters/_renderers/enablement.py:51,68; params.py — _layers_by_family / _checkpoint_weight_map against common/model_paths.py:72-97; apply_verification.py against kernelforge/gemm_tune/evidence.py:543-556 and :570-577 (apply_verdict.hit/.miss are always ints and merged_tables/consulted_tables always sorted string lists, including for an unreadable log) and its only caller phases/kernel.py:2755-2766 with _AITER_ENV_TO_TABLE at :140-148 (fixed file names, so dropping the strip/drop normalisation of runtime_table_names is safe); artifacts.py — _archive_copy and the evidence sweep.
Ran: a standalone comparison of _layers_by_family against the old per-family regex over realistic weight_map keys — identical present-layer sets; gh api compare/<base>...<head> for the per-file line counts. No local suite: CI is green at this head (all 12 test shards, coverage, code-metrics, pr-hygiene, ruff, mypy, pylint, import-linter, CodeQL).
Base bb2da63 · Head dc27317.
Not checked: #1032, #1154, #1276 and #1869 list more than 100 files each, so only their first 100 were compared for in-flight overlap.
Verified at dc27317: run_sgl_kernel_build is now _SglKernelBuild(_TargetedRecipe) and its baseline row is gone. CI green, merges cleanly with main. xiaofei-zheng's review on this head (description still says the sgl recipe is untouched; stale totals) is a separate open item.
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
Verified at dc27317: run_sgl_kernel_build is now _SglKernelBuild(_TargetedRecipe) and its baseline row is gone. Every review item on the code is fixed, CI is green and the branch merges cleanly with main.
Before or right after merge, please refresh the PR description against this head (line and module counts, commit table, and the statements xiaofei-zheng / ZhengGong-amd listed): the release notes are built from it.
LGTM
fengshaoyi-amd
changed the title
What this does
Seven functions under
src/hyperloom/orchestrator/were over the code-metrics gate: the enablement round archive (snapshot_round), the lane's rearm (maybe_rearm_enablement), the checkpoint weight facts (_derive_checkpoint_weight_facts), the apply-verification verdict (verify_applied), and the AITER, vLLM and SGL targeted-build recipes (run_aiter_build,run_vllm_source_build,run_sgl_kernel_build). This PR deletes what they did twice or what no caller used, and moves the ROCm build setup the three recipes each carried into one_TargetedRecipebase.Production code: +649 / -828 lines (net -179) in 6 files. Tests: unchanged. Code-metrics baseline: 12 entries retired, none added.
What changes in each module
measurement/apply_verification.py:verify_appliedreads Forge's apply report asparse_log_fileproduces it, without theor {}/or 0/or []fallbacks, theint()/str()casts and the normalisation ofruntime_table_names.enablement/params.py:_layers_by_familygroups the offending names and the checkpoint keys by family with one rule, replacing a regex rebuilt for every family, and_checkpoint_weight_mapreads the sharded index'sweight_map.enablement/artifacts.py:_archive_copycopies one file and records its role, replacing_copyand the copy-then-record block around each call.enablement/lane.pyandbreakdown/recorder/enablement_event.py: the always-emptyRoundSettlement.stop_reasonand the stall branch that read it are deleted. A round counts as advanced from its status alone, in the lane and in the breakdown recorder, instead of also reading theadvancedflag its only producer writes beside that status._archive_roundtakes the archive step out ofmaybe_rearm_enablement.enablement/runtime/targeted_build.py:_TargetedRecipeholds the setup the AITER, vLLM and SGL recipes each carried: disk and ROCm toolchain preflight, the isolation workspace and ROCm constraints, andinstalled_versions._AiterBuild,_VllmBuildand_SglKernelBuildkeep only their install and probe steps, andrun_aiter_build,run_vllm_source_buildandrun_sgl_kernel_buildkeep their signatures and run their recipe.gpu_archis derived from the action, and each recipe states throughgpu_arch_errorwhether it needs one: vLLM and SGL do, AITER does not. The duplicated block the baseline carried for this file is gone.Functions brought under the gate
Baseline entries this PR retires, with their values at base:
enablement/artifacts.py:snapshot_round(cognitive 38)enablement/lane.py:EnablementLane.maybe_rearm_enablement(lines 104)enablement/params.py:EnablementParams._derive_checkpoint_weight_facts(cognitive 32, lines 117)enablement/runtime/targeted_build.py:run_aiter_build(cognitive 42, cyclomatic 23, lines 218); the module's duplicated lines 80;run_sgl_kernel_build(lines 159);run_vllm_source_build(lines 220)measurement/apply_verification.py:verify_applied(cognitive 32, lines 81)Observable effect
None for any input the repo's own callers produce: archived files and their roles, setting scripts, settle outcomes and stall counts, round and lane-event rows, log lines, the weight-facts text, apply verdicts and their details, and build results (failure classes, summaries, runtimes, and
installed_versionswith its key order) are the same.Inputs that no in-repo caller produces now behave differently:
verify_applied. A parser report withoutapply_verdict,merged_tablesorconsulted_tables, or withNonein their place, raises instead of reading as empty; counts and table names are no longer converted withint()/str(), andruntime_table_namesentries are compared as given._derive_checkpoint_weight_facts. A~-prefixedmodel_pathwhose expansion is not a directory, but whose literal path is, now gives no facts; ashared_statewithoutmodel_pathraisesAttributeError; a layer index too long forint()raisesValueError; a checkpoint key containing a literal{N}can count toward a family, and one ending in a newline no longer does.maybe_rearm_enablement. A result that is not adictis no longer ignored (NoneraisesAttributeError). A result with"advanced": Trueand a status other than"advanced"is no longer treated as advanced, by the lane or by the breakdown round row;integrate_patchwrites"advanced": Trueonly with"status": "advanced". A non-string status whosestr()is"kept"or"advanced"is still rearmed but no longer exports a setting script or records an accepted launch config, and an unhashable status also logs "enablement: artifact write failed".run_vllm_source_build. Amax_jobsthatint()cannot convert raises after the workspace is prepared instead of before;TargetedBuildAction.from_statecoerces it.Checklist
test_targeted_build_recipes.py(the AITER, vLLM and SGL recipes' installs, probes,installed_versionsand setup-stage failures),test_apply_verification.py,test_artifacts.py, and the weight-facts and rearm tests intest_enablement_coordinator_wiring_unit.pyandtest_sbd_v6_enablement_wiring.py. Linux CI is green at headdc27317f7.size-exception. Most of it is the three targeted-build recipes moving onto_TargetedRecipetogether; a base class carrying one recipe would be an abstraction without its second implementation. No function this PR adds or changes crosses a gate limit. Over a review trigger and within the gate, each one ordered pass over its input:_derive_checkpoint_weight_facts(76 lines),verify_applied(74),maybe_rearm_enablement(72) andsnapshot_round(65 lines, cyclomatic 12, down from 18).targeted_build.py,lane.pyandenablement_event.pyare over the 800-line module trigger and under the 1,200-line limit;targeted_build.pyis 142 lines shorter andlane.pyone line longer.maybe_rearm_enablementnarrows its annotation fromdict[str, Any] | Nonetodict[str, Any], which both callers already pass. The removedRoundSettlement.stop_reasonfield and themovedandis_advancedparameters were used only insidelane.py.orchestrator/enablement/andorchestrator/measurement/under the gate.Commits
Squash-merged as
30d1a0fb1from headdc27317f7.6a96dbcd8refactor(measurement): read forge's apply report as it is produced54283d25arefactor(enablement): group checkpoint keys by the same family rulea736d4215refactor(enablement): record each archived copy where it is made9b75e6a08refactor(enablement): settle a moved round from its status alone2982b26dfrefactor(enablement): share the ROCm build preamble between AITER and vLLMb7d116a43refactor(enablement): retire code-metrics baseline entries now under the gate58cf18721refactor(enablement): decide an advanced round from its status alone in the breakdown recordera4ba1434crefactor(enablement): return a build step's failure instead of storing it on the recipe30ea99087refactor(enablement): restore the comments that state constraints36156b6bbrefactor(enablement): share targeted build setuporigin/main