[gregorweiss]

Closes #9 (CPU scalability scope only — GPU scaling is out of scope for this PR).

Implements a CPU scalability benchmark for a prepared simulation directory. The question it answers is narrow and concrete: which is the highest CPU count for this system that is still justified by the 75% parallel-efficiency rule of thumb?

Scope

• CPU scalability only. GPU benchmarking was prototyped earlier on this branch and has been reverted in full (sweep axis, multi-rank srun -n R launch, -npme, gres scaling, GPU_ID rework). The mdrun app keeps only what the CPU sweep needs.
• Every trial is forced CPU-only (disable_gpu=True), so pointing the command at a GPU-capable executor config cannot silently produce a GPU curve labelled as CPU.

How it works

1. BenchmarkConfig.cpu_counts (default 1 2 4 8 16 32 64), duration_ps (default 100), efficiency_threshold (default 0.75).
2. One short-trial MDP is derived from the production MDP: nsteps set for duration_ps, output intervals stretched to the trial length so I/O does not distort throughput.
3. One allocation, sized to the largest swept count, forced exclusive, single worker, single block — so the only variable across trials is the mdrun thread count.
4. Each trial runs grompp → mdrun in .benchmark/cpus<N>; ns/day is extracted on the worker into a unique sidecar file (a driver-side log read can be served from a stale pre-backup inode).
5. BenchmarkResult derives everything from the trials: baseline (smallest successful count), efficiency(trial) (speedup ÷ core ratio), and max_justified (highest count still ≥ threshold). Persisted as benchmark_result.json.

Files

File	Change
mdfactory/performance/benchmark.py	CPU-only sweep, efficiency accounting, MDP derivation, trial runner
mdfactory/cli.py	mdfactory benchmark command (--cpus, --duration-ps, --efficiency, --dry-run)
mdfactory/orchestration/apps.py	performance_file sidecar + CPU thread pinning (-pin on)
docs/content/docs/user-guide/cli-reference.mdx	mdfactory benchmark reference section
tests	46 benchmark tests + 5 CLI tests; robustness tests for the sed extractor and pinning

[gregorweiss]

Implementation Plan

Problem

The pipeline uses hardcoded resource settings (num_cores: 8, num_gpus: 1). Different system sizes have different optimal hardware configurations. A short benchmark sweep discovers the throughput sweet spot before committing to a full campaign.

Deliverables

1. GROMACS md.log performance parser

Function that reads a GROMACS log file (any {deffnm}.log) and extracts ns/day from the performance summary block at the end. Handles: normal completion, truncated/incomplete logs (no performance block), restart logs with multiple performance blocks (use last).

2. BenchmarkConfig

Pydantic model specifying sweep parameters:

• cpu_counts: list[int] — core counts to sweep (e.g. [1, 2, 4, 8, 16])
• gpu_replicas: list[int] — GPU sharing replicas to sweep (e.g. [1, 2, 4])
• duration_ps: float — benchmark trial duration (default: 100 ps)
• selection: Literal["ns_per_day", "efficiency"] — optimize for raw throughput or throughput per core-hour
• Base executor config to derive sweep variants from

3. BenchmarkResult

Pydantic model for results:

• All trial results with config, ns/day, wall time
• Selected optimum based on selection criterion
• Serialization to/from JSON for persistence as campaign sidecar

4. Short benchmark MDP generator

Reuse mdp.py utilities (parse_mdp, modify_mdp_value, write_mdp) to take an existing production MDP and override nsteps for a short trial (~100 ps), with minimal output frequency to reduce I/O overhead.

5. Sweep runner

run_benchmark_sweep(system_path, config) -> BenchmarkResult:

• Generate one ExecutorConfig (or SlurmExecutorConfig) variant per sweep point, varying cpus_per_node / gres / max_workers_per_node
• For each point: open a sequential parsl_session, run a short mdrun trial using get_grompp_app() / get_mdrun_app(), parse the resulting log
• Collect all trials, select optimum

6. Dry-run mode

Show what configs would be tested without submitting any work — follow the existing dry_run pattern from run_simulations and build_systems.

7. JSON persistence

Save BenchmarkResult as benchmark_result.json sidecar in the campaign directory.

Files to create or modify

File	Action	What
mdfactory/performance/benchmark.py	Create	Parser, config, result, MDP generator, sweep runner
mdfactory/performance/__init__.py	Modify	Re-export BenchmarkConfig, BenchmarkResult, run_benchmark_sweep
mdfactory/tests/test_benchmark.py	Create	All tests

Testing approach

File: mdfactory/tests/test_benchmark.py

1. Parser tests — synthetic GROMACS log fixtures (embedded strings) covering: normal performance block, truncated log, multiple performance blocks (restart), edge cases
2. BenchmarkConfig validation — empty sweep lists, invalid selection criteria
3. MDP generation — verify nsteps override, output frequency reduction
4. Sweep runner (mocked) — mock parsl_session and apps, verify correct number of configs generated, correct config parameters per sweep point
5. Dry-run — verify no Parsl loaded, correct plan output
6. Result persistence — JSON round-trip serialization
7. Result selection — verify optimum selection for both ns/day and efficiency criteria

Integration points

• performance/cluster.py: discover_cluster() / NodeType for auto-discovering sweep grid from available hardware
• orchestration/config.py: ExecutorConfig / SlurmExecutorConfig — one variant per sweep point
• orchestration/session.py: parsl_session — sequential sessions, one per sweep point
• orchestration/apps.py: get_grompp_app() / get_mdrun_app() for running trials
• orchestration/stages.py: StageSpec / ResourceHints / _extract_resource_hints for stage conventions
• orchestration/mdp.py: parse_mdp / modify_mdp_value / write_mdp for short-trial MDP generation

Risks and open questions

1. GROMACS log format stability — the performance block format may vary across GROMACS versions. Parser should be tested against representative samples.
2. Sweep execution time — sequential Parsl sessions mean sweep time is O(n) in number of sweep points. Acceptable for short trials (~100 ps each).
3. Issue mentions dependency on issues 7/8 (cluster autodiscovery) — cluster.py already exists with full autodiscovery. Dependency is met.

---

Plan created by mach6

[gregorweiss]

Progress Update

Implemented the full benchmark sweep module:

New files

• mdfactory/performance/benchmark.py (486 lines) — complete module with:

  • parse_mdlog_performance() — GROMACS md.log parser extracting ns/day from performance block (handles normal, restart, truncated logs)
  • BenchmarkConfig — Pydantic model for sweep parameters (cpu_counts, gpu_replicas, duration_ps, selection criterion, MDP overrides)
  • TrialResult / BenchmarkResult — result models with optimum selection (ns/day or efficiency) and JSON persistence
  • _generate_benchmark_mdp() — creates short-duration MDP from production MDP, reusing orchestration/mdp.py utilities
  • run_benchmark_sweep() — sequential Parsl sessions per sweep point with dry-run mode

• mdfactory/tests/test_benchmark.py (370 lines) — 24 tests covering:

  • Parser: normal log, restart (multiple blocks), truncated, missing, empty
  • Config validation: defaults, empty cpu_counts, negative duration
  • Trial result: efficiency calculation, optimum selection (both criteria)
  • MDP generation: nsteps override, I/O reduction, extra overrides, dt override
  • Sweep config: CPU-only and CPU×GPU cross-product
  • Dry-run: no Parsl loaded, correct trial count
  • JSON persistence: save/load round-trip, field completeness

Modified files

• mdfactory/performance/__init__.py — re-exports BenchmarkConfig, BenchmarkResult, run_benchmark_sweep

Integration points

• Reuses parsl_session for sequential trial execution
• Reuses get_grompp_app() / get_mdrun_app() for GROMACS invocation
• Reuses mdp.py for benchmark MDP generation
• Reuses find_structure_file() and _extract_resource_hints() from orchestration

All 24 tests pass, lint clean.

Commit: a658cf1

---

Progress tracked by mach6

[gregorweiss]

Code Review

Critical

Finding 1 (code-reviewer, critical): grompp_app called with wrong keyword argument names. Uses structure_file, topology_file, tpr_output but the actual run_grompp function expects gro_file, top_file, tpr_file. Every non-dry-run execution raises TypeError. (benchmark.py lines ~356-362)

Important

Finding 2 (code-reviewer, medium): IO-reduction loop enables outputs that were intentionally disabled. nstfout = 0 (never write forces) gets overridden to 50000, turning ON force output. Should skip keys already set to "0". (benchmark.py lines 203-208)

Finding 3 (error-auditor, high): CPU sweep is silently a no-op for ExecutorConfig (local provider). cpus_per_node is not a field on ExecutorConfig — only on SlurmExecutorConfig. Pydantic model_copy(update=...) silently discards unknown keys, so all sweep points use identical configs. (benchmark.py lines 260-280, 344)

Finding 4 (error-auditor, high): Broken symlink causes FileExistsError outside the per-trial try/except, aborting the entire sweep. Path.exists() follows symlinks — returns False for broken symlinks — but the symlink entry still exists, causing symlink_to() to fail. (benchmark.py lines 353-358)

Finding 5 (test-reviewer, high): Real execution path (dry_run=False) has zero test coverage. The ~110-line live path (trial dirs, symlinks, Parsl sessions, grompp/mdrun, log parsing) is never exercised with mocks.

