SeedReport: Saving & Replaying RNG seed capability.

[drewconnelly-qntm]

Changes:

In crates/pecos-engines/src/monte_carlo/engine.rs:

• Introduces SeedReport which stores the RNG seed information for a monte carlo run
• Introduces run_with_workers_seed_report which runs a monte carlo job with workers, while managing the RNG seeds with a seed report. Has a bool option to save the report to a JSON names seed_report.json.
• Introduces WorkerSeedRecord which stores the worker index, num shots, and seed for that worker, which is what is held in a vector inside the overall job's SeedReport for each monte carlo job.
• Introduces methods for saving a SeedReport to JSON as well as loading them from JSON.

Potential Future Work:

• Lets work start up on Error Pruning, Visualizing now that we can save monte carlo runs and rerun them.
• Opens the possibility for single-shot replaying, so the user can identify a particular shot (or maybe all shots with logical failures) and rerun that exact shot. Right now only rerunning the entire monte carlo job is permitted.

Testing:

cargo test -p pecos-engines was all good
just build; just test ran through the rust tests fine but had issues with the python pytests due to some bugs with my environment locally.

[qciaran]

I haven't looked closely but just skimming things: You might want to see if you can do this without using an Arc Mutex. They are slightly expensive.

[drewconnelly-qntm]

I haven't looked closely but just skimming things: You might want to see if you can do this without using an Arc Mutex. They are slightly expensive.

Thanks, I'll check on how necessary that is.

[ciaranra]

Review

Nice feature and the right general shape: the report is the single source of truth for worker seeds, the JSON round-trip is tested, and error paths map to PecosError::Input. run_with_workers() delegating to the new method (rather than keeping a third copy of the run loop) is also good. I ran the PR's tests (7/7 pass) and clippy (clean) locally. That said, there are two blocking issues and a few smaller ones.

1. The replay tests are vacuous (blocking)

The two headline tests — rerun_from_seed_report_reproduces_original_results and rerun_from_seed_report_loaded_from_json_reproduces_original_results — currently prove nothing. Verified by experiment: running the fixture with seed 42 vs seed 999999 produces identical results. ExternalClassicalEngine always returns result: 0 and new_with_defaults uses pass-through noise, so shot output does not depend on seeds at all. The reproduce assertions would still pass if all seeding logic were deleted.

Shot-level determinism is untested anywhere: run_with_seed_report_is_deterministic_for_same_seed_workers_and_shots only compares report metadata, and its different-seed engine (c) only asserts the seeds differ, not the results.

Fix: use a fixture whose output actually depends on the RNG (e.g. new_with_depolarizing_noise with a circuit that measures), then mutation-check the tests: a different seed must produce different shots, and replaying the recorded seeds must produce equal shots.

2. ~110 lines duplicated verbatim (blocking)

rerun_from_seed_report is a byte-for-byte copy of the execution body of run_with_workers_seed_report — worker-engine setup, dedicated thread pool, panic-catching shot loop, result sorting — differing only in where the seeds come from. Any future fix to the pool/panic/ordering logic now has to be made twice, and the copies will drift. Suggest factoring one private helper, e.g.:

fn run_workers_with_seeds(
    &self,
    worker_seeds: &[u64],
    shots_per_worker: &[usize],
) -> Result<ShotVec, PecosError>

and having both paths call it.

3. Replay half-ignores the loaded report

A SeedReport loaded from JSON is a system boundary, and replay handles it loosely:

• seed_report.workers[worker_idx] panics if workers.len() < num_workers. A truncated or hand-edited file should produce PecosError::Input, not a panic.
• The recorded WorkerSeedRecord.shots and worker_idx fields are never used: replay recomputes distribute_shots(num_shots, num_workers) and indexes positionally. A report with reordered workers or edited shot counts replays silently wrong. Either use the recorded values or validate that they match — otherwise they're dead data.

4. self.seed = seed_report.root_seed leaves the engine inconsistent

