You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
scripts/run_benchmark_matrix.py is a 1296-line procedural module: 36 top-level
functions, 3 classes (two of which are exception types). Its tests can only
reach that logic by mutating sys.path so a script becomes importable. The
result is that unit-testing orchestration behavior is awkward, and one test
asserts against the Makefile by regexing its raw text. Extracting the domain
logic into classes — leaving the script as a thin CLI wrapper — would make the
existing 183 script tests target real seams instead of reaching into a script.
If we are going to test logic, I think that logic should be extracted to
class(es). We can write unit tests for those class(es), and then the script
can just be a thin wrapper over that logic.
Agreed in principle. Filing separately because it restructures the whole module
and invalidates the import lines in the existing test files, so it cannot be
scoped down to the one PR that surfaced it.
Current Behavior
The module is flat and procedural.DimensionSpec (:228) is the only
genuine data class; PreflightError (:137) and CellFailure (:141) are
exceptions. Everything else is a free function operating on dicts.
run_matrix (:897–1128) is a single 232-line function doing six distinct
jobs in sequence, with no seam between them:
unpack config, generate series combos, validate the cell count (:905–930)
the triple-nested cell loop — 115 lines of inline work at :980–1094:
resolve bindings, generate a workload file, generate a driver config, collect
env overrides, build the make command, spawn the subprocess, attach the
system monitor, validate that a record landed, classify the failure, record
the cell — then summary reporting at :1104–1122.
Tests reach the logic through a sys.path shim.scripts/tests/conftest.py
inserts scripts/ into sys.path "so we can import
generate_interactive_graphs and run_benchmark_matrix". Three suites then import
straight from the script:
test_matrix_config.py:5 — 6 symbols
test_matrix_failure_visibility.py:10 — 20 symbols, including run_matrix itself
test_engine_build.py:9 — 3 symbols
A test asserts on Makefile source text.TestMakefileContract
(test_engine_build.py:93) reads the Makefile and regexes it for ^<engine>-build: / ^<engine>-run-nobuild: rules, because there is no object
that owns the "which make target does this engine dispatch to" mapping.
There is no testing CI, so none of the above is enforced automatically —
tracked separately from this issue.
Proposed Behavior
Move the domain logic into importable classes and reduce run_benchmark_matrix.py to argparse plus construction and a single call.
Candidate seams, following the six phases already visible in run_matrix:
MatrixConfig — owns parsing and validation (parse_matrix_config, DimensionSpec, generate_series_combos, resolve_binding), and the cell
count check that currently raises PreflightError inline at :918.
EngineRegistry — owns DRIVER_ENGINE_MAP, detect_engine_for_driver, engines_for_combos, and the engine → make-target mapping. Gives TestMakefileContract a real object to assert against instead of a regex over
Makefile text.
CellRunner — one cell: generate the workload and driver config, build
the command, spawn, monitor, validate that a record landed, classify failures.
This is the 115-line loop body, and it is the piece most worth having under
direct test.
SweepRunner — drives iterations × x_values × combos over CellRunner,
owns results-dir/manifest/latest-link lifecycle and the summary.
Reporting (the banner and the closing summary) should be separable from
execution so tests do not have to capture stdout to assert on outcomes.
Exact class boundaries are for review — the goal is that each phase is
independently constructible and testable, not any specific decomposition.
Implementation Notes
Behavior-preserving. No change to CLI flags, exit codes (EXIT_OK / EXIT_CELL_FAILURES / EXIT_PREFLIGHT), manifest schema, ndjson layout, or
the latest symlink. Worth landing as a pure refactor with the 183 existing
tests green before and after.
Migrate the existing tests in the same PR. The import lines above all
break, test_matrix_failure_visibility.py most of all (20 symbols, and it
calls run_matrix directly). Those 61 tests encode the fail-loudly guarantees
from Matrix runner: fail loudly, isolate runs, report status honestly #2 and must keep passing — porting them, not rewriting them, is the
safest route.
Drop the sys.path shim only when nothing needs it.conftest.py's
insert also serves generate_interactive_graphs, so it stays until that
script is handled too.
generate_interactive_graphs.py is larger (78 KB vs 52 KB) and has the
same shape. Out of scope here; worth a follow-up if this lands well.
Sequence against in-flight work. This touches the whole module, so it will
conflict with anything else in run_benchmark_matrix.py — notably the open
Python engine work, which extends DRIVER_ENGINE_MAP (the table EngineRegistry would absorb). Land those first, or this one quickly.
Adding testing CI is a prerequisite for the payoff, not part of this
issue: a refactor this broad is much safer to review when the suite runs
automatically on the PR.
Summary
scripts/run_benchmark_matrix.pyis a 1296-line procedural module: 36 top-levelfunctions, 3 classes (two of which are exception types). Its tests can only
reach that logic by mutating
sys.pathso a script becomes importable. Theresult is that unit-testing orchestration behavior is awkward, and one test
asserts against the Makefile by regexing its raw text. Extracting the domain
logic into classes — leaving the script as a thin CLI wrapper — would make the
existing 183 script tests target real seams instead of reaching into a script.
Raised by @currantw in review of #7:
Agreed in principle. Filing separately because it restructures the whole module
and invalidates the import lines in the existing test files, so it cannot be
scoped down to the one PR that surfaced it.
Current Behavior
The module is flat and procedural.
DimensionSpec(:228) is the onlygenuine data class;
PreflightError(:137) andCellFailure(:141) areexceptions. Everything else is a free function operating on dicts.
run_matrix(:897–1128) is a single 232-line function doing six distinctjobs in sequence, with no seam between them:
:905–930):932–948)latestsymlink (:950–963):967):970):980–1094:resolve bindings, generate a workload file, generate a driver config, collect
env overrides, build the
makecommand, spawn the subprocess, attach thesystem monitor, validate that a record landed, classify the failure, record
the cell — then summary reporting at
:1104–1122.Tests reach the logic through a
sys.pathshim.scripts/tests/conftest.pyinserts
scripts/intosys.path"so we can importgenerate_interactive_graphs and run_benchmark_matrix". Three suites then import
straight from the script:
test_matrix_config.py:5— 6 symbolstest_matrix_failure_visibility.py:10— 20 symbols, includingrun_matrixitselftest_engine_build.py:9— 3 symbolsA test asserts on Makefile source text.
TestMakefileContract(
test_engine_build.py:93) reads theMakefileand regexes it for^<engine>-build:/^<engine>-run-nobuild:rules, because there is no objectthat owns the "which make target does this engine dispatch to" mapping.
There is no testing CI, so none of the above is enforced automatically —
tracked separately from this issue.
Proposed Behavior
Move the domain logic into importable classes and reduce
run_benchmark_matrix.pyto argparse plus construction and a single call.Candidate seams, following the six phases already visible in
run_matrix:MatrixConfig— owns parsing and validation (parse_matrix_config,DimensionSpec,generate_series_combos,resolve_binding), and the cellcount check that currently raises
PreflightErrorinline at:918.EngineRegistry— ownsDRIVER_ENGINE_MAP,detect_engine_for_driver,engines_for_combos, and the engine → make-target mapping. GivesTestMakefileContracta real object to assert against instead of a regex overMakefile text.
Preflight—resolve_cli_path,probe_server,preflight_server,driver_server_settings,unique_server_credentials,non_standalone_modes.CellRunner— one cell: generate the workload and driver config, buildthe command, spawn, monitor, validate that a record landed, classify failures.
This is the 115-line loop body, and it is the piece most worth having under
direct test.
SweepRunner— drives iterations × x_values × combos overCellRunner,owns results-dir/manifest/
latest-link lifecycle and the summary.Reporting (the banner and the closing summary) should be separable from
execution so tests do not have to capture stdout to assert on outcomes.
Exact class boundaries are for review — the goal is that each phase is
independently constructible and testable, not any specific decomposition.
Implementation Notes
EXIT_OK/EXIT_CELL_FAILURES/EXIT_PREFLIGHT), manifest schema, ndjson layout, orthe
latestsymlink. Worth landing as a pure refactor with the 183 existingtests green before and after.
break,
test_matrix_failure_visibility.pymost of all (20 symbols, and itcalls
run_matrixdirectly). Those 61 tests encode the fail-loudly guaranteesfrom Matrix runner: fail loudly, isolate runs, report status honestly #2 and must keep passing — porting them, not rewriting them, is the
safest route.
sys.pathshim only when nothing needs it.conftest.py'sinsert also serves
generate_interactive_graphs, so it stays until thatscript is handled too.
generate_interactive_graphs.pyis larger (78 KB vs 52 KB) and has thesame shape. Out of scope here; worth a follow-up if this lands well.
conflict with anything else in
run_benchmark_matrix.py— notably the openPython engine work, which extends
DRIVER_ENGINE_MAP(the tableEngineRegistrywould absorb). Land those first, or this one quickly.issue: a refactor this broad is much safer to review when the suite runs
automatically on the PR.