Finding 6 (test-reviewer, high): Error paths untested — missing production MDP, missing structure file, and trial-level exceptions are never exercised.

Suggestions

Finding 7 (code-reviewer, medium): _extract_resource_hints imported as a private symbol across module boundary. Consider promoting to public API.

Finding 8 (test-reviewer, medium): test_extra_overrides_applied assertion assert "0" in content is vacuously true — "0" appears in nearly every MDP file.

Finding 9 (code-reviewer, low): TrialResult.efficiency docstring says "per core-hour" but computes ns/day per core (divides by cpu_count only).

Finding 10 (test-reviewer, low): cpu_count=0 efficiency branch untested.

Strengths

• Clean separation: parser, models, MDP generator, and sweep runner are well-decomposed
• Reuses existing infrastructure (mdp.py, parsl_session, stages, apps) rather than reinventing
• Dry-run follows established project patterns
• JSON persistence with Pydantic model validation
• Good parser test coverage (normal, restart, truncated, missing, empty)

Agents run: code-reviewer (4 findings), error-auditor (2 findings), test-reviewer (4 findings), completeness-checker (failed, done manually — all scope items delivered)

---

Reviewed by mach6

[gregorweiss]

Review Assessment

Review comment

Classifications

Finding	Classification	Reasoning
1: Wrong grompp kwarg names	Genuine	Factual: Verified — run_grompp declares gro_file, top_file, tpr_file (apps.py L486-488); benchmark passes structure_file, topology_file, tpr_output. Raises TypeError on every non-dry-run call. Scope: Parser and sweep must work correctly per acceptance criteria.
2: IO loop enables disabled outputs	Genuine	Factual: Verified — nstfout = 0 means "never write forces"; overriding to 50000 turns it ON, increasing I/O. Scope: Benchmark MDP should minimize I/O per the plan. Correctness issue in delivered code.
3: CPU sweep no-op for local provider	False positive	Factual: Tested — model_copy(update={"cpus_per_node": 16}) on ExecutorConfig DOES set the attribute via __dict__. _extract_resource_hints uses getattr() which finds it. ntasks=16 is correctly passed. The sweep works for both providers.
4: Broken symlink FileExistsError	Genuine	Factual: Verified — Path.exists() returns False for broken symlinks but symlink_to() raises FileExistsError because the entry exists. This is outside the per-trial try/except, aborting the entire sweep. Scope: Sweep must handle re-runs safely.
5: No test coverage for execution path	Genuine	Factual: Confirmed — no test exercises dry_run=False. Scope: PR adds new testable code; tests must ship with the feature.
6: Error paths untested	Genuine	Factual: Confirmed — missing MDP, missing structure, trial failure never exercised. Scope: New testable error paths must be covered.
7: Private symbol import	Deferred	Factual: True — _extract_resource_hints has a leading underscore. Scope: Convention issue, not a correctness or safety requirement for the benchmark to work. Can be addressed in a follow-up.
8: Vacuous test assertion	Genuine	Factual: "0" appears in SAMPLE_MDP as nstfout = 0, so assert "0" in content always passes. Scope: Test quality — the assertion does not verify the feature it claims to test.
9: Efficiency docstring inaccuracy	Nitpick	Factual: True — says "core-hour" but computes "per core". Scope: Docstring wording, doesn't affect behavior.
10: cpu_count=0 branch untested	Deferred	Factual: True. Scope: cpu_count=0 is not a realistic sweep input. Edge case in a guard clause, not a regression risk.

Action Plan

1. Fix grompp kwarg names (finding 1) — structure_file → gro_file, topology_file → top_file, tpr_output → tpr_file
2. Fix IO reduction to preserve disabled outputs (finding 2) — skip keys with value "0"
3. Fix broken symlink handling (finding 4) — either unlink stale symlinks or move symlink creation inside the per-trial try/except
4. Add mocked execution path test (finding 5) — mock parsl_session, apps, verify correct invocations
5. Add error path tests (finding 6) — missing MDP, missing structure, trial failure
6. Fix vacuous test assertion (finding 8) — assert the actual key-value pair, not bare "0"

6 genuine / 1 nitpick / 1 false positive / 2 deferred

---

Assessment by mach6

[gregorweiss]

Progress Update — Review Fixes

All 10 review findings addressed (6 genuine + 2 deferred + 1 nitpick + 1 false positive):

Code fixes (6 files, +220/-50 lines)

• Finding 1 (critical): Fixed grompp kwarg names — structure_file → gro_file, topology_file → top_file, tpr_output → tpr_file
• Finding 2: IO reduction loop now preserves disabled outputs (value "0") instead of overriding them
• Finding 4: Moved symlink creation inside per-trial try/except; stale symlinks are removed before re-creating
• Finding 7: Promoted _extract_resource_hints → extract_resource_hints (public API) across 4 files
• Finding 9: Fixed efficiency docstring: "per core (ns/day / cpu_count)" not "per core-hour"

Test additions (+6 tests, 30 total)

• Finding 5: test_mocked_sweep_runs_all_points — mocked execution path verifying Parsl sessions, grompp/mdrun invocations, result shape
• Finding 6: test_failed_trial_continues_sweep — failed trial records error, sweep continues; test_missing_production_mdp and test_missing_structure_file — error paths
• Finding 8: Fixed vacuous assertion — uses unique value "99999" instead of "0"
• Finding 10: test_efficiency_none_when_zero_cpus — covers cpu_count=0 guard
• Bonus: test_disabled_outputs_preserved — verifies IO fix preserves nstfout=0

Finding 3 (false positive)

Verified that model_copy(update={"cpus_per_node": N}) on ExecutorConfig does set the attribute via __dict__, and extract_resource_hints picks it up via getattr(). CPU sweep works correctly for both local and Slurm providers.

Commit: 9a1eb4d

---

Progress tracked by mach6

[gregorweiss]

Code Review (Re-Review)

All 10 original findings confirmed fixed correctly by code-reviewer. No critical or high-severity issues found.

Suggestions

Finding 1 (error-auditor, medium): result.save() outside try/except — if disk write fails after a multi-minute sweep, the in-memory BenchmarkResult is lost because the exception propagates before return result. Consider returning first or wrapping save in try/except with a warning. (benchmark.py lines ~448-458)

Finding 2 (error-auditor, medium): mdrun exits zero but parse_mdlog_performance returns None — trial is recorded with ns_per_day=None, error=None, indistinguishable from success in the JSON. If all trials hit this, optimum=None is returned silently. Should set error="no performance data" and log a warning when ns_per_day is None after nominally successful mdrun. (benchmark.py lines ~405-420)

Finding 3 (error-auditor, low): fh.seek(size - read_bytes) in text mode is implementation-defined per Python docs. Works on CPython + ASCII (GROMACS logs are ASCII), but open(log_path, "rb") would be spec-compliant. (benchmark.py lines 56-61)

Finding 4 (test-reviewer, low): io_interval floor-cap (max(nsteps, 1000)) behavior not tested — when duration_ps < 2.0, nsteps < 1000 and IO interval exceeds nsteps. No test exercises this branch. (benchmark.py line 204)

Finding 5 (test-reviewer, low): test_io_frequency_reduced counts raw "50000" occurrences including nsteps itself. Per-key get_mdp_value checks would be more precise. (test_benchmark.py line ~235)

Finding 6 (test-reviewer, low): JSON round-trip not tested with optimum=None (all-failed or dry-run scenario).

Strengths

• All 10 original findings correctly resolved
• Symlink fix is thorough: handles broken, stale, and fresh cases
• Mock patch targets correctly reference source modules for deferred imports
• __exit__ = MagicMock(return_value=False) correctly prevents exception suppression
• Clean module decomposition: parser, models, MDP generator, sweep runner
• 30 tests with good coverage of happy paths and error paths

Agents run: code-reviewer (0 new findings), error-auditor (3 findings), test-reviewer (3 findings), completeness-checker (failed, done manually — all criteria met), simplifier (failed)

---

Reviewed by mach6

[gregorweiss]

Review Assessment (Re-Review)

Review comment

Classifications

Finding	Classification	Reasoning
1: result.save() outside try/except	Deferred	Factual: True — disk error loses in-memory result. Scope: Acceptance criteria don't require fault-tolerant persistence. Edge case (disk full). Low-priority hardening.
2: ns_per_day=None with error=None	Genuine	Factual: True — successful mdrun with unparseable log produces ns_per_day=None, error=None, indistinguishable from success in JSON. Scope: Result JSON must contain meaningful trial data per acceptance criteria. Silent data loss in the result undermines the sweep's utility.
3: text-mode fh.seek(byte_offset)	Deferred	Factual: Technically true per Python docs. Scope: GROMACS logs are pure ASCII; no multibyte sequences. Works correctly on all platforms in practice. Defensive hardening, not a correctness issue.
4: io_interval floor-cap untested	Nitpick	Factual: True — no test for duration_ps < 2.0. Scope: The floor cap is intentional (prevent nsteps=0 edge); GROMACS handles IO interval > nsteps gracefully. Not a coverage gap for the acceptance criteria.
5: test_io_frequency_reduced count assertion	Nitpick	Factual: True — count includes nsteps line. Scope: test_disabled_outputs_preserved already verifies per-key behavior. The count assertion is imprecise but not wrong.
6: JSON round-trip with optimum=None	Nitpick	Factual: True — not tested. Scope: Pydantic handles None serialization correctly; this is a standard framework behavior, not custom logic.

