Repository navigation
refactor: delete the dead and repeated code in fourteen over-gate functions - #2024
Merged
Merged
Conversation
…d retrieval code ran inline Each function below was over a cyclomatic complexity of 20, counting every branch, loop, except handler, boolean operator, ternary and comprehension clause. The extracted helper names the job the function did inline: - _compile_call 25 -> 19: _call_head picks the call's one builtin head and rejects the keys that head does not accept. - RetrievalConfiguration.__post_init__ 22 -> 12: _limits and _provider_refs validate the two per-capability maps. - FieldDeclaration.__post_init__ 26 -> 17: _check_role holds the role rules. - QueryView.from_dict 21 -> 16: _field_lookup_from_dict decodes field_lookup. - collect 21 -> 19: _project_unit returns a unit's projection, or the SkippedUnit that says why there is none. No behaviour change; every error message is unchanged. The code-metrics baseline drops the two cognitive-complexity entries this clears and lowers collect's function length from 86 to 82. Co-authored-by: Cursor <cursoragent@cursor.com>
…equests ran inline write_decision_trace now reads as the pipeline its docstring describes: group the calls by decision key, build each KEEP/REVERT and dynamic_action row, then lay the rows out in ts order with their token rollups. The proposer attribution (component, provenance, scope, fingerprint) is one helper, the grouping the journal entry itself gives those fields. stream_json_requests names the usage counters a row reports and whether the per-request outputs add up to the result row's total. No behaviour change. The two write_decision_trace baseline entries are removed. Co-authored-by: Cursor <cursoragent@cursor.com>
The keep_revert and dynamic_action rows of decision_trace.jsonl, and a stream-json request that reports no usage, had no test of their own; the code that builds them now lives in named helpers, so pin their contract. Co-authored-by: Cursor <cursoragent@cursor.com>
…s ran inline Bring DecisionReviewer.prepare_review, DecisionReviewer._commit_coordinator_inbox, classify_attempt, conc_pair_comparison and parse_unified_diff to cyclomatic complexity 20 or below by lifting each inline job into a private helper: - decision_reviewer: the KB-read skip gates and the prior lookups (now one _PriorLookups trail) leave prepare_review; advice grouping, verdict validation and standalone advice intents leave _commit_coordinator_inbox. - assessment: the MUST-have file gap and the validator report each map to an outcome in their own function, under one manifest guard. - gain_math: the pair speedup, the guard-axis reading and the summary stats leave conc_pair_comparison; the pair counts derive from rows and speedups. - unified_diff: filing one hunk body line leaves parse_unified_diff. No behaviour change: messages, exception types, returned shapes and the order of side effects are unchanged. The six code-metrics baseline entries these functions no longer exceed are removed. Co-authored-by: Cursor <cursoragent@cursor.com>
Bring main, _kill_prior and _reap_stale_engines_by_cmdline in the Infera pod launcher to cyclomatic complexity 20 or below by lifting each inline job into a private helper in the same stdlib-only script: - launch_infera_node: _kill_prior and _reap_stale_engines_by_cmdline share one SIGTERM-then-SIGKILL escalation helper, _term_then_kill, instead of each carrying its own copy of the loop; the /proc cmdline scan leaves the reaper as _stale_engine_pids; main hands off the argument parser, the rendezvous-leader choice, the shared server-log dir check (now made once instead of twice), the GPU sampler files and its optional start, the launch-argument validation, the ray node teardown and the leader health wait with its log tail. No behaviour change: the CLI flags and defaults, exit codes, the stdout JSON (key order included), log lines, the prior-server RuntimeError, files written and the order of subprocess, signal and network calls are unchanged. _kill_prior now sends signal.SIGTERM and signal.SIGKILL where it sent 15 and 9; they are the same signal numbers. The four code-metrics baseline entries these functions no longer exceed are removed. Co-authored-by: Cursor <cursoragent@cursor.com>
Bring seed_native_geak_proxy, agentx_measurement, find_reference_rows, gap_analysis, match_variants_to_priors, build_manifest, ingest and _summary_line to cyclomatic complexity 20 or below by lifting each inline job into a private helper: - geak_proxy: assembling the proxy's runtime environment (explicit overrides, the executable's PATH entry, no device pinning) leaves seed_native_geak_proxy. - report: converting the latency percentiles to milliseconds leaves agentx_measurement. - inferencex_client: the single-node hardware filter and the fixed-shape filter become named predicates of find_reference_rows. - research_hints: the three axis-gap formulas, the synthetic and AgentX primary-gap rules and the AgentX status fields leave gap_analysis; tokenizing the hints and a variant leaves match_variants_to_priors. - manifest: the workload block, the run identity (one _RunIdentity) and the all-digits env read leave build_manifest, which also drops eight `args is not None` guards that only restated getattr's default. - backfill_langfuse: one agent span with its generations, the recipe-KB audit subtree and the decision scores leave ingest. - dump_session_breakdown: the GEAK keep count leaves _summary_line. No behaviour change: messages, exception types, returned shapes and the order of side effects are unchanged. The six code-metrics baseline entries these functions no longer exceed are removed. Co-authored-by: Cursor <cursoragent@cursor.com>
fengshaoyi-amd
added
the
size-exception
This was referenced Oct 19, 2026
Closed
Code metrics gate: PASSEDJudged: the 14 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.
116 findings in files this change does not touchOutside this change (informational): 116
...and 91 more; the job log lists every row.
Thresholds and sources: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
fengshaoyi-amd
marked this pull request as draft
October 20, 2026 03:34
Contributor
Author
|
Back to draft while I rework this along the review guidance on the parallel complexity PRs: delete mechanisms instead of relocating them, keep production code net-negative, and touch only functions that were over the code-metrics gate thresholds. I will mark it ready again with the gate's own before/after numbers in the description. |
The first pass split twenty-three functions into helpers without removing a mechanism, so it moved code instead of shrinking it. Every file it touched goes back to its origin/main content: the twenty-two source and test files, the code-metrics baseline entries it added for the new helpers, and test_decision_trace_rows.py, which only pinned helpers that no longer exist. The nine functions that only a cmetrics floor flagged (none of them is over the code-metrics gate) stay at this version. The fourteen that are over the gate are reworked deletion-first in the commits that follow. Safe: these files are byte-identical to origin/main, so their behaviour is main's. Co-authored-by: Cursor <cursoragent@cursor.com>
…ation Deleted the leader /health wait: _wait_health, the --health-port and --health-wait-sec flags, INFERA_FRONTEND_PORT and the argv elements that fed them. build_node_launch_args, the only producer of the launcher's argv, always passed --health-wait-sec 0, so the wait, health_ok and log_tail never ran. The launcher is bundled and shipped with its caller on every launch, so the two cannot drift apart. _kill_prior and _reap_stale_engines_by_cmdline each ran the same SIGTERM, wait, SIGKILL, wait loop. _term_then_kill is that loop once, with the optional process-group signal _kill_prior adds; kill order, liveness probes, timeouts and log lines are unchanged. _kill_prior also loses the is_file() pre-check (the read's OSError handler already covers a missing pid file), the empty targets default and its if (_proc_tree always includes the root) and the truthiness test in front of pid > 0. main reads HYPERLOOM_MN_SERVER_LOG_DIR once instead of evaluating the same check twice, calls _detach_launch once after the framework branch, lets _start_gpu_sampler read its own interval as its first statement, drops the `or ""` on an env value that is always a str, and keeps its argparse definition in _parse_args. Observable: a manual run that passes --health-port or --health-wait-sec now exits 2 with "unrecognized arguments"; nothing in the tree passes them. A non-regular file at the pid-file path is now read instead of skipped; that path is always the launcher's own pid file. Co-authored-by: Cursor <cursoragent@cursor.com>
Deleted _to_num, a forwarding wrapper around coerce.to_float; its callers use to_float directly, and test_to_num goes with the wrapper it tested. _gap_pct is the rule both percent gaps (throughput and interactivity) share in both modes: a gap exists only when both sides parse as positive numbers. It replaces the AgentX-only positive-or-None normalisation of four inputs and the two inline gap conditions. gap_analysis now computes, per mode, only the inputs, TPOT ratio and primary-gap rule that mode uses; AgentX no longer computes the synthetic inputs and rule and then overwrites them. The synthetic primary-gap rule's two branches (with and without a throughput gap) are one comparison against the throughput gap, or 0 when there is none. Observable: a synthetic competitor-target row with a negative interactivity no longer yields an interactivity gap; it is treated like a zero or missing value, as AgentX already did. An accepted throughput that is negative, boolean or infinite no longer yields a throughput gap; gap_for_state, the only production caller, passes a positive number or None. Co-authored-by: Cursor <cursoragent@cursor.com>
Deleted from write_decision_trace:
- the intermediate row's "kind" entry, which nothing read;
- consumed_keys and its if/else: popping a key's calls out of
calls_by_key hands them to the first decision, in time order, that
names the key, exactly as the set did;
- the join-time rebuild of every row with `or ""` / `or {}` defaults:
iso_z and phase_at always return a str and every row carries its
decision dict, so the row built per source is the persisted row and
the join only adds its tokens (the file is written with sort_keys, so
key order does not change);
- the provenance conditional around proposer_for, which already maps ""
to "orchestration";
- five copies of "copy the field when its text is non-empty" (kind,
provenance, scope, fingerprint, variant_name), now one loop.
_keep_revert_decision and _dynamic_action_decision each build the row
for one decision source; write_decision_trace indexes the calls, builds
the rows, attaches token rollups in time order and writes.
write_session_decision_trace is untouched.
Safe: origin/main and this version wrote byte-identical
decision_trace.jsonl files and identical warnings for 3,000 randomly
generated sessions.
Co-authored-by: Cursor <cursoragent@cursor.com>
…fill
Deleted from ingest:
- `or a_lo` and `or p_lo` in the agent and phase end-time fallbacks:
_time_bounds returns both bounds or neither and a datetime is always
truthy, so those operands never decide anything. The fallback that
remains is named once per phase (phase_start).
- the recipe-KB block's own min/max over parsed timestamps; it uses
_time_bounds, which now reads ``ts`` with .get as audit rows need.
- `plan.get("recipe_audit") or []`: build_plan always sets the list.
- `score.get("metadata") or {}`, `score.get("comment") or ""` and the
two hand-copied five-field kwargs lists: decision_to_scores builds
every score with exactly name, value, data_type, comment (a str) and
metadata (a dict), so the score is passed as it is.
- `span is not None` in front of hasattr(span, "score"), which is
already False for None; the phase lookup uses lfmap.phase_of instead
of a copy of it.
- `if p_hi is not None: last_end = p_hi`, now `p_hi or last_end`.
_emit_agent (one agent span with its generations) and _emit_recipe_audit
(the agent:recipe_kb subtree) each emit one subtree of the trace. The
langfuse import handler is untouched.
Safe: against a recording fake SDK, origin/main and this version made
identical span, end, score, create_score, flush and log calls for 1,500
randomly generated sessions.
Co-authored-by: Cursor <cursoragent@cursor.com>
… reviewer prepare_review: - the per-proposal and per-decision prior lookups were two copies of the same list_priors call, hit extraction, unreachable check and request record; one loop now runs over (msg_id, topic) pairs, where msg_id is None for the decision-level lookup exactly as the persisted request record already states it. Proposal.msg_id is always a non-empty str (parse_request and the inbox parser both guarantee it), so None identifies the decision lookup. - the any_kb_unreachable flag is derived from the request records it duplicated. - the read path (scope, lookups, audit trail, notes) is _fetch_kb_priors; prepare_review keeps the bundle setup and the four skip decisions. _commit_coordinator_inbox: - review["advice"] was read and filtered twice (dict with a body_md); it is filtered once and both the per-target inlining and the standalone advice intents read that list. - the validated (item, target, verdict, intent) tuples are gone: the first pass validates every verdict and builds its intent, the second pass commits each item, reading target and verdict from items the first pass already checked. Nothing is committed unless every verdict validates, as before. Safe: with recording fakes for the session memory and the KB writer, origin/main and this version produced identical bundles, outcomes, exceptions and call sequences for 4,000 random prepare requests and 4,000 random coordinator-inbox commits. Co-authored-by: Cursor <cursoragent@cursor.com>
parse_unified_diff: - the "@@" branch is gone: a hunk header starts with "@", which none of the body-line prefixes match, so it already fell through untouched. - the three "+" / "-" / " " branches are one table from prefix to the FileChange list it feeds; the "+++" / "---" exclusion is one check. - the "no leading diff --git" guard skips first and opens the section after, instead of an if/else around the open. Safe: origin/main and this version produced identical FileChange lists for 9,000 random patches built from header, hunk, body, "+++x"/"---x" and empty lines. Co-authored-by: Cursor <cursoragent@cursor.com>
_compile_call: - the built-in heads are BUILTINS.intersection(node) and the rejected keys node.keys() - allowed, replacing two filtering comprehensions. - str(heads[0]) is (head,) = heads: every BUILTINS member is already a str, and len(heads) == 1 was checked just above. Safe: the error messages sort their keys, so set order never shows. origin/main and this version compiled 9,000 random mapping nodes to the same expression or the same MappingError text. Co-authored-by: Cursor <cursoragent@cursor.com>
…fy_attempt classify_attempt: - steps (4) and (5) were guarded by the same "manifest_present and not manifest_parse_error" test twice in a row; they are one block, checked in the same order. - the four "step == FAIL -> outcome" branches are a table walked in the same order, so the first failing step still decides the outcome. Safe: ValidationSteps is a frozen dataclass, so reading every step up front has no side effect. origin/main and this version classified 9,000 random attempt artefacts to the same OutcomeId. Co-authored-by: Cursor <cursoragent@cursor.com>
conc_pair_comparison:
- the speedups list and the successful/failed counters duplicated what
the rows already record; the summary derives them from the rows with
a speedup.
- the best-rung search no longer re-filters with isinstance(float): a
speedup is either None or ot / bt of two to_float results. max() over
the paired rows keeps the first maximum, as the enumerate form did.
- the hand-rolled median is statistics.median, which averages the two
middle values the same way.
- the "guard_axis and" checks and "if guard_axis else None" ternaries
are gone: off the interactivity objective the axes dicts are already
empty, so .get("") is None.
- _norm_conc is gone (see below).
Observable effect: a point whose "conc" is a bool now keys and reports
as True/False instead of 1/0. No in-tree producer emits one: the only
production caller, conc_sweep's _point_from_variant, builds conc with
int(envs.get("CONC", "0")).
Safe: origin/main and this version produced identical rows and
summaries for 6,000 random point ladders, and for 3,000 more with bool
conc values once conc is shown as int.
Co-authored-by: Cursor <cursoragent@cursor.com>
…b.suppress The kill escalation shared by the two kill paths kept their empty except clauses, which CodeQL reports as an empty except without an explanation; contextlib.suppress states the same rule without one. Co-authored-by: Cursor <cursoragent@cursor.com>
fengshaoyi-amd
changed the title
…erloom-misc Co-authored-by: Cursor <cursoragent@cursor.com>
This was referenced Oct 20, 2026
fengshaoyi-amd
marked this pull request as ready for review
October 20, 2026 11:11
Contributor
Author
|
/review |
|
Automated review started for head 88c2711. Progress: https://github.com/AMD-AGI/Hyperloom/actions/runs/39413612949 |
|
Automated review: no verdict applied -- the review job ended with result 'failure'. Comment |
…erloom-misc Co-authored-by: Cursor <cursoragent@cursor.com>
This was referenced Oct 20, 2026
This was referenced Oct 21, 2026
This was referenced Oct 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Fourteen functions across the multi-node launcher, the decision trace, the Langfuse backfill, the critic's decision reviewer, research hints, the session manifest, two
hyperloom.commonhelpers, the KB collector and the quantization driver were over the code-metrics gate. This PR brings each one under every limit it broke, mostly by deleting code: a health wait its only caller always switched off, a second copy of the kill escalation, coercions of values the only caller already passes as numbers, decision-trace rows rebuilt after they were built, span-time and score fallbacks the producer never needs, a KB prior lookup written out twice, and manifest fields read once from the env and again from args.Production code: +384 / −637 (net −253) across 12 files. The code-metrics baseline loses 21 entries and gains none. The cleanup continues in #2026 and #2052.
How to read it. This branch was opened before the rework, so its history keeps the first pass (helpers extracted from these functions) and
8925eee2a, which returns all of it to main's code. Review the overall diff, or the commits from0bd299e05on: each changes one module, and its message lists what it deleted and why that is safe.multi_node/scripts/launch_infera_node.py,multi_node/_internal/infera_support.pytrace/decision_trace.pytools/backfill_langfuse.pyagents/critic/runtime/decision_reviewer.pybaseline_comparison/research_hints.pysession/manifest.pycommon/gain_math.pycommon/unified_diff.pyhyperloom_kb/collect/engine.pyhyperloom_kb/collect/expressions.pyclassify_attemptagents/quantization/driver/assessment.pymulti_node/,trace/,tools/,baseline_comparison/andsession/are undersrc/hyperloom/inference_optimizer/;agents/andcommon/undersrc/hyperloom/;hyperloom_kb/issrc/hyperloom_kb/.Gate numbers per function
Measured with the gate's own tools (ruff C901, complexipy, function lines), base → head. Limits are 20, 30 and 80; a value at a limit passes. Bold values were over the limit at base.
launch_infera_node.main_kill_prior_reap_stale_engines_by_cmdlinewrite_decision_tracebackfill_langfuse.ingestprepare_review_commit_coordinator_inboxgap_analysisbuild_manifestconc_pair_comparisonparse_unified_diffcollect(KB collector)_compile_callclassify_attemptAcross the 12 touched production files: C901 total 780 → 727 and cognitive total 1,398 → 1,226. Functions over a gate limit: cognitive 13 → 1 and length 9 → 0. The one left,
_reap_stale_engine_ports(cognitive 51, already in the baseline), is a function this PR does not change.New names
_term_then_kill(launcher): SIGTERM, wait, SIGKILL, wait, for a list of pids and an optional process group._kill_priorand_reap_stale_engines_by_cmdlineeach wrote this loop out._parse_args(launcher): the launcher's argparse definition, out ofmain._keep_revert_decisionand_dynamic_action_decision(decision trace): the row for one KEEP/REVERT entry and for one dynamic-action dispatch.write_decision_traceindexes the calls, builds the rows, attaches token rollups in time order and writes._emit_agentand_emit_recipe_audit(Langfuse backfill): one agent span with its generations, and theagent:recipe_kbsubtree._fetch_kb_priors(decision reviewer): one loop over(msg_id, topic)pairs for the KB prior lookupsprepare_reviewwrote out once for proposals and again for the decision.msg_id=Noneis the decision-level lookup, as the persistedkb_priors_tracerequest record already states it._gap_pct(research hints): the one rule both percent gaps share in both modes, a gap only when both sides are positive numbers._env_digits(manifest): an env var as an int when it is all digits, elseNone; five manifest fields use it.hyperloom.common.env.env_intis not a fit: it accepts negatives and raises on other text.Removed:
_wait_health, the--health-portand--health-wait-secflags andINFERA_FRONTEND_PORT(launcher);_to_num, a forwarding wrapper aroundcoerce.to_float(research hints);_norm_conc(conc-pair comparison).Why the deletions are safe
Each one rests on a fact at the call site:
build_node_launch_args, the only producer of the launcher's argv, always passed--health-wait-sec 0(infera_support.py:139-140at the merge base), so the wait never ran. The launcher is bundled with its caller and shipped on every launch (commands/infera.py:149), so the two cannot drift apart.gap_analysis. Its only production caller,gap_for_state(research_hints.py:361), passes a positive number orNone._norm_conc. The only production caller,conc_sweep's_point_from_variant, buildsconcwithint(envs.get("CONC", "0"))(conc_sweep.py:183).prepare_review. It is recognised bymsg_id is None. Every proposal'smsg_idis a non-empty string:parse_requestrequires it (request_models.py:162) and the inbox parser only matches[A-Za-z0-9_\-]+(inbox_parser.py:24,:205).collect's two error handlers._prepare, the one place both errors are raised from and called only fromcollect(engine.py:495), now raises the skip itself with the same reason.classify_attemptreading every step up front.ValidationStepsis a frozen dataclass (quantization/driver/result_collector.py:27-28), so the reads have no side effect.or []and hand-copied score fields.build_planalways setsrecipe_audit(backfill_langfuse.py:161), anddecision_to_scores(trace/langfuse_mapping.py:253) builds every score with exactly name, value, data_type, comment and metadata.getattron an argparse namespace has no side effect andbuild_provenancenever writesos.environ, so reading each field once in a different order reads the same values.Observable effect
Nothing an operator sees changes. Launches, kills, decision traces, Langfuse spans and scores, critic reviews, research hints, manifests, KB collection reports and log lines are the same for every input the repo's own callers produce.
The differences below are reachable only by input that no caller in the repo produces:
launch_infera_node.pywith--health-portor--health-wait-secnow exits 2 with "unrecognized arguments". Nothing in the tree passes them.gap_analysis. A synthetic competitor-target row with a negative interactivity no longer yields an interactivity gap; it is treated like a zero or missing value, as the AgentX mode already did. An accepted throughput that is negative, a bool or infinite no longer yields a throughput gap.conc_pair_comparison. A point whoseconcis a bool is keyed and reported asTrue/Falseinstead of1/0.build_manifest. With--isl/--oslgiven,$ISL/$OSLis no longer parsed. That matters only for a valuestr.isdigit()accepts andint()rejects, such as "²", on which the base code raised even though the argument overrode it.Checklist
test_to_numgoes with_to_num, the forwarding wrapper it tested;gap_analysiskeeps its existing tests. No other test changes.decision_trace.jsonland warnings.classify_attempt: 9,000 inputs each.conc.size-exception. Eleven modules share one review round, each rework commit changes one module and reads on its own, and the net is −253.build_manifest(80 lines) and_commit_coordinator_inbox,gap_analysis,parse_unified_diff,_compile_callandclassify_attempt(cognitive 30 each) sit at a limit._to_num,_norm_conc, the AgentX-only normalisation ingap_analysis).hyperloom mnships with each launch, and it never enabled them.Known merge interactions
73b01c071, fix: narrow, remove or log the remaining broad handlers outside the files under restructuring #2049's own commit. Its conflict inlaunch_infera_node.mainis resolved by keeping this PR's sampler call under fix: narrow, remove or log the remaining broad handlers outside the files under restructuring #2049'sexcept (OSError, ValueError)and log line; the interval parse that can raise theValueErrornow runs at the top of_start_gpu_sampler, still inside thattry._start_gpu_sampler, which here reads its own interval instead of takinginterval_s, and adds a constant next to_wait_health, which this PR deletes.conc_pair_comparison, which this PR rewrites.scripts/code_metrics_baseline.txt. About forty other open PRs share only that file.Gate
scripts/code_metrics.py --base-ref origin/mainpasses: 0 new, 0 worse. Baseline: −21 entries, +0.ruff checkandruff format --checkare clean on the touched files.