Repository navigation
refactor: every variant ends on the one abort path - #1896
chennyiiis wants to merge 1 commit into
Conversation
Sixteen places end a variant, and three different mechanisms decided what that meant for the grid: the abort handler, `_record_round_stop`, and two inline tails after a benchmark that returned nothing usable. Each read `keep_going_on_failure` or returned a flag of its own, and two of them filled the rest of the grid with not-run rows. All sixteen now raise `_VariantAborted`, and the halt is data on it: CONTINUE for an outcome the grid always walks past, ON_FAILURE for one the policy decides, STOP for one that ends the batch and carries the rows for the variants it will not reach. The rc asymmetry that was buried in two `if rc != 0 and not keep_going_on_failure` tails is now the difference between two enum members. `_record_round_stop` becomes `_abort_on_round_stop` and only builds the result: it no longer appends, no longer extends the grid, and no longer answers a question its caller then re-asked. Cyclomatic complexity of run_grid drops from 61 to 55, cognitive from 196 to 177, and its length from 1040 lines to 1026. Refactor with nothing observable: the same rows in the same order, the same abort markers, and the same decision at every one of the sixteen endings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code metrics gate: PASSEDJudged: the 3 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.
89 findings in files this change does not touchOutside this change (informational): 89
...and 64 more; the job log lists every row.
Thresholds and sources: |
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #1896 -- refactor: every variant ends on the one abort path
What it does: In run_grid, the decision "record this variant's result and decide whether the grid stops" had three implementations: the _VariantAborted handler, _record_round_stop's boolean that three call sites re-tested, and two inline rc != 0 and not keep_going_on_failure tails. This PR moves every ending onto the one _VariantAborted handler. The stop decision now travels with the exception as a _Halt enum (CONTINUE / ON_FAILURE / STOP), and the not-run rows for a batch-ending stop travel as trailing. The halt mapping is the same as on base at all five rewritten endings, and nothing an operator can see changes.
Blocking issues: 1
- [V5] The PR conflicts with main in scripts/code_metrics_baseline.txt [verified]
Problem: meta.txt reportsmergeable: CONFLICTING.git merge-tree --write-tree origin/main 9fc336eshows the only conflicted file is scripts/code_metrics_baseline.txt; _grid_runner.py and the test file merge cleanly.
Impact: The PR cannot merge until the baseline file is reconciled, and the baseline numbers this PR lowers were measured against a tree main has since moved past.
Action: Rebase onto main and regenerate therun_gridand module-length entries in scripts/code_metrics_baseline.txt from the rebased tree (python scripts/code_metrics.py --update-baseline).
SKIPPED: V4 -- "# PARTIAL: #1869 changes 336 files, only its first 100 were compared", and the same for #1675, #1276, #1154 and #1032. Overlap with those PRs was not checked.
SKIPPED: V2 -- pytest and ruff are not installed on the review host. Behaviour checks rely on the CI test shards and code-metrics, which are green at head 9fc336e.
Checked: _grid_runner.py (_Halt, _VariantAborted, _abort_on_round_stop, the three round-stop sites, the no-ws and invalid-report tails, the except handler, _variant_progress_note); an AST check that no abort raise sits inside a broad try body; test_grid_runner_behavior_lock.py (TestKeepGoingAsymmetry, TestVariantAbort); test_grid_runner.py (cancelled warmup tests); test_grid_runner_kill_and_branches_unit.py (AgentX preflight); main-side drift via git diff base..origin/main | Ran: git show/grep/diff, a Python AST walk; pytest SKIPPED | Base: 190604f | Head: 9fc336e
This branch has not been deployed
2 participants
Follow-up to review feedback on #1882 (finding 1,
mechanism-added-not-removed). The reviewer is right and the title of #1882 was not: unifying eleven pre-benchmark aborts left_record_round_stopand two inline rc-gated tails owning the same concern, so "record the result and decide whether the grid stops" had three implementations instead of one.All sixteen endings now raise
_VariantAborted, and the halt is data on it:_HaltCONTINUErc == 0ON_FAILUREkeep_going_on_failuredecidesrc != 0STOPtrailingcarries the rows for the variants it will not reachThe rc asymmetry that
TestKeepGoingAsymmetrydocuments - anrc == 0failure always continues, anrc != 0failure does not - was buried in twoif rc != 0 and not keep_going_on_failuretails. It is now the difference between two enum members._record_round_stopbecomes_abort_on_round_stopand only builds the result: it no longer appends toresults, no longer extends the grid with not-run rows, and no longer answers a question its caller then re-asked.Why not the requested
_run_variantextraction. I measured both ways it can be written, and both have a hard cost:async def, which is the only way it avoids plumbing: the gate'sfunction_line_findingsde-folds nested definitions - I probed it, a 103-line nested function reports as its ownouter.inner 103entry - so the extraction creates a new ~900-linefunction-linesfinding. CI refuses any baseline entry added relative to the base, so that needs a secondbaseline-raise. And ruff's mccabe folds nested definitions into the enclosing function, sorun_grid's cyclomatic complexity would not drop at all.def: roughly thirty parameters, which is the signature rule the extraction was meant to serve.So the extraction trades one oversized unit for another, needs a waiver, and moves no complexity number. This PR instead makes "one abort path" true at a cost of 111 changed lines and no re-indent. If you still want the extraction after the above, I will open it separately - but I would rather it be the split of
run_gridinto named phases than a single function holding the same body.Finding 2 (
exception-control-flow) is not addressed here and I have not argued it away: the threetry: ... except Exceptionblocks in the loop body are still there, and an abort added inside one of their bodies in future would be caught and re-recorded under the wrong class. Today every raise sits in a handler or at loop level - I re-ran the AST check and all sixteen are safe - but that is a property of the current code, not a guarantee. Closing it properly means removing the exception, which is the extraction above.Review feedback on #1882.
TestVariantAbortis kept, with the reviewer's two tidies: thesetenv/base-yaml setup moves into_run, and thec0marker assertion thatTestKeepGoingAsymmetryalready pins is dropped, leaving the assertion on the aborted variant's own marker.Run locally:
pytestovertest_grid_runner.py,test_grid_runner_behavior_lock.py,test_grid_runner_kill_and_branches_unit.py,test_conc_sweep.py- 322 passed; plustest_baseline_warmup_double_run.py,test_agentx_preflight.pyandsrc/hyperloom/orchestrator/actions/executors/tests- 910 passed, the 2 failures being a missing optionalTraceLensimport in the local environment that reproduces onmain.run_grid_grid_runner.pygoes from 2247 lines to 2246. No new unit above the ceiling:_Haltand the extended_VariantAbortedcarry no branches.run_gridis still far over and this PR does not claim otherwise - what it claims is that the sixteen endings now share one path.Removed: two of the three implementations of "record the result and decide whether the grid stops", the boolean
_record_round_stopreturned for its callers to re-test, and the two inline rc tails. Preserved: the same rows in the same order (the progress note readsresults[idx], so moving the trailing rows after the report changes nothing), the same abort markers, and the same decision at every ending - pinned byTestKeepGoingAsymmetry,TestVariantAbortand the kill/branches suite.None - refactor with nothing observable.
Breaking changes: no
PR addresses single concern: yes
Root cause is upstream: no