Action Plan

1. Fix finding 2 — set error="no performance data" and log a warning when ns_per_day is None after successful mdrun

1 genuine / 3 nitpicks / 0 false positives / 2 deferred

---

Assessment by mach6

[gregorweiss]

Progress Update — Re-Review Fix

Fixed the one genuine finding from re-review:

• Finding 2: When mdrun succeeds but log has no performance block, trial now records error="no performance data in {log_file}" and logs a warning. Previously ns_per_day=None, error=None was indistinguishable from success in the JSON.
• Added test_missing_performance_data_sets_error (31 tests total)

Commit: f02030d

---

Progress tracked by mach6

[gregorweiss]

Progress Update

Added a first-class mdfactory benchmark CLI command and fixed two bugs found while running the sweep end-to-end on SLURM.

CLI

• mdfactory benchmark SOURCE --slurm <cfg> [--cpus ...] [--gpus ...] [--duration-ps ...] [--selection ns_per_day|efficiency] [--dry-run]
• --slurm is required, so the command cannot accidentally run trials on the login node.

Fixes

• The sweep called the grompp/mdrun apps without inputs, so Parsl iterated None and every trial died with TypeError: 'NoneType' object is not iterable. Now passes explicit input lists, matching the simulate path.
• Trial directories only staged md.mdp, the structure, and topology.top, so GROMACS aborted with Topology include file "extra_params.itp" not found. Now stages all top-level inputs, including companion .itp files and force-field directories.
• Trial failures now log the full traceback instead of swallowing the cause.

Tests

• Regression assertions for both bugs plus a new CLI test module. The benchmark/CLI tests pass and Ruff is clean.

Verified end-to-end on SLURM (build, then EM/NVT/NPT, then sweep) for a 1000-water/100-ethanol mixedbox:

• cpus=1: 146.4 ns/day
• cpus=2: 283.0 ns/day
• cpus=4: 392.0 ns/day
• cpus=8: 957.6 ns/day (selected optimum)

Commit: cdf2141

---

Progress tracked by mach6

[gregorweiss]

Progress Update

Hardened the benchmark sweep after the end-to-end run surfaced several issues.

Sweep execution

• Runs all sweep points inside a single Parsl allocation sized to the largest requested CPU count (varying only mdrun thread count), instead of one allocation per point. Rejects multi-node allocations up front.

Thread pinning

• CPU mdrun now passes -pin on (explicit gmx/gmx_mpi flags and both auto-detect branches). Previously GROMACS disabled its own affinity because Slurm confines the job to a cpuset, leaving threads free to migrate/span NUMA nodes and producing erratic 2x swings.

Result extraction (stale-read fix)

• The mdrun script extracts ns/day on the node that wrote the log and writes it to a per-trial UUID-named sidecar (bench.<hex>.perf, performance_file= app kwarg); the driver reads that file via read_performance_file().
• This removes two failure modes: driver-side reads over a shared filesystem returning a previous run's #bench.log.N# inode, and Parsl @bash_app.result() returning the process return code rather than stdout (so a stdout marker silently yielded None).

Concurrency guard

• run_benchmark_sweep now holds an exclusive flock on .benchmark/.lock. A second concurrent sweep on the same system fails fast instead of racing on the shared cpus* trial directories; the lock self-releases if the holder dies.

Validation

• Clean single sweep now yields a reproducible curve (peak at 8 threads for the test system): 1 -> 149.1, 2 -> 282.7, 4 -> 537.1, 8 -> 956.6, 16 -> 862.0, 32 -> 749.6, 64 -> 557.4 ns/day.
• Unit tests updated/added for all four points; targeted suites pass and ruff is clean.

Commit: e96804d

---

Progress tracked by mach6

[gregorweiss]

Code Review

Scope: issue 9 plus the mach6-plan comment. Prior review rounds fixed the earlier findings; this round targets the current head (e96804d) and confirms no regressions.

Critical

None.

Important

Finding 1 (code-reviewer + error-auditor, high): CPU-only sweep points derive mdrun thread count from the base config's max_workers_per_node. _build_sweep_configs sets only cpus_per_node, then extract_resource_hints computes ntasks = cpus_per_node // max_workers_per_node. With a base config such as max_workers_per_node=4, the sweep labels trials cpus=1,2,4,8,... but actually runs 0/0/1/2 threads (0 falls back to auto-detect), while the allocation is separately normalized to max_workers_per_node=1. The scaling curve and optimum are silently wrong.