rerun_from_seed_report assigns the seed field directly, but set_seed() also reseeds self.rng and the hybrid_engine_template. After a replay, the engine claims seed == root_seed while its RNG stream and template are elsewhere — a subsequent run_with_workers call produces a report whose root_seed doesn't correspond to its base_seed. Either call self.set_seed(seed_report.root_seed) or don't mutate engine state at all (replay doesn't need it — the worker seeds come from the report). The assignment also sits before the zero-shot/zero-worker asserts.

5. API shape

The save_seed_report: bool flag plus the hardcoded "seed_report.json" path is a footgun: it silently overwrites in the current working directory, concurrent runs clobber each other, and the caller can't choose a path. Simpler API: drop the bool, always return the report, and give SeedReport a to_json_file(path) method symmetric with from_json_file. That also removes MonteCarloEngine::save_seed_report_json, which doesn't touch the engine — the behavior belongs on SeedReport.

Nits

• crates/pecos-engines/Cargo.toml: use tempfile.workspace = true — the workspace root already defines tempfile = "3" and the other crates use the workspace entry.
• save_seed_report_json calls std::fs::write even though use std::fs; is imported.
• Worth a doc note on SeedReport that root_seed alone cannot reproduce base_seed if the engine ran jobs before this one (the RNG has advanced) — the per-worker seeds are the actual replay contract.

[ciaranra]

Review: SeedReport — saving & replaying RNG seeds

One blocking issue and several boundary-validation and packaging problems below. Line numbers refer to the PR head.

Blocking

1. Every normal run now rewinds the engine RNG (crates/pecos-engines/src/monte_carlo/engine.rs:388)

run_with_workers and run both funnel through run_with_workers_report_seeds → run_with_workers_from_seed_report, which unconditionally calls self.set_seed(seed_report.root_seed). Before this PR, base_seed advanced via self.rng.next_u64() on each run; now it is re-derived from the same root seed every time.

Reproduced against this branch:

engine.set_seed(42);
let a = engine.run_with_workers(8, 2)?;   // base_seed = 15898102487349570925
let b = engine.run_with_workers(16, 2)?;  // base_seed = 15898102487349570925
// worker-0 shots of `b` are byte-identical to worker-0 shots of `a`

engine.run(20) twice likewise returns identical ShotVecs. The multi-batch usage documented in crates/pecos/examples/sim_api_final.rs (run(100), then run(500), then run_with_workers(1000, 8) on one engine) therefore accumulates duplicated samples: correlated statistics and underestimated variance, with no error or warning.

Suggested shape: keep the recording path drawing a fresh base_seed from self.rng, and let the replay path take the base seed from the report without touching engine state.

2. Replay mutates the caller's engine (engine.rs:369)

run_with_workers_from_seed_report(&mut self, ...) overwrites self.seed, reseeds self.rng, and reseeds hybrid_engine_template with the report's root_seed, and never restores them. Verified: an engine at set_seed(999) that replays a report recorded with root_seed = 42 has engine.seed == 42 afterwards.

The realistic path to trouble: a user loads an archived report to inspect a suspicious shot, then continues their sweep on the same engine — every later run reproduces the archived data. reset() restores self.seed, so it cannot recover the original stream either. Replay should either be &self (deriving worker seeds without mutating engine state) or save/restore around the call.

Boundary validation

A SeedReport comes from a JSON file on disk, so it is untrusted input; the replay path currently validates it with panics inside a Result-returning API.

• engine.rs:414 — assert!(seed_report.workers[i].worker_idx == item.0, "..") and the matching shots assert. A schema-valid report {num_shots: 10, num_workers: 2, workers: [{worker_idx: 0, shots: 7, ...}, {worker_idx: 1, shots: 3, ...}]} parses fine and then panics here with the message ... Across an FFI or server boundary this aborts instead of surfacing a recoverable error. These should return PecosError::Input with a message naming the mismatch.
• engine.rs:378 — assert!(num_shots > 0) / assert!(num_workers > 0) on deserialized values. The too-few-workers check five lines below already has the right shape (PecosError::Input); these should match it.
• engine.rs:380 — validation is asymmetric: workers.len() < num_workers errors cleanly, but a report with more worker records than num_workers passes this guard and panics later at line 414. workers.len() != num_workers would route both shapes through the same error path.
• engine.rs:393, :422 — num_shots flows straight into Vec::<(usize, usize, Shot)>::with_capacity(num_shots) and num_workers into ThreadPoolBuilder::num_threads. A corrupted report with "num_shots": 10000000000000 requests a multi-terabyte allocation before any shot runs; a large num_workers tries to spawn that many OS threads. Worth a sanity bound at load time.

Design

Recorded per-worker shots are ignored (engine.rs:386-387) — replay recomputes distribute_shots(num_shots, num_workers) and drives execution from the re-derived split; workers[i].shots is only asserted against, never used. That makes the report reproducible only while the distribution policy is unchanged: a report written by another build or policy, or hand-edited to reproduce one worker's shots, either trips the assert or runs a different split than the report documents. Driving replay from the recorded (worker_idx, shots, seed) records would make the report self-describing and remove the need for the assert entirely.

Packaging

• crates/pecos-engines/Cargo.toml:28 — tempfile is in [dependencies] but used only by tests/seed_report.rs. Every other crate in the workspace lists it under [dev-dependencies]. As written, pecos-engines and all its dependents ship tempfile and its rustix/getrandom/windows-sys chain as runtime dependencies.
• Cargo.lock — beyond the tempfile edge, the lockfile moves unrelated transitive dependencies backwards: unicode-width 0.2.2 → 0.1.14, windows-sys 0.61.2 → 0.52.0 in five places, getrandom 0.4.3 → 0.3.4. A clean re-resolve does not produce these, so it looks like regeneration under an older toolchain. Worth restoring the lockfile and re-adding only the tempfile edge.

Tests

to_json_file is the only new public write path and has no coverage — the tests exercise from_json_str, from_json_file, and the missing-file error, but never write a report and read it back. A save/load round-trip test would close that gap (and would justify the tempfile dependency, once it moves to [dev-dependencies]).

[ciaranra]

Anchoring three of the items from my earlier comment to the lines they apply to — same findings, no new ones.

[ciaranra]

This is the blocking item. Because run and run_with_workers both funnel through run_with_workers_from_seed_report, this line re-seeds the engine on every run, so consecutive runs on one engine return byte-identical shots and replaying an archived report overwrites the caller's seed.

Replay does not need it: each worker is seeded from the report at line 404, and nothing downstream reads self.rng or the template seed. Removing it restores base_seed = self.rng.next_u64() advancing per run, and makes replay non-mutating.

Suggested change

self.set_seed(seed_report.root_seed); // make sure to update root seed.

With this gone, the only remaining use of &mut self in this method is the template clone, so the signature can become &self — which turns "replay does not mutate the engine" into a compiler-checked property. Existing callers are unaffected.

[ciaranra]

Related to the same comment: the worker tuples are built from shots_per_worker (re-derived by distribute_shots on line 387) rather than from the records in the report, which is what forces the two assert!(.., "..")s below to police the redundancy — and those abort the process on a hand-edited or older-format report instead of returning PecosError::Input.

Driving replay from the records instead makes the report self-describing and lets the asserts, the distribute_shots call, and the one-sided workers.len() < num_workers check all go away:

let worker_engines: Vec<_> = seed_report
    .workers
    .iter()
    .map(|record| {
        let mut engine = self.hybrid_engine_template.clone();
        engine.set_seed(record.seed);
        (record.worker_idx, record.shots, engine)
    })
    .collect();

Then num_threads takes seed_report.workers.len(), and the Vec::with_capacity(num_shots) on line 393 can be a plain Vec::new() — a few reallocations are nothing next to running the shots, and a corrupted "num_shots": 10000000000000 no longer requests a multi-terabyte allocation before the first shot.

If the scalar fields should stay authoritative, one consistency check at the top of the method replaces all three panics:

if seed_report.workers.is_empty() {
    return Err(PecosError::Input(
        "Seed report contains no worker records".to_string(),
    ));
}
if seed_report.workers.len() != seed_report.num_workers {
    return Err(PecosError::Input(format!(
        "Seed report contains {} worker records, but num_workers is {}",
        seed_report.workers.len(),
        seed_report.num_workers
    )));
}

Either way the # Panics section on the doc comment can go.

[ciaranra]

tempfile is used only by tests/seed_report.rs, so as written it ships as a runtime dependency of pecos-engines and everything downstream (pecos, pecos-hugr, pecos-qasm, pecos-foreign, pecos-phir-json, benchmarks), pulling in its rustix/getrandom/windows-sys chain. Every other crate in the workspace lists it under [dev-dependencies].

Suggested change

tempfile.workspace = true

and add at the end of the file:

[dev-dependencies]
tempfile.workspace = true

[ciaranra]

Follow-up: I applied the suggestions from the inline comments in a scratch worktree at c8a9c721c and ran them, so the numbers below are measured rather than predicted.

The regression reproduces on the current head. Two tests, both failing before any change:

consecutive_runs_on_one_engine_are_not_identical
  base_seed did not advance between runs: 15898102487349570925

replay_does_not_mutate_caller_engine_stream
  replay perturbed the caller's RNG stream
  left: 1289665474627176275   right: 15898102487349570925

The second compares two engines seeded at 999: one runs twice, the other runs, replays an unrelated report, then runs again. The report's root_seed leaks into the second engine's stream.

With the fixes applied — drop the self.set_seed(...) line, drive worker_engines from seed_report.workers, replace the asserts with one PecosError::Input consistency check, Vec::new() instead of with_capacity(num_shots), num_threads(seed_report.workers.len()), &self instead of &mut self, and tempfile moved to [dev-dependencies]:

• cargo test -p pecos-engines: 17 test binaries, all pass, 0 failures. That includes the seven existing tests/seed_report.rs tests unchanged — both replay-reproduces-original tests still pass, so the replay guarantee is preserved.
• cargo clippy -p pecos-engines --all-targets (run cold): clean, no warnings.
• cargo check --all-targets on pecos, pecos-hugr, pecos-qasm, pecos-phir-json: all OK, so nothing downstream depended on the &mut self signature or on tempfile being a normal dependency.

Net diff: 28 insertions, 36 deletions across engine.rs, Cargo.toml, and two lines of tests/seed_report.rs.

Two things the change turns up that are worth knowing before you apply it:

1. Removing the whole # Panics doc section trips clippy's missing_panics_doc, because results_vec.lock().expect("results mutex poisoned") in the worker loop still panics. Keeping a one-line # Panics that names only the poisoned mutex clears it.
2. The &mut self → &self change makes the two let mut replay_engine = ... bindings in tests/seed_report.rs warn variable does not need to be mutable. Dropping mut in both is the whole fix.

Not verified on my side: the Cargo.lock re-resolve (re-resolving here would only reflect my toolchain), and a full cargo check --workspace, which fails in my environment on llvm-sys for want of a local LLVM — unrelated to this PR.