Finding 2 (error-auditor, high): The benchmark MDP is generated before _sweep_lock is taken. A second sweep overwrites the shared benchmark.mdp (which the first sweep's trial dirs expose as md.mdp) and only then trips the lock — silently changing the running sweep's trials to the other run's duration/overrides, with no error recorded.

Finding 3 (completeness-checker, high): The GPU sweep axis never launches concurrent replicas. Points enumerate gpu_replicas, but each point runs one grompp plus one blocking mdrun, so no aggregate throughput for N replicas sharing a GPU is ever measured.

Finding 4 (test-reviewer, high): The production-MDP → benchmark-MDP replacement inside each trial dir is not asserted. A regression there would silently run the full-length production MDP for every trial and all tests would still pass.

Suggestions

Finding 5 (error-auditor, medium): mdfactory benchmark exits 0 with no artifact when a summary YAML resolves to zero simulations; it lacks the "No simulations found." guard that simulate has.

Finding 6 (code-reviewer, medium): -pin on differs between explicit gmx_binary="gmx_mpi" and auto-detected gmx_mpi for GPU runs. The resolver pins, the auto preamble does not; the _build_binary_detection_preamble docstring is now stale.

Finding 7 (code-reviewer, medium): run_benchmark_sweep does Path(system_path) rather than .resolve(), unlike the rest of the orchestration code, so a relative path diverges between driver and worker.

Finding 8 (completeness-checker, medium): dry-run logs descriptors only; it does not emit the srun/mdrun command text the issue acceptance criteria mention. Note the plan comment softens this wording.

Finding 9 (completeness-checker, medium): "Reuse across HT batch" (benchmark a representative system, apply to the campaign) is not implemented. The plan comment omits this section.

Finding 10 (test-reviewer, medium): no test asserts the allocation's max_workers_per_node, and no live sweep with non-empty gpu_replicas exercises the thread-division path.

Finding 11 (test-reviewer, medium): the CLI test never asserts dry_run is forwarded, so --dry-run could silently submit real work.

Finding 12 (test-reviewer, medium): no test verifies run_benchmark_sweep actually enters _sweep_lock.

Finding 13 (code-reviewer, low): five tests were renamed to testextract_resource_hints_*, dropping the test_ prefix separator.

Finding 14 (code-reviewer, low): cpu_counts validation accepts zero and negative values.

Finding 15 (simplifier, low): the base_config parameter of _build_sweep_configs is unused.

Finding 16 (simplifier, low): duplicated GPU/CPU points.append branches in _build_sweep_configs.

Finding 17 (simplifier, low): redundant read_bytes intermediate in parse_mdlog_performance.

Finding 18 (simplifier, low): redundant default=0 in the max_replicas computation.

Strengths

• Parser coverage is thorough (normal/restart/truncated/missing/empty), and the new marker/sidecar helpers are well tested.
• Worker-side ns/day extraction into a uniquely named sidecar is a solid fix for the shared-filesystem staleness.
• set -euo pipefail and empty-sidecar handling degrade to an explicit recorded error, not a silent None.
• Symlink staging correctly handles broken and stale symlinks; -pin on is verified for both CPU build branches and both auto branches.
• No regressions from the prior review rounds.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[gregorweiss]

Independent Assessment

Each review finding was re-verified against the current code (head e96804d) by an independent agent, applying a factual gate and a scope gate. Authoritative scope is issue 9 plus the mach6-plan comment.

Classifications

Finding	Classification	Why
1 — CPU sweep derives ntasks from base max_workers_per_node	genuine	Confirmed by execution: with base max_workers_per_node=4, labels cpus=1,2,4,8 yield ntasks 0,0,1,2, so the recorded cpu_count does not match the threads mdrun uses. The CPU sweep is the core deliverable, and a mislabeled thread count makes its output meaningless.
2 — benchmark MDP generated before _sweep_lock	genuine	Confirmed: the MDP write precedes the lock, and trial dirs expose the shared benchmark.mdp as md.mdp, so a second sweep mutates a running sweep's trials before it trips the lock. The lock was added by this PR to protect exactly this shared state; its protection is defeated for the MDP.
3 — GPU axis never launches concurrent replicas	genuine	Confirmed: exactly one grompp + one blocking mdrun per point; gpu_replicas only divides threads. The issue and plan both specify varying concurrent replicas sharing one GPU, so the axis as built does not measure what it advertises.
4 — trial-dir MDP replacement unasserted	genuine	Confirmed test gap for new plan-mandated behavior. A regression would run the full production MDP for every trial with all tests passing.
10 — allocation max_workers_per_node and live GPU sweep untested	genuine	Confirmed gap on plan items 2 and 5; it would not have caught finding 1.
11 — CLI test does not assert dry_run forwarded	genuine	Confirmed gap on an authorized deliverable; a regression would make --dry-run submit real work.
5 — CLI no "no simulations" guard	deferred	Real but not required by issue 9 or the plan (the CLI is an addition beyond the plan's file table). Optional robustness follow-up.
7 — Path(system_path) not resolved	deferred	Real only for direct API callers with relative paths; the CLI resolves before calling. Optional hardening.
8 — dry-run emits no command text	deferred	Plan item 6 defines dry-run as showing which configs would run, not emitting srun text. Not required.
9 — "Reuse across HT batch" missing	deferred	Explicitly absent from the approved plan; review evidence cannot expand scope.
12 — no test that the sweep enters _sweep_lock	deferred	The lock is optional concurrency hardening not in issue 9 or the plan.
14 — cpu_counts accepts zero/negative	deferred	Real but outside the plan's stated validation; optional follow-up.
6 — -pin on differs explicit vs auto gmx_mpi on GPU	nitpick	No demonstrated correctness impact; minor inconsistency plus a stale docstring introduced here.
13 — tests named testextract_resource_hints_*	nitpick	Verified all five are still collected; pure naming.
15 — unused base_config param	nitpick	Dead parameter, no behavioral effect.
16 — duplicated points.append branches	nitpick	Readability only.
17 — read_bytes intermediate	nitpick	The value is used twice, so "redundant" is inaccurate; stylistic.
18 — redundant default=0	nitpick	Harmless.

Action Plan (genuine issues only, in priority order)

1. Finding 1 — make each CPU trial's thread count equal its labeled cpu_count regardless of the base max_workers_per_node (normalize CPU points to max_workers_per_node=1, or pass the thread count explicitly instead of letting extract_resource_hints divide).
2. Finding 3 — implement the GPU replica axis (launch N concurrent mdrun replicas sharing the GPU and measure aggregate throughput) or stop advertising an axis that only divides threads.
3. Finding 2 — move benchmark-MDP generation inside _sweep_lock.
4. Finding 4 — add a regression test asserting the trial's md.mdp resolves to benchmark.mdp.
5. Finding 11 — add a CLI test asserting dry_run is forwarded.
6. Finding 10 — add execution-path assertions for the allocation's max_workers_per_node and a live GPU-replica sweep test.

Deferred (optional, non-blocking): findings 5, 7, 8, 9, 12, 14. Nitpicks: 6, 13, 15, 16, 17, 18.

---

Assessed by mach6

[gregorweiss]

Progress Update

Fixed all six findings classified as genuine in the independent assessment.

Sweep execution (mdfactory/performance/benchmark.py)

• Finding 1 — CPU-only sweep points now set max_workers_per_node: 1 explicitly, so the mdrun thread count always equals the labeled cpu_count regardless of the base config's max_workers_per_node (previously a base value of 4 produced ntasks 0, 0, 1, 2 for labels 1, 2, 4, 8, silently corrupting the scaling curve).
• Finding 3 — the GPU axis now actually launches concurrent replicas: a point with R replicas submits all R mdruns before joining any, each with cpus // R threads, distinct deffnms (bench_r0…), per-replica TPR symlinks to the single shared grompp TPR, and unique perf sidecars. The recorded ns_per_day is the aggregate sum across replicas; docstrings updated to advertise this.
• Finding 2 — _sweep_lock is now acquired before benchmark-MDP generation (it wraps the MDP write, structure check, and Parsl session), so a losing concurrent sweep fails fast without overwriting the running sweep's shared benchmark.mdp.

Tests

• Finding 4 — regression test asserting each trial's md.mdp resolves to benchmark.mdp with the short-trial nsteps (50000, not the production 500000), and that grompp consumes it.
• Finding 11 — CLI tests now assert dry_run is forwarded to run_benchmark_sweep, for both True and False.
• Finding 10 — allocation assertions (max_workers_per_node == 1 for the CPU sweep, == 2 for the GPU sweep) plus a live GPU-replica sweep test verifying submission-before-join ordering (concurrency), thread division (4 cpus / 2 replicas → ntasks [2, 2]), and aggregate throughput (5.234 + 5.234 = 10.468 ns/day).

Verification

• ruff check: clean.
• pytest -k "not build" mdfactory: 2996 passed, 4 skipped. The single failure (test_settings.py::test_defaults_loaded) reproduces on clean head and is a pre-existing local-environment issue unrelated to this change.

Deferred (non-blocking) and nitpick findings were left untouched, per the assessment's classifications.

Commit: 25d9c23

---

Progress tracked by mach6

[gregorweiss]

Progress Update

The benchmark sweep now requests an exclusive whole-node allocation, so the measurement isn't polluted by neighbouring jobs.

Why: two end-to-end runs of the identical CPU sweep on the shared 128-core partition disagreed by 37% at 8 cores (956 → 600 ns/day) and 12% at 16 cores, which flipped the selected optimum from 8 → 16 cores. Under 4 cores the runs reproduced to three decimals — the harness itself is deterministic; only the ≥8-core points were exposed to neighbour contention.

Change (mdfactory/performance/benchmark.py)

• The single sweep allocation is built with exclusive=True whenever the config carries the field (i.e. SLURM configs), translating to #SBATCH --exclusive.
• Production/simulate runs are untouched — they keep the default shared-node behaviour from the earlier "disable exclusive by default" fix; only the benchmark's own allocation opts back in.
• Applied via model_copy(update=...), so the caller's base config is not mutated.
• The Single allocation: log line now reports exclusive=....

Tests

• New test_sweep_allocation_is_exclusive: a SlurmExecutorConfig(exclusive=False) must still yield an exclusive sweep allocation, while the base config stays False.

Verification: ruff check + ruff format --check clean; 102 tests pass across test_benchmark.py, test_orchestration_cli_benchmark.py, test_orchestration_config.py.

Commit: af696dd

---

Progress tracked by mach6

[gregorweiss]

Code Review

Scope: issue 9 plus the mach6-plan comment, reviewed at head af696dd (prior head e96804d plus two fix commits 25d9c23, af696dd). The human-approved GPU-axis rework (multi-GPU single-sim; MPS → issue 10) is treated as known-pending, not a gap. Full short suite passes (2997 passed; lone test_settings failure is a pre-existing local-config.toml issue), ruff check/format clean.

Critical

None.

Important

Finding 1 (error-auditor + code-reviewer, high, conf 90): GPU-replica points where cpu_count < gpu_replicas compute ntasks=0, which is the auto-detect sentinel. extract_resource_hints floors cpus_per_node // max_workers to 0, so each replica silently falls back to ${SLURM_CPUS_PER_TASK:-…:-$(nproc)} — the allocation's core count, not the labeled cpu_count // R. Reachable with stock --gpus 2 against default cpu_counts=[1,2,4,8] (points (1,2),(1,4),(2,4) all hit it) and worse against issue 9's full axes (10 generated points get ntasks=0). The oversubscribed, mislabeled trial is recorded as valid and can win _select_optimum, poisoning the reported optimum. The code comment documents intent the code doesn't deliver. The 25d9c23 fix normalized only the CPU-only branch; the GPU branch is unguarded.

Finding 2 (completeness-checker, high, conf 100): The issue's "Reuse across HT batch" section is entirely absent. None of its three behaviors exist — no ±20% atom-count grouping, no representative-system selection, no mechanism applying benchmark_result.json's optimum to production runs. The only consumers of the sidecar are its writer and tests; the CLI does the opposite, benchmarking every campaign sim dir independently (cli.py:760-763).

Finding 3 (test-reviewer, high, conf 90): The worker-side sed performance extraction (_PERF_EXTRACT) is never executed against a realistic log. The test only asserts substring presence. Mutation-verified: corrupting the sed pattern (Performance: → Throughput:) leaves all tests green. This program is the sole producer of real ns/day in the live path, so a regex regression breaks every production trial with "no performance data" while CI stays green.

Finding 4 (code-reviewer, medium, conf 85): mdp_overrides are silently dropped when the key is absent from the source MDP. modify_mdp_value only replaces existing lines — it never appends or warns. The documented example {"nstxout-compressed": "0"} is a no-op on any md.mdp lacking that key, so GROMACS' default writes a full trajectory during every trial — exactly the I/O the benchmark MDP generator exists to remove — with no log line to catch it.

Finding 5 (error-auditor, medium, conf 85): --gpus N with a SLURM YAML lacking gres runs CPU-only trials (disable_gpu=True) that are labeled and reported as GPU-replica points. GPU enablement derives solely from the base config's gres; nothing cross-checks that a GPU sweep actually has a GPU, so a misconfigured invocation silently produces mislabeled data that feeds _select_optimum.

Finding 6 (error-auditor, medium, conf 83): mdfactory benchmark never validates the executor is SLURM. The docstring says SLURM is required, but _load_executor_config accepts any YAML and the only structural check is nodes > 1. A provider: local file runs the whole sweep on the submitting (login) node and reports the result as a cluster measurement, with the provider never logged.

Finding 7 (completeness-checker, medium, conf 85): No per-configuration srun commands — one shared allocation replaces per-point sessions. Deviates from acceptance criterion (b) and plan deliverable 5's "per-point parsl_session". Deliberate and justified (contention rationale), with a test asserting call_count == 1, but not human-approved as a scope change (only the GPU rework was). Read functionally ("each config runs with correct resource params") it's met; read literally it's partial.

Finding 8 (test-reviewer, medium, conf 95): Hyphenated mdp_overrides keys — the documented form — are untested, and the normalization is load-bearing. Mutation-verified: deleting replace("-", "_") leaves all tests passing; since modify_mdp_value never appends, an un-normalized hyphen key matches nothing and the override is dropped. Compounds Finding 4.

Finding 9 (test-reviewer, medium, conf 90): Dry-run isn't pinned as side-effect-free. Mutation-verified: making dry-run save benchmark_result.json before returning (clobbering a real sweep's results with all-null trials) leaves all tests passing. Also unasserted: cpu_count labels, no .benchmark/ dir or lock created, and the docstring's "does not import Parsl" claim has no backing assertion.

Finding 10 (test-reviewer, medium, conf 90): CLI per-trial reporting (N/A + error annotation) has no assertion. Mutation-verified: deleting the entire per-trial loop from _report_benchmark_result — so users never see failed trials or their errors — leaves all 4 CLI tests passing.

Finding 11 (simplifier, medium, conf 88): The same ~10-line mock scaffolding is repeated across all seven execution tests (test_benchmark.py:519-846). A shared fixture would remove ~60 duplicated lines while preserving every assertion; the repo already uses this fixture pattern elsewhere.

Suggestions

Finding 12 (error-auditor, low, conf 85): If every trial fails, the CLI still exits 0 (_report_benchmark_result only warns), so mdfactory benchmark … && … can't detect total failure. Suggest sys.exit(1) when optimum is None.

Finding 13 (code-reviewer, low, conf 90): -pin on differs between explicit gmx_binary="gmx_mpi" (-ntomp $NTHR -pin on) and auto-detection (-ntomp $NTHR, no pin) for the same binary+GPU mode, so a benchmark comparing gmx_binary settings also compares pinning regimes.

Finding 14 (code-reviewer + test-reviewer, low, conf 95/100): Rename sed artifact — five tests are testextract_resource_hints_* (underscore eaten). They still collect, but the names are mangled and drop out under a stricter test_* pattern.

Finding 15 (code-reviewer, low, conf 95): _build_sweep_configs takes a base_config it never uses — misleading for the upcoming GPU rework, which will actually need base-config facts.

Finding 16 (test-reviewer, low, conf 95): CLI --gpus → gpu_replicas wiring untested (both tests pass gpus=None; a constant [] would pass). Cheap one-line assertion; extend fully alongside the GPU rework.

Finding 17 (test-reviewer, low, conf 95): Parser tail-read branch (size > 4096 → seek) is dead in the suite — every fixture is a few hundred bytes, yet that's the branch every real GROMACS log takes.

Finding 18 (test-reviewer, low, conf 85): On-disk persistence of a failed sweep never verified — the live test only asserts the file exists, and the round-trip test's trials all have error=None.

Finding 19 (test-reviewer, low, conf 85): MDP-generation edge inputs untested — source MDP without dt (default fallback) and the io_interval = max(nsteps, 1000) floor.

Finding 20 (simplifier, low, conf 90): MDRUN_PERF_MARKER / parse_performance_marker docstrings still describe a stdout-based design that no longer exists — the script writes to a sidecar file, and benchmark.py self-contradicts (stdout vs caller-named file three lines below).

Finding 21 (simplifier, low, conf 85): Two idioms test the same exclusive capability ("exclusive" in type(...).model_fields vs hasattr(...)); bind once.

Finding 22 (simplifier, low, conf 82): Redundant or 0.0 in the default selection branch (the efficiency branch's or 0.0 must stay — None is possible there).

Finding 23 (simplifier, low, conf 80): _generate_benchmark_mdp's dt parameter is unreachable in production (sole caller never passes it); the branch exists only for its own test.

Also noted (below threshold, informal): make BenchmarkResult.save atomic (temp + os.replace); log config.provider/gres at sweep start; clean up accumulating *.perf sidecars.

Strengths

• The two commits under review are correct and regression-free; each fix ships with a dedicated test that fails against pre-fix code (CPU thread labels, lock-before-MDP, submit-before-join event ordering, exclusive-not-mutating-base).
• exclusive gating done right: class-level model_fields check, non-mutating model_copy, local configs untouched, base config asserted unmutated.
• Sweep measurements are well-defended against HPC footguns: uuid-unique sidecars defeat stale shared-FS reads, ns/day extracted on the writing worker, single allocation removes placement variance, per-trial try/except isolates failures, flock is correct (fail-fast, finally unlock, CLOEXEC, kernel release).
• Rationale-carrying comments (staleness, lock ordering, the ~37% swing behind --exclusive) encode why tied to observed failures; NumPy docstring convention followed throughout.
• Pure Parsl-free helpers (parse_*, _generate_benchmark_mdp, _sweep_lock) are what make the mocked execution tests possible.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[gregorweiss]

Independent Assessment

Each review finding was re-verified against the actual source at head af696dd by an independent agent, applying a factual gate and a scope gate. Authoritative scope is issue 9 plus the mach6-plan comment, plus the human-approved GPU-axis rework note (queued, not yet implemented).

Review: #54 (comment)

Classifications

Finding	Classification	Why
1 — GPU points with cpu_count < gpu_replicas compute ntasks=0	genuine	Confirmed in code: GPU branch sets max_workers_per_node: gpu_rep, extract_resource_hints floors cpus // max_workers to 0 — the auto-detect sentinel — so replicas run the allocation's core count, not the labeled cpu_count // R. Mislabeled trial recorded error=None and can win _select_optimum, poisoning acceptance criterion (c). Data-integrity bug in today's code; the 25d9c23 fix guarded only the CPU-only branch.
2 — "Reuse across HT batch" absent	genuine	Confirmed: zero atom-count grouping, representative selection, or consumers of benchmark_result.json beyond its writer; the CLI runs an independent full sweep per directory. The section is verbatim in issue 9's scope; the plan's omission of it is a decomposition gap, not an approved scope reduction, and only the GPU axis was human-approved as a change.
3 — sed extraction never executed	genuine	Confirmed: _PERF_EXTRACT is the sole live-path producer of ns/day; the only test asserts a substring that survives pattern mutation. Plan testing approach mandates parser fixtures. Test gap on PR-added behavior — a regex regression breaks every live trial with CI green.
4 — mdp_overrides silently dropped when key absent	genuine	Confirmed: modify_mdp_value only replaces matching lines, never warns. Consequence slightly overstated (GROMACS default is a compressed trajectory at default frequency, and in-repo templates include the key), but the core problem — documented user config silently ignored — is real. mdp_overrides is PR-added documented config; correct behavior requires append-or-warn.
5 — --gpus without gres → mislabeled GPU trials	genuine	Confirmed: GPU enablement derives solely from base gres; no cross-check anywhere; a YAML lacking gres yields disable_gpu=True CPU trials labeled gpu_replicas=R, feeding _select_optimum. Correctness/integrity failure introduced by this PR's --gpus flag; one-line validation required.
6 — No SLURM executor validation	deferred	Factual: docstring requires SLURM, nothing enforces it. Scope: JSON makes no false in-data claim, user supplies the YAML, acceptance (d) anticipates no-cluster operation. Optional input hardening.
7 — One shared allocation vs per-point sessions	deferred	Factual: one parsl_session wraps all points, test asserts call_count == 1, deviating from plan deliverable 5's wording. Scope: acceptance (b) is met functionally — each config runs with its correct thread count sequentially; the deviation is deliberate, documented, and improves measurement validity. Process item: obtain the repo owner's explicit sign-off that single-allocation supersedes the plan's per-point-session wording.
8 — Hyphenated mdp_overrides untested	genuine	Confirmed: documented hyphen form never applied in tests; deleting the normalization leaves all green while the override is dropped. Test gap on PR-added documented behavior, and the test is needed to lock in finding 4's fix.
9 — Dry-run side-effect-freeness unpinned	genuine	Confirmed: no assertion blocks a mutation that saves benchmark_result.json before returning (clobbering real results with null trials). Acceptance (d) + plan testing approach item 5 ("verify no Parsl loaded") mandate exactly these assertions.
10 — CLI per-trial reporting unasserted	genuine	Confirmed: deleting the per-trial loop leaves all 4 tests green. Sweep-time logs still warn (slight overstatement), but the CLI summary's N/A + error lines are unverified PR-added user-facing behavior. Low priority.
11 — Repeated mock scaffolding (7 tests)	nitpick	True but pure readability refactor; no correctness impact.
12 — All-failed sweep exits 0	deferred	Factual: only a warning is emitted. Scope: exit-code semantics are in neither issue nor plan; failure is surfaced via warnings, per-trial errors, and optimum: null. Optional CLI robustness.
13 — -pin on explicit vs auto	nitpick	Within a sweep, gmx_binary is inherited uniformly, so all trials use identical pinning; no demonstrated correctness impact.
14 — testextract_resource_hints_* names	nitpick	Five mangled names verified; still collected; cosmetic only.
15 — unused base_config param	nitpick	Dead parameter, no behavioral effect.
16 — --gpus wiring untested	genuine	Confirmed: both CLI tests pass gpus=None; a constant [] would pass. CLI wiring for an issue-9 sweep dimension — cheap assert in an existing module; test ships with PR-added behavior.
17 — Parser tail-read branch dead in suite	genuine	Confirmed: all fixtures <4 KB so the seek branch every real log takes is never executed. Coverage gap (not a live defect) on deliverable 1 / acceptance (a); a cheap >4 KB fixture covers it.
18 — Failed-sweep persistence unverified	genuine	Confirmed: live test only asserts the file exists; round-trip trials all error=None. Acceptance (c) requires all trials persisted; persistence of failed trials is on the acceptance path.
19 — MDP edge inputs untested	nitpick	Uncovered branches are GROMACS-default fallbacks and a defensive floor; substantive behaviors all covered.
20 — Stale stdout docstrings	nitpick	Documentation accuracy only; no behavioral impact.
21 — Two exclusive idioms	nitpick	Readability/bind-once preference; both correct.
22 — Redundant or 0.0	nitpick	Harmless stylistic redundancy.
23 — dt param unreachable	nitpick	Dead-by-default API parameter with a tested branch; simplification preference.
Informal group (atomic save, provider/gres logging, *.perf cleanup)	deferred	Real observations; hardening/cleanup not required by issue 9, plan, or acceptance criteria.

Action Plan (genuine issues only, in priority order)

1. Finding 1 — Guard the GPU branch so each replica's thread count equals its label (ntasks = max(1, cpu_count // R), or pass thread counts explicitly rather than relying on extract_resource_hints' floor); add a regression test for cpu_count < gpu_replicas. Mislabelled trials must not be recordable as valid until the multi-GPU rework lands.
2. Finding 2 — Implement issue 9's "Reuse across HT batch" (±20% grouping, representative selection, apply optimum to the batch) or obtain an explicit human-approved deferral to a follow-up issue before merge.
3. Finding 5 — Fail fast (or warn loudly) when gpu_replicas is non-empty but the executor config carries no GPU gres.
4. Finding 4 — Make mdp_overrides append absent keys or warn on no-match, and correct the overdrawn docstring consequence.
5. Finding 8 — End-to-end test applying the documented hyphenated {"nstxout-compressed": "0"} form (locks in finding 4's fix).
6. Finding 3 — Execute the generated sed extraction against a realistic log fixture and assert the value so a pattern regression fails CI.
7. Finding 9 — Pin dry-run side-effect-free: no benchmark_result.json, no .benchmark/ dir or lock, cpu_count labels, and no Parsl import.
8. Finding 16 — Assert --gpus 2 4 reaches BenchmarkConfig.gpu_replicas == [2, 4].
9. Finding 10 — Assert _report_benchmark_result per-trial lines include N/A + error annotation.
10. Finding 17 — Add a >4 KB log fixture exercising the tail-read branch.
11. Finding 18 — Assert failed/partial sweep JSON round-trips trials with their error fields.

Deferred (optional, non-blocking): findings 6, 7 (process sign-off), 12, and the informal hardening group. No tracking issues exist for these yet.

---

Assessed by mach6

[gregorweiss]

Progress Update

Fixed four genuine findings from the latest review round.

Sweep correctness (mdfactory/performance/benchmark.py)

• GPU thread-count labels (finding 1): _build_sweep_configs now skips any GPU point where cpu_count < gpu_replicas — that combination used to floor per-replica ntasks to 0, hit the auto-detect sentinel, and run node-wide threads under a false label while feeding _select_optimum. Validators now also reject cpu_counts/gpu_replicas values < 1, and an all-infeasible grid raises a clear ValueError instead of an empty-max(). Every surviving point provably gives each replica >= 1 thread.
• --gpus without a GPU (finding 5): a sweep with gpu_replicas set but no GPU gres now fails fast with ValueError instead of silently running CPU-only trials labeled as GPU points. Validated after the dry-run return so a cluster-free preview still works.
• Dropped mdp_overrides (finding 4): an override naming a key absent from the source MDP is now appended rather than silently ignored (modify_mdp_value only replaces). Present keys still replace; the documented {"nstxout-compressed": "0"} example now actually takes effect.

Tests

• Sed extraction executed (finding 3): new TestPerfExtractProgram runs the real _PERF_EXTRACT sed program against normal / restart / truncated GROMACS logs — pinning the exact value and the | tail -1 behavior, plus a marker round-trip. Previously only the script's substring was asserted, so a corrupted pattern would pass CI while every live trial reported "no performance data".
• Regression tests for all the above: infeasible-point skipping, all-infeasible raise, zero-value validation, absent-key append, and no-gres GPU sweep raising.

Verification: ruff check + ruff format --check clean (131 files); full short suite 3005 passed, 4 skipped. The single failure (test_settings.py::test_defaults_loaded) is pre-existing — the untracked local config.toml sets database.TYPE=csv; settings is untouched by this PR and the test fails on clean HEAD too.

Not in this batch (queued): finding 2 ("Reuse across HT batch" — needs an implement-vs-defer decision) and the smaller genuine items 8, 9, 10, 16, 17, 18.

Commit: 942e436

---

Progress tracked by mach6

[gregorweiss]

Progress Update

Reworked the GPU sweep axis and fixed the multi-rank launch path.

GPU axis: one simulation across N GPUs

• A GPU point with R now runs ONE simulation across R GPUs via srun -n R --gpus-per-task=1 (one MPI rank per GPU), replacing the earlier N-independent-simulations/aggregate model. The recorded ns/day is that single simulation's throughput.
• Cores tied 1:1 to GPUs (option a): each point uses R cores, one per rank, 1 thread/rank — pure MPI, no OpenMP hybrid, so threads/rank is constant across the sweep. --gpus 1 2 4 → points (1,1), (2,2), (4,4); --cpus is ignored for GPU sweeps (removed the fixed-CPU cross-product validator accordingly).

Multi-rank launch fixes

• OMP clash: the cluster's task-prolog exports OMP_NUM_THREADS=$SLURM_CPUS_PER_TASK per srun task, which overrode our export and clashed with -ntomp (GROMACS 2024 fatal: OMP_NUM_THREADS (8) != 4). Fixed by passing --cpus-per-task=$NTHR on the srun launcher so the prolog enforces the matching value.
• -npme 1: GROMACS aborts multi-rank -pme gpu without an explicit -npme, and this build lacks cuFFTMp so the value must be ≥ 1 (-npme 0 spreads PME over multiple CUDA devices → fatal). A dedicated PME rank keeps each PME task on one GPU.
• GPU_ID=0: each rank's one pinned GPU is device 0 inside the process (CUDA renumbers it); the old GPU_ID=$CUDA_VISIBLE_DEVICES sent an out-of-range physical id for ranks ≥ 1.

Single-allocation hard guarantee

• The sweep's allocation_config now forces max_blocks=1 (alongside exclusive/gres/available_accelerators), so "one allocation" is guaranteed rather than emergent from the sequential design. Base configs (simulate) keep their multi-block parallelism.

Local (uncommitted) tuning note: GMX_ENABLE_DIRECT_GPU_COMM=1 was tried and removed — it made multi-rank runs hang in GPU-aware MPI (single-GPU unaffected; gpus=2 completed at 93.6 ns/day without it, hung at 49 s with it). Left off in bench_e2e/slurm_gpu.yaml with a NOTE.

Verification: ruff check + ruff format --check clean (131 files); full short suite 3011 passed, 4 skipped. The single failure (test_settings.py::test_defaults_loaded) is pre-existing — a local config.toml sets database.TYPE=csv; settings is untouched by this PR.

Commit: 87cbfff

---

Progress tracked by mach6

[gregorweiss]

Code Review

Scope: issue 9 plus the mach6-plan comment, plus the human-approved GPU-axis rework (one simulation across N GPUs; shared-GPU MPS deferred to issue 10). Reviewed at head 87cbfff, which builds on the prior fix commits 25d9c23, 942e436, af696dd. Full PR suite green at head (184 tests in the three touched test files); ruff clean. No critical issues.

Critical

None.

Important

Finding 1 (completeness-checker, high, conf 100): Issue 9's "Reuse across HT batch" section is entirely absent. None of its three behaviors exist: no ±20% atom-count grouping, no representative-system selection, and no mechanism that applies a stored optimum to production runs. benchmark_result.json's only consumers are its writer and tests, and the CLI does the opposite — it loops every sim directory and runs an independent full sweep per system (cli.py:771-774). The plan comment omitted this section, but the issue body governs, and only the GPU-axis rework was human-approved as a scope change.

Finding 2 (test-reviewer, high, conf 95): The gpus kwarg forwarding inside the real mdrun app is never executed by any test. get_mdrun_app()'s returned function is never invoked; every mdrun test calls _build_mdrun_script directly, and the benchmark test only checks gpus on a MagicMock (test_benchmark.py:839). Mutation-verified: deleting gpus=gpus at apps.py:893 leaves all 184 tests green — every real GPU trial would silently run single-rank with no srun -n R, --gpus-per-task=1, or -npme 1, directly on acceptance criterion (b). (performance_file= forwarding at apps.py:892 has the same untested-link structure.)

Finding 3 (test-reviewer, high, conf 95): The mdrun future join and mdrun-failure path are untested. The only failure-path test raises from grompp's .result(), not mdrun's, and the perf sidecar is written at mock submit time, so a reordered read is invisible. Mutation-verified: deleting mdrun_future.result() at benchmark.py:737 leaves all 184 tests green. In production the sweep would not wait for mdrun or propagate its exceptions, perf reads would race the worker's sidecar write (every trial → "no performance data"), and wall_seconds would be meaningless.

Suggestions

Finding 4 (error-auditor, medium, conf 90): A totally failed sweep exits 0. When every trial fails, run_benchmark_sweep returns normally with optimum=None, _report_benchmark_result logs one warning (cli.py:787), and benchmark() returns — so mdfactory benchmark … && … and CI treat a fully failed sweep as success. Suggest sys.exit(1) when optimum is None and not dry-run (cli.py:770-797).

Finding 5 (error-auditor, medium, conf 83): A CPU sweep against a GPU-enabled executor config silently runs GPU-offloaded trials labeled gpu_replicas=0. The "GPU sweep without gres" guard only fires when gpu_replicas is non-empty (benchmark.py:548-557); the reverse case is unguarded. extract_resource_hints sets disable_gpu=False whenever gres names a GPU (stages.py:186), so --cpus 1 2 4 on gres: gpu:l40s:1 produces -nb gpu -pme gpu -gpu_id 0 trials recorded as CPU-only, feeding _select_optimum. The config docstring explicitly promises an empty gpu_replicas runs a CPU-only sweep. Suggest symmetric validation or explicitly forcing disable_gpu=True on CPU-point allocations.

Finding 6 (error-auditor, medium, conf 80): The documented "a SLURM executor is required" is never enforced. _load_executor_config accepts any YAML and only nodes > 1 is rejected, so a provider: local file runs the whole sweep on the submitting/login node while cpus_per_node/exclusive are injected into config fields LocalProvider doesn't use (silently dropped by model_dump). The result JSON reports cluster-style labels for login-node measurements. Suggest rejecting non-SLURM providers after the dry-run return.

Finding 7 (code-reviewer, medium, conf 82): The multi-rank GPU launcher never enforces an MPI GROMACS build. srun -n {gpus} … is applied purely on GPU count (apps.py:681) while the binary is hints.gmx_binary, defaulting to "auto" — which prefers thread-MPI gmx first (apps.py:324-330). On a host where gmx resolves, an R>1 point emits srun -n R gmx mdrun -ntmpi 1 …, i.e. R independent single-rank processes sharing one deffnm, contradicting the approved one-rank-per-GPU design and recording single-rank ns/day as an R-GPU point. Suggest resolving to gmx_mpi for multi-rank points or raising on thread-MPI + gpus > 1.

Finding 8 (test-reviewer, medium, conf 95): selection, duration_ps, and mdp_overrides are never exercised through run_benchmark_sweep — only against the bare helpers/CLI construction. Mutations verified surviving (184 passed): hardcoding _select_optimum(trials, "ns_per_day") (benchmark.py:771/776) makes --selection efficiency a silent no-op; hardcoding duration_ps=100.0 and dropping extra_overrides (benchmark.py:639-640) makes --duration-ps and --mdp-override no-ops.

Finding 9 (test-reviewer, medium, conf 88): gmx_binary hint forwarding from the sweep to the mdrun app is unasserted (benchmark.py:728). Mutation-verified: deleting gmx_binary=hints.gmx_binary leaves 184 passed. Trials would fall back to "auto" and could land on thread-MPI gmx despite a pinned gmx_mpi config.

Finding 10 (simplifier, medium, conf 90): The same ~14-line mock-wiring block is verbatim in five execution tests and near-verbatim in two more (test_benchmark.py:626-956). Extracting a _wire_sweep_mocks(...) helper (like the existing _setup_sim_dir/_mdrun_writes_perf) removes ~85 duplicated lines with identical wiring order and assertions.

Finding 11 (completeness-checker, low, conf 85): Dry-run satisfies the cluster-free half of acceptance (d) but "generates scripts" half is missing — it logs only descriptors (cpus=…, gpu_replicas=…, duration=…, benchmark.py:526-537), no grompp/mdrun command text, whereas the run_simulations dry-run pattern it claims to follow prints resolved gmx commands per stage.

Finding 12 (error-auditor, low, conf 80): The first trial's wall_seconds includes SLURM block/queue startup because the timer starts at benchmark.py:674 while the Parsl block launches asynchronously; trials 2..N do not. The persisted wall_seconds is not cross-trial comparable. Suggest starting the timer just before mdrun submission or warming the block once, and documenting what it covers.

Finding 13 (code-reviewer, low, conf 95): The new gpus: int = 1 parameter of _build_mdrun_script and run_mdrun (apps.py:607, 820) is undocumented in their NumPy-style Parameters blocks; every other parameter is documented. AGENTS.md mandates NumPy-style docstrings.

Finding 14 (code-reviewer, low, conf 85): --dry-run always ends with _report_benchmark_result's "No successful trial" warning, because the dry-run result has optimum=None by construction (cli.py:787). A successful preview prints a failure warning for every system.

Finding 15 (code-reviewer, low, conf 90): The multi-node rejection reason is now stale: it says "Benchmark trials run as a single MPI rank" (benchmark.py:508, 596) while 87cbfff makes GPU trials explicitly multi-rank. The rejection is still correct, but the stated reason contradicts the feature.

Finding 16 (code-reviewer, low, conf 80): gpu_replicas now names a quantity that no longer exists — a gpu_replicas=2 point is one simulation across 2 GPUs. The name survives in BenchmarkConfig/TrialResult, the persisted benchmark_result.json keys, and log output, while the CLI help already says "GPU counts".

Finding 17 (test-reviewer, low, conf 90): GPU trial-directory assertions are vacuous. assert not (trial_dir / "bench_r0.tpr").exists() passes whether or not trial_dir was ever created; the GPU test never asserts the dir exists, that topology.top/extra_params.itp are staged, or that its md.mdp resolves to the short benchmark.mdp (all positively asserted only for the CPU point).

Finding 18 (test-reviewer, low, conf 95): test_dry_run_no_parsl (test_benchmark.py:509-517) asserts nothing about Parsl despite its name/docstring — no sys.modules check and no assertion that dry-run leaves no filesystem side effects.

Finding 19 (test-reviewer, low, conf 95): test_dry_run_with_gpu's docstring still describes the removed cpu_counts × gpu_replicas cross-product (test_benchmark.py:519-521); no dry-run test asserts trial.cpu_count == trial.gpu_replicas for a GPU point.

Finding 20 (simplifier, low, conf 95): _build_sweep_configs takes a base_config parameter it never reads (benchmark.py:333-334). Drop it (and the docstring's "from base config" phrase) and update the five call sites.

Finding 21 (simplifier, low, conf 95): Five test names were mangled to testextract_resource_hints_* by the _-stripping rename (test_orchestration_simulate.py:1837, 1851, 1861, 1871, 1882); restore the underscore.

Finding 22 (simplifier, low, conf 85): All three test_benchmark_command_* tests repeat the same staging + patch block (test_orchestration_cli_benchmark.py:33-109); a _run_benchmark_cli(...) helper would remove the duplication while preserving assertions.

Finding 23 (simplifier, low, conf 95): _assemble_mdrun_command has a dead conditional — f"{launcher}{command}" if launcher else command is identical to f"{launcher}{command}" for all inputs (apps.py:238).

Finding 24 (simplifier, low, conf 95): Redundant forwarding intermediate ns_per_day = float(...); return ns_per_day in parse_mdlog_performance (benchmark.py:76-77).

Finding 25 (simplifier, low, conf 88): The CLI CPU-count fallback list(gpus) if gpus else (cpus or [1, 2, 4, 8]) can be simplified to gpus or cpus or [1, 2, 4, 8] (cli.py:763), with identical resulting BenchmarkConfig.

Strengths

• Human-approved GPU rework implemented to spec and pinned at the builder layer: exact script text tests for srun -n 2 --cpus-per-task=$NTHR --gpus-per-task=1 gmx_mpi mdrun, -npme 1 (with its two omission cases), GPU_ID=0/no CUDA_VISIBLE_DEVICES, and CPU-only scripts free of GPU_ID.
• Allocation semantics thoroughly asserted: single parsl_session, cpus_per_node = max point, max_workers_per_node=1, max_blocks=1, exclusive=True forced on a copy with base-config immutability asserted, gres scaling (gpu:l40s:1→2), available_accelerators=0.
• Measurement integrity is well defended: worker-side ns/day extraction into uuid-unique sidecars defeats stale shared-FS reads; ns_per_day=None after a clean mdrun is converted to an explicit error; per-trial try/except isolates failures; the flock sweep lock is taken before the shared MDP write and fails fast/self-releases.
• Both prior fix commits are intact at head (lock-before-MDP, max_workers_per_node normalization, explicit inputs, .itp staging, absent-key MDP override append, GPU-without-gres fail-fast), and the now-obsolete cross-product skip removal is consistent with the approved rework.
• Parser coverage is genuinely end-to-end: both the Python parser and the worker-side sed program (the sole live-path ns/day producer) are executed against normal/restart/truncated fixtures, including the | tail -1 collapse.
• Load-bearing comments explain why (stale inodes, the measured ~37% contention swing behind --exclusive, the cuFFTMp -npme constraint, the prolog/-ntomp clash); NumPy docstring convention followed throughout.
• End-to-end SLURM runs documented in progress comments (reproducible CPU 1→64 curve, multi-GPU runs) corroborating acceptance criteria (a)–(c) beyond mocks.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[gregorweiss]

Independent Assessment

Each review finding was re-verified against the actual source at head 87cbfff by an independent agent, applying a factual gate and a scope gate. Authoritative scope is issue 9 + the mach6-plan comment + the human-approved GPU-axis rework (one simulation across N GPUs; shared-GPU MPS deferred to issue 10). Review: #54 (comment)

Classifications

Finding	Classification	Reasoning
1 — "Reuse across HT batch" absent	genuine	Factual: no ±20% atom-count grouping, no representative selection; cli.py:771-774 loops an independent full sweep per sim dir; BenchmarkResult.load has no production consumer. Scope: issue 9 explicitly includes this section; only the GPU rework was human-approved as a scope change.
2 — gpus forwarding in real mdrun app untested	genuine	Factual: get_mdrun_app appears only as a patch target; the returned run_mdrun is never invoked, so gpus=gpus at apps.py:~893 never runs; only a MagicMock kwarg is asserted. Scope: PR-added wiring on the exact path that produces the srun -n R commands required by acceptance (b).
3 — mdrun join / mdrun-failure untested	genuine	Factual: the only failure test raises from grompp; every mdrun mock returns 0 and writes the sidecar at submit time, so deleting mdrun_future.result() leaves all tests green. Scope: the join is load-bearing PR-added code; without it every trial races to "no performance data".
4 — failed sweep exits 0	deferred	Factual: confirmed — no sys.exit; only a warning. Scope: exit-code semantics are in neither issue nor plan; optional CI/usability improvement.
5 — CPU sweep on GPU config runs GPU-offloaded trials labeled gpu_replicas=0	genuine	Factual: guard only fires when gpu_replicas is non-empty; extract_resource_hints sets disable_gpu=False for gres: gpu:..., so CPU points run -nb gpu while recorded as CPU-only and fed to _select_optimum. Scope: silently corrupts the result JSON relative to labels — core deliverable correctness; the PR's own mirror-image guard treats this as a bug.
6 — SLURM executor not enforced	deferred	Factual: confirmed — _load_executor_config accepts any provider; only nodes > 1 is rejected. Scope: not required by issue/plan; hardening contingent on a user-supplied local config.
7 — multi-rank launcher doesn't enforce MPI build	genuine	Factual: srun -n {gpus} applied on GPU count alone; binary defaults to "auto", whose detection prefers thread-MPI gmx; yields srun -n R gmx mdrun -ntmpi 1, i.e. R independent single-rank processes. Scope: PR-introduced path under the approved one-rank-per-GPU rework; approved behavior silently not delivered.
8 — selection/duration_ps/mdp_overrides untested through the sweep	genuine	Factual: only tested against the bare helpers/config/CLI construction; hardcoding any of them survives the suite. Scope: selection and the short benchmark MDP are explicit issue deliverables; end-to-end wiring is PR-added.
9 — gmx_binary forwarding unasserted	genuine	Factual: zero gmx_binary matches in the benchmark tests; deleting the forwarding leaves the suite green. Scope: PR-added forwarding on the execution path that selects the GROMACS binary (coupled to finding 7, acceptance (b)).
10 — repeated mock-wiring scaffolding	nitpick	Test-style deduplication only.
11 — dry-run emits no scripts/command text	genuine	Factual: dry-run logs only descriptors and returns; no command/script text anywhere. Scope: acceptance (d) literally requires "generates scripts without submitting"; the cluster-free half passes but the script half is unmet.
12 — first trial wall_seconds includes queue startup	deferred	Factual: confirmed timing asymmetry. Scope: wall_seconds is informational, unused for selection or acceptance.
13 — gpus param undocumented	nitpick	One-line docstring gap; style only.
14 — dry-run prints "No successful trial"	nitpick	Cosmetic log inconsistency.
15 — stale "single MPI rank" rejection message	nitpick	Wording stale; rejection itself still correct.
16 — gpu_replicas naming stale	nitpick	Renaming would be a breaking schema change; not required.
17 — vacuous GPU trial-dir assertions	nitpick	Staging code is shared with the CPU point (positively asserted); failures surface via trial.error; weak-assertion style only.
18 — test_dry_run_no_parsl asserts nothing re Parsl	nitpick	Cluster-free behavior is exercised by running with a local config; extra import assertion is optional strictness.
19 — stale cross-product docstring	nitpick	Docstring-only; cores-tied-1:1 asserted elsewhere.
20 — unused base_config param	nitpick	Dead-parameter cleanup.
21 — mangled testextract_resource_hints_* names	nitpick	Cosmetic; tests still collect.
22 — repeated CLI-test setup block	nitpick	Test-style deduplication.
23 — dead conditional in _assemble_mdrun_command	nitpick	Behavior-preserving simplification.
24 — redundant return intermediate	nitpick	Pure style.
25 — simplifiable CPU-count fallback	nitpick	Style-only refactor.

Action Plan (genuine issues only, priority order)

1. Finding 1 — implement issue 9's "Reuse across HT batch" (±20% atom-count grouping, representative-system selection, one sweep per group, and apply the group's optimum to production runs instead of sweeping every sim dir), with tests; or obtain an explicit human-approved deferral to a follow-up issue before merge.
2. Finding 5 — make CPU points run CPU-only on a GPU-enabled executor config (force disable_gpu=True for gpu_reps == 0, or strip gres on the CPU allocation), or add symmetric fail-fast validation; test with gres: gpu:l40s:1 + a CPU sweep.
3. Finding 7 — for gpus > 1, raise if hints.gmx_binary == "gmx" and resolve "auto" to gmx_mpi (or make detection prefer gmx_mpi when the srun -n R launcher is active); test that no multi-rank point emits thread-MPI gmx.
4. Finding 3 — test mdrun .result() raising (trial records the error, sweep continues) and that the perf sidecar is read only after the join, so deleting mdrun_future.result() fails.
5. Finding 2 — exercise the function returned by get_mdrun_app() and assert gpus/performance_file reach _build_mdrun_script, so deleting gpus=gpus fails.
6. Finding 8 — add sweep execution tests for selection="efficiency", non-default duration_ps (→ nsteps), and mdp_overrides reaching the written MDP.
7. Finding 9 — with a pinned gmx_binary: gmx_mpi, assert mock_mdrun.call_args.kwargs["gmx_binary"] == "gmx_mpi".
8. Finding 11 — log the resolved grompp/mdrun command lines (including srun -n R --gpus-per-task=1) per trial in the dry-run branch, with a test on the command text.

8 genuine / 14 nitpicks / 0 false positives / 3 deferred

Deferred (optional, non-blocking): finding 4 (exit code on total failure), finding 6 (reject non-SLURM providers), finding 12 (wall_seconds timer scope). No tracking issues exist for these yet.

---

Assessment by mach6

[gregorweiss]

Progress Update — re-scoped to CPU scalability only

The PR now covers only CPU scalability: the highest CPU count that is still justified by the 75% parallel-efficiency rule of thumb. All GPU benchmark work is reverted.

Removed (GPU axis)

• BenchmarkConfig.gpu_replicas, TrialResult.gpu_replicas, per-point config overrides and _build_sweep_configs.
• Multi-rank launch path in apps.py: the srun -n R --gpus-per-task launcher, -npme 1, the GPU_ID=0 rework (restored GPU_ID=${CUDA_VISIBLE_DEVICES:-}), and gpus parameters/forwarding.
• _scale_gres, gres scaling and available_accelerators=0 in the sweep allocation; the GPU-without-gres fail-fast.
• The _extract_resource_hints → public-API rename and its fallout (stages.py, simulate.py, config.py, five mangled test names). The sweep no longer needs hints: thread count is the point's CPU count.

Re-scoped (CPU axis)

• BenchmarkConfig: cpu_counts (default 1 2 4 8 16 32 64), duration_ps, efficiency_threshold (default 0.75), mdp_overrides.
• Selection is no longer a ns_per_day/efficiency criterion on a stored optimum. BenchmarkResult now derives baseline, efficiency(trial) and max_justified from the trials, so there is one answer instead of two modes.
• Trials force disable_gpu=True, so a GPU-capable executor config cannot produce a GPU curve labelled as CPU.
• _select_optimum and parse_mdlog_performance are gone: the former duplicated what properties now express, the latter was unused by the live path (the worker-side sed program plus the sidecar reader is the only producer of ns/day).

Still CPU-relevant and kept

• Worker-side ns/day extraction into a unique sidecar (stale shared-filesystem reads).
• -pin on for CPU thread flags: GROMACS otherwise stops pinning when Slurm confines the job to a cpuset, which would make the scaling curve noisy.
• Single exclusive allocation sized to the largest count, max_workers_per_node=1, max_blocks=1, and the flock sweep lock taken before the shared MDP write.

Docs and size

• docs/content/docs/user-guide/cli-reference.mdx gains a mdfactory benchmark section stating the CPU-only scope.
• Net effect vs. the previous head: 717 insertions / 1557 deletions across 11 files; the diff against develop drops from 2237 to 1339 added lines.

Verification

ruff check and ruff format --check clean on all touched files. 191 passed for the three benchmark/simulation test files; full short suite 3470 passed, 5 skipped with 3 pre-existing environment failures unrelated to this PR (test_proteinbox TestCleanPdb needs pdbfixer; test_utilities::test_lock_folder_processes fails on macOS spawn-pickling).

Commit: 3ea26ca

---

Progress tracked by mach6