Skip to content

ENH: reproducible Monte Carlo via per-simulation-index seeding - #1054

Draft
thc1006 wants to merge 45 commits into
RocketPy-Team:developfrom
thc1006:enh/reproducible-montecarlo-seeding
Draft

ENH: reproducible Monte Carlo via per-simulation-index seeding#1054
thc1006 wants to merge 45 commits into
RocketPy-Team:developfrom
thc1006:enh/reproducible-montecarlo-seeding

Conversation

@thc1006

@thc1006 thc1006 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Scope, up front, because three reviews have now asked me not to claim things this does not claim.

This makes the sampled inputs reproducible: for a given root seed, simulation index i draws the same stochastic parameters whether the run is serial or parallel and however many workers it uses. That is what .inputs.txt records and what the tests compare.

It does not make the flown inputs or the trajectory reproducible, and it does not close #1053. What is still open is filed rather than folded in:

#1091 (Parachute pressure noise on the process-global np.random) was the other one. It is closed: #1134 gave Parachute a per-instance RNG and StochasticParachute now derives its seed from this tree.

#1109 was the third one, and it is fixed on develop now: #1122 made dict_generator walk the inputs a model declares rather than every attribute on it, so a tuple initial_solution is no longer read as a distribution. The issue is still open because a pull request into develop cannot close one. This branch dropped its own narrower guard, which walked the whole instance and skipped the component collections, since walking the declared inputs never saw them.


Pull request type

  • Code changes (bugfix, features)

Current behavior

MonteCarlo.simulate() seeds the stochastic models per worker in parallel mode (from a fresh, unseeded np.random.SeedSequence().spawn(n_workers)) and once at construction in serial mode. So the sampled inputs depend on the execution mode and the number of workers, and parallel runs are not reproducible run to run. This is #1053.

New behavior

Adds a keyword-only random_seed to simulate(). From that root, simulation index i is seeded from its own child of the root seed, derived before simulation i runs, so index i maps to the same seed no matter which worker runs it. The sampled inputs come out identical across serial, parallel(2) and parallel(N), and reproducible from the seed.

A few specifics:

  • O(1) per-index derivation. The child for index i is built by extending the root's spawn_key, which is exactly how SeedSequence.spawn derives it, so child(i) is bit-identical to root.spawn(number_of_simulations)[i]. Nothing pre-spawns a full list: a worker reconstructs any index from a small root state (entropy, spawn_key, pool_size, counter) that travels with the pickled instance, so nothing O(N) is sent to each process.
  • 128-bit int seeds. Each model is reseeded with a plain 128-bit int, not a SeedSequence. An int is the seed type numpy.random.default_rng and the stdlib random.Random both accept (a SeedSequence raises TypeError in random.Random since Python 3.11), so a custom sampler whose reset_seed documents an int keeps working. All four uint32 words are combined by value, so the seed is byte-order independent and keeps the full 128-bit pool rather than collapsing to 32 bits.
  • List-valued attributes are now seeded too. StochasticModel.dict_generator drew list attributes with the stdlib random.choice (an unseeded global instance), so random_seed did not govern them. It now draws the index from the model's own seeded generator, which also avoids numpy.random.choice coercing a heterogeneous list (Function, paths, arrays) to a single dtype.

random_seed is a seed, not a live RNG: it takes an int, a numpy integer, a sequence of ints, or a SeedSequence, with None = fresh entropy so existing behavior is unchanged unless you pass a seed. A supplied SeedSequence is copied from its full state before use, so it is never mutated and repeated calls with the same object reproduce the same run. This is informed by SPEC 7 and NumPy's parallel idiom, but keeps immutable seed-snapshot semantics rather than SPEC 7's stateful rng: a Generator/BitGenerator is not accepted, because reducing it to its underlying SeedSequence would ignore how far it has been consumed. Pass rng.bit_generator.seed_seq to seed from an existing generator.

Relation to #1071

#1071 targets the same issue. This PR takes the two ideas it got right, deriving each index's seed on demand instead of pre-spawning a list, and handing the samplers a plain int, and combines them with the parallel-claim lock below, the full 128-bit width (a single 32-bit word collides near 2**16 streams), the list-sampling fix, and cross-platform tests. Happy to reconcile the two however the maintainers prefer.

Notes from review

  • Parallel workers claimed the next index with an unlocked keep_simulating() + increment(). Near the end of a run two workers could both pass count < n and then both claim an index, running past the requested count. The claim now holds the shared mutex across the check and the increment, so each index is handed out once.
  • A supplied SeedSequence was returned as-is, and spawn() advances its child counter, so passing the same object twice was not reproducible. It is now copied from its full state, and Generator/BitGenerator are no longer accepted (see above).

Follow-up review round

A closer pass after the first reviews turned up four more fixes, all pushed here:

  • StochasticRocket._set_stochastic gave the same seed to the rocket body and to every surface, motor, rail button and parachute, so components that sample the same distribution (a main and a drogue parachute, for instance) drew identical cd_s and lag quantiles. Each component now gets its own child of the run's seed, in a fixed order, so they stay independent and reproducible.
  • dict_generator and StochasticRocket._randomize_position sampled list-valued attributes (component positions included) with the stdlib random.choice, which random_seed did not govern. Both now draw the index through the model's seeded generator, via a shared _random_choice helper.
  • simulate() set up (and, for append=False, truncated) the output files before the seed was validated, so passing a rejected seed destroyed a previous run's results. The seed is validated first now.
  • Corrected the seed helper's docstring about RandomState, and moved it to rocketpy.tools so the stochastic models can share it.

Known limitations and follow-ups

The 128-bit int does not fit the legacy numpy.random.RandomState, which caps seeds at 2**32 - 1. A custom sampler built on the modern default_rng (or the stdlib random.Random) takes it fine; one built on RandomState would need to reduce it. Since RandomState is the discouraged legacy path this felt like the right trade for keeping the full 128-bit decorrelation, but I am happy to revisit if you would rather cap the width.

Larger items from the review are better handled on their own, so they are filed separately rather than growing this PR:

Failure safety and log integrity

A later review round found several paths where a run that went wrong could still be reported as a success. Those are fixed here too, with tests:

  • The parent waited for every worker with an unbounded join(), in start order. One worker stuck in a native call held it there while another had already set the error event, so neither the error nor the cleanup after it was reached, and Ctrl-C hung on the same join a second time. The wait is bounded now and returns as soon as the event is set. Shutdown signals the whole fleet before waiting on any of it, with a kill fallback.
  • The completeness check accepted a corrupt file: unreadable rows were skipped, rows with no index or an index outside the run were ignored, and JSON true or 1.0 passed for index 1 because both compare equal to it. Every row now has to be an object with a plain non-negative int index, the two files have to agree on the exact set, and an interrupted run may be short but not corrupt.
  • Both run paths cleared the current payload after the interruptible call rather than before it. Serial Ctrl-C on the first lap surfaced as an UnboundLocalError over the interrupt, and between laps the handler still held the row that had just been written. In the worker, a claim that failed on a later lap reported the simulation that had just succeeded.
  • The normal write path released the mutex in finally whether or not acquire() had returned, so a manager that died during acquire raised a second error over the first.
  • The error record kept either the inputs or the traceback, never both.
  • n_workers was validated after the logs were opened "w+", so asking for a worker count the run cannot use destroyed the previous results on the way to raising.

Tests

Seed handling is unit tested in tests/unit/simulation/test_monte_carlo_determinism.py: accepted seed types, the SeedSequence copy preserving the full .state, the O(1) child equal to spawn bit-for-bit including a root whose counter has advanced and indices past 2**32, the 128-bit width, and the parallel index claim.

tests/integration/simulation/test_monte_carlo_determinism.py runs the real parallel path, no stub, under fork, spawn and forkserver, comparing serial against parallel(2) and parallel(4) per index. The fixtures are built so the properties can fail: the shared stochastic environment has zero wind at every altitude and zero times any factor is zero, so a compounding baseline cannot show up in it, and a bare StochasticAirBrakes gives every parameter a standard deviation of zero. The assertions check the eccentricities and the air brake are among the compared fields, or stripping object identity could quietly empty the comparison.

tests/unit/simulation/test_monte_carlo_log_integrity.py covers what the run is allowed to call a success and how the fleet comes down when it is not.

The thorough version of that test is marked slow and pull-request CI skips those, so it gated nothing. I said earlier that the weekly run would still cover it; that was wrong. Scheduled Tests triggers on schedule and on push to master, with no pull_request, so it only ever runs the default branch and never sees a test that is still on a PR. There is a small version of it now that is not marked, and because it takes the platform default each job ends up gating the start method it actually runs: spawn on Windows and macOS, forkserver on Python 3.14's POSIX default, fork below that. It costs about six seconds.

That turned out to be worth more than the argument for it. While fixing the shutdown timing below I reused a helper that sets the error event, and every successful parallel run started reporting itself as failed. The new gate caught it within seconds of being written, on a path the slow-marked version would not have run in CI at all.

Later review round

Three more from a closer look at the head, all fixed here:

  • The completeness check judged the whole file, so a duplicate, a torn row or a pair that disagrees anywhere in it failed the run. The documented way to resume is to interrupt a run and carry on with append=True, and an interrupted run is exactly what leaves that damage, so a file could be damaged once and never resumed. It judges only the indices this run claimed now, and reports rather than raises on what an earlier run left.
  • The parent terminated the fleet the moment a worker reported an error, so a worker part way through a write was cut off. Measured at 0.0 ms against the 5000 ms the interrupt path already gave. The shutdown was producing the torn rows the check then reported. Both paths give the same window now.
  • A torn row also makes the two files disagree, and the cross-file check ran first, so the error named the symptom rather than the cause. Reordered.

Checkpoint validation

The resume point came from a line count rather than from the indices on disk, and the completeness check trusted it. A blank line makes the two disagree:

two rows                  -> num_of_loaded_sims = 2
two rows + one blank line -> 3
two rows + two blanks     -> 4      (the file holds only 0 and 1 either way)

So the next run starts at 2, index 1 is never written, and a check scoped to the new range reports success. Appending now reads both logs first and refuses unless what it finds is the run it is being asked to continue: every row readable, no index twice, both files holding the same set, and the indices forming exactly the range below the resume point. Nothing is opened for writing until that passes. A run that was not interrupted is then held to the whole range rather than to its own share of it.

A file numbered from 1 is named rather than reported as an off-by-one, since serial runs used to be numbered that way and the answer is to re-baseline.

A file with a hole in it is refused rather than repaired. Filling holes needs workers to claim from a plan instead of counting on from the end, which is
#1075. Until then, refusing loudly beats resuming in the wrong place quietly. The manifest settles which generation wrote the rows and how many there were; it does not decide which index to run next.

The test that used to empty both logs and then assert a four-simulation result holding only indices 2 and 3 was a success is gone. That was the shape of the bug rather than a guard against it.

Two other ways a previous run could be lost:

  • multiprocess is an optional extra and was imported inside the parallel path, which runs after both logs have been emptied. An install without rocketpy[monte-carlo] lost its results on the way to the ImportError.
  • The Generator rejection advised rng.bit_generator.seed_seq, which NumPy grew in 1.25 while this package declared numpy>=1.13, so the advice raised AttributeError on versions it claimed to support. The floor moves to 1.17, which default_rng has needed all along.

Also: the custom sampler fixture built a Generator in reset_seed and dropped it while sample() drew from the process-global np.random, so nothing in it answered to a seed and the 128-bit path went untested.

Round after the review above

Everything here came out of a reading of the append protocol, and each one was reproduced before it was fixed.

A seed that is an array crashed the lineage check. numpy.random.SeedSequence takes array_like[ints], so entropy can be an ndarray, and comparing two roots holding one answers with an array rather than a verdict: ValueError: The truth value of an array with more than one element is ambiguous. [1, 2, 3] and (1, 2, 3) are the same seed and compared unequal, which warned about a lineage nobody had left. Roots are compared by a canonical fingerprint of the words they generate; n_children_spawned stays in it because __child_seed counts from there. Only int, None and a different int had been covered.

A warning was not enough, and the lineage moved before the files did. One manifest holds one root, so appending from another left the file with two lineages while the manifest named only the newer half: the provenance it reported was wrong rather than incomplete, and my own test wrote that down as correct. An append now continues the root the logs already hold. Leaving random_seed out means continue, a matching seed is allowed, and a different one is refused before anything opens a file, so a file can no longer be given two lineages at all.

A checkpoint cannot be recognised by its indices. The previous release numbered parallel runs from 0 as well, so a clean one of those passed every structural check while its rows came from per-worker entropy, shared component seeds and a different sampling order. Each run now writes a manifest beside its output log naming the schema version, the sampling scheme, the root, a run_id, a committed_count and the two log filenames. It is staged, fsynced and moved into place, and an append validates the whole document: a non-empty checkpoint with no manifest, one that parses but describes no rebuildable root, or one whose seed_chosen is the string "false" rather than a boolean, are all refused. The manifest outlives the object, so the check survives a fresh interpreter.

Three atomic renames are not one transaction. Staging the empty logs first only covered a staging failure. os.replace is atomic per file, so a failure on the second left the first already emptied, the untouched temporaries behind, and the destination narrowed from 0644 to the 0600 a staged file opens at. Each destination is moved aside before its replacement goes in, anything already installed is put back if a later one fails, and the mode is carried across. Every one of the six renames is driven to fail in the tests, plus a KeyboardInterrupt. It is still not a filesystem transaction and the docstring says so.

A bool is an int. simulate refuses one through _is_whole_number; simulate_convergence checked with a plain isinstance, so batch_size=True ran one simulation. tolerance had the same hole.

A row that stopped said nothing about how. A simulation that raised carried error; one dropped because a peer crashed or the user interrupted carried nothing, so it read as a simulation that simply had no error. Those now carry a status.

An early failure lost the inputs. The row is only assembled once the flight is, so anything raising inside create_object or Flight itself left a traceback with nothing about the inputs behind it. dict_generator builds a local dictionary and binds last_rnd_dict once, after its whole loop, so recovery is per model rather than per field: a model that finished publishes, one that raised part way through its own draw does not. Both failure paths recover what was published and mark the row partial. Telling a fresh publication from the previous simulation's is done by keeping a reference to what each model held at seeding, so nothing clears last_rnd_dict, which dict_generator documents as the last generated dictionary.

The manifest recorded a target, not an outcome. It moved on the number of simulations asked for, so a Ctrl-C before the first new row still moved it while the logs stayed where they were, and a target that was never reached claimed rows that are not there. A run opens a generation once its logs exist, so a fresh set belongs to its root even if the first row never lands, and an append stays in the generation it continues. The count is taken from the rows themselves, after the completeness check, and best effort: the run has finished and the rows are on disk, so a count that cannot be taken leaves the previous one for the next append to refuse on.

Breaking change

  • Yes

The exact numbers a run produces change (per-index seeding, the env/rocket/flight split and the per-component split within a rocket, the 128-bit int seeds, the serial index now counting from 0 to match parallel, and list-valued attributes and positions now sampled through the seeded generator), so external code that pinned exact Monte Carlo samples would need to re-baseline. The in-repo Monte Carlo tests do not pin exact values (test_monte_carlo_simulate checks apogee and impact velocity within a tolerance and still passes), and random_seed is opt-in.

Partially addresses #1053. The per-index seeding for serial and parallel runs is here. The log not matching the flight is #1090 and belongs to #1126, simulate_convergence is #1077, and what is left of #1075 is claiming indices from a plan. I would rather leave #1053 open until those land than close it on a guarantee that does not cover them.

What this PR does and does not promise

The guarantee here is over the sampled inputs: for a given root seed, simulation index i draws the same stochastic parameters whether the run is serial or parallel and however many workers it uses. That is what .inputs.txt records and what the tests compare.

It is deliberately not a guarantee about the whole trajectory yet. The gaps are filed rather than grown into this PR:

#1091, Parachute pressure noise drawn from the process-global np.random, was
open when this PR was written. It is closed now: #1134 gave Parachute a per-instance RNG and StochasticParachute feeds it a seed derived from this tree, so parachute deployment is inside the guarantee.

Until those land, two runs agreeing on .inputs.txt does not prove they flew the same thing. Once they do, the promise can be restated in terms of results.

@thc1006
thc1006 marked this pull request as ready for review July 8, 2026 19:35
@thc1006
thc1006 requested a review from a team as a code owner July 8, 2026 19:35
Copilot AI review requested due to automatic review settings July 8, 2026 19:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.40719% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.76%. Comparing base (6c15649) to head (cf99e87).

Files with missing lines Patch % Lines
rocketpy/simulation/monte_carlo.py 96.00% 18 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1054      +/-   ##
===========================================
+ Coverage    84.33%   85.76%   +1.43%     
===========================================
  Files          130      130              
  Lines        17258    17673     +415     
===========================================
+ Hits         14554    15158     +604     
+ Misses        2704     2515     -189     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@thc1006

thc1006 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

The seeding logic is unit-tested in tests/unit/simulation/test_monte_carlo_determinism.py: __root_seed_sequence accepts an int, a sequence of ints, or a SeedSequence (copied from its full state so the caller's object is not mutated and repeated calls reproduce) and rejects a stateful Generator/BitGenerator; __seed_simulation splits each child seed three ways; and _claim_next_index (the atomic index claim from the race fix) has a deterministic barrier-based test that over-claims and fails if the lock is removed. End-to-end reproducibility (serial, and serial == parallel) lives in tests/integration/, with only the fork-based worker-invariance test marked slow.

The lines codecov still shows uncovered are all in the parallel path: simulate's parallel=True dispatch, the worker setup in __run_in_parallel, and the __sim_producer loop. The coverage jobs cannot reach them because parallel=True is only exercised by the slow worker-invariance test (the jobs do not pass --runslow), and the producer body runs in forked worker processes that coverage.py does not instrument without concurrency = multiprocessing. The behavior is covered by the slow determinism and test_monte_carlo_simulate[parallel] tests, and the claim logic by the fast unit test above. Glad to set up multiprocessing coverage separately if you want the parallel path counted, but that felt out of scope for this PR.

@Gui-FernandesBR
Gui-FernandesBR force-pushed the enh/reproducible-montecarlo-seeding branch from 0d37ed6 to 761c092 Compare July 9, 2026 21:08

@phmbressan phmbressan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation is very clear and throughout, nice work.

The explanation on the concepts behind per index seeding (both in the issue and PR description) were rather helpful. I agree having reproducible results was an issue with the parallel per worker seeding.

Regarding the decisions on parameter naming, I agree with most of the decisions taken here. Moreover, the rng attribute is well docstringed, so it shouldn't be a matter of confusion to the user.

@MateusStano could you give your two cents on the changes here before we proceed with a merge?

Comment thread tests/integration/simulation/test_monte_carlo_determinism.py
Comment thread rocketpy/simulation/monte_carlo.py Outdated
Comment thread rocketpy/simulation/monte_carlo.py Outdated
thc1006 added a commit to thc1006/RocketPy that referenced this pull request Jul 11, 2026
Addresses review feedback on RocketPy-Team#1054.

Parallel workers claimed the next index with an unlocked
keep_simulating() + increment(), so near the end of a run two workers
could both pass the count < n check and then claim sim_idx == n; the
per-index child_seeds lookup turned that into an IndexError (before, it
only wrote one extra record). Move the claim into a _claim_next_index
helper that holds the shared mutex across the check and the increment,
so each index is handed out once and the counter never overshoots. A
deterministic unit test (a barrier plus a widened check-to-increment
window) over-claims and fails if the lock is dropped.

__root_seed_sequence returned the caller's SeedSequence, and spawn()
advances its child counter, so passing the same object to simulate()
twice produced different children. Copy it from its full state instead,
which leaves the caller untouched and keeps repeated calls reproducible.
Also drop Generator/BitGenerator from the accepted types: a stateful
generator is not a seed, and reducing it to its underlying SeedSequence
ignores how far it has been consumed. random_seed now takes an int, a
sequence of ints, or a SeedSequence.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@thc1006
thc1006 requested a review from MateusStano July 11, 2026 01:18
thc1006 added a commit to thc1006/RocketPy that referenced this pull request Jul 11, 2026
Addresses review feedback on RocketPy-Team#1054.

Parallel workers claimed the next index with an unlocked
keep_simulating() + increment(), so near the end of a run two workers
could both pass the count < n check and then claim sim_idx == n; the
per-index child_seeds lookup turned that into an IndexError (before, it
only wrote one extra record). Move the claim into a _claim_next_index
helper that holds the shared mutex across the check and the increment,
so each index is handed out once and the counter never overshoots. A
deterministic unit test (a barrier plus a widened check-to-increment
window) over-claims and fails if the lock is dropped.

__root_seed_sequence returned the caller's SeedSequence, and spawn()
advances its child counter, so passing the same object to simulate()
twice produced different children. Copy it from its full state instead,
which leaves the caller untouched and keeps repeated calls reproducible.
Also drop Generator/BitGenerator from the accepted types: a stateful
generator is not a seed, and reducing it to its underlying SeedSequence
ignores how far it has been consumed. random_seed now takes an int, a
sequence of ints, or a SeedSequence.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@thc1006
thc1006 force-pushed the enh/reproducible-montecarlo-seeding branch from 9c020b6 to 3e22729 Compare July 11, 2026 06:38
thc1006 added a commit to thc1006/RocketPy that referenced this pull request Jul 18, 2026
Addresses review feedback on RocketPy-Team#1054.

Parallel workers claimed the next index with an unlocked
keep_simulating() + increment(), so near the end of a run two workers
could both pass the count < n check and then claim sim_idx == n; the
per-index child_seeds lookup turned that into an IndexError (before, it
only wrote one extra record). Move the claim into a _claim_next_index
helper that holds the shared mutex across the check and the increment,
so each index is handed out once and the counter never overshoots. A
deterministic unit test (a barrier plus a widened check-to-increment
window) over-claims and fails if the lock is dropped.

__root_seed_sequence returned the caller's SeedSequence, and spawn()
advances its child counter, so passing the same object to simulate()
twice produced different children. Copy it from its full state instead,
which leaves the caller untouched and keeps repeated calls reproducible.
Also drop Generator/BitGenerator from the accepted types: a stateful
generator is not a seed, and reducing it to its underlying SeedSequence
ignores how far it has been consumed. random_seed now takes an int, a
sequence of ints, or a SeedSequence.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@thc1006
thc1006 force-pushed the enh/reproducible-montecarlo-seeding branch from 3e22729 to 6bf8bb6 Compare July 18, 2026 21:06
@thc1006

thc1006 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

@MateusStano friendly ping when you have a moment. Both points from your last pass are addressed: the parallel index claim now holds the shared mutex across the check-and-increment (with a deterministic test that goes red if the lock is removed), and a supplied SeedSequence is copied from its full state before spawning, so repeated calls reproduce and the caller is left untouched. I replied inline on both threads. The test matrix and lint pass on the current head; the only red is the soft codecov patch check, which is the parallel-only lines I covered in the thread above. Whenever you get a chance to take another look, I would appreciate it.

@thc1006
thc1006 force-pushed the enh/reproducible-montecarlo-seeding branch from 6bf8bb6 to c529d0a Compare July 20, 2026 06:09
thc1006 added a commit to thc1006/RocketPy that referenced this pull request Jul 20, 2026
Addresses review feedback on RocketPy-Team#1054.

Parallel workers claimed the next index with an unlocked
keep_simulating() + increment(), so near the end of a run two workers
could both pass the count < n check and then claim sim_idx == n; the
per-index child_seeds lookup turned that into an IndexError (before, it
only wrote one extra record). Move the claim into a _claim_next_index
helper that holds the shared mutex across the check and the increment,
so each index is handed out once and the counter never overshoots. A
deterministic unit test (a barrier plus a widened check-to-increment
window) over-claims and fails if the lock is dropped.

__root_seed_sequence returned the caller's SeedSequence, and spawn()
advances its child counter, so passing the same object to simulate()
twice produced different children. Copy it from its full state instead,
which leaves the caller untouched and keeps repeated calls reproducible.
Also drop Generator/BitGenerator from the accepted types: a stateful
generator is not a seed, and reducing it to its underlying SeedSequence
ignores how far it has been consumed. random_seed now takes an int, a
sequence of ints, or a SeedSequence.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@thc1006

thc1006 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Heads up that this changed enough since the last look to be worth a fresh pass rather than merging on the earlier approval. @MateusStano @phmbressan when you have a moment.

What is new since the review:

  • Per-index child seeds are now derived in O(1) by extending the root spawn_key (bit-identical to spawn(n)[i]) instead of pre-spawning the whole list, so nothing O(N) is pickled to each worker.
  • The samplers get a plain 128-bit int rather than a SeedSequence, so a custom sampler's int-typed reset_seed keeps working (a SeedSequence raises TypeError in random.Random since 3.11). The int combines all four words by value, so it is byte-order independent.
  • List-valued stochastic attributes now draw from the model's seeded generator, so random_seed governs them too. That closes the gap the previous description called out as a known limitation.
  • Added a start-method-invariance test that runs under fork, spawn and forkserver in ordinary CI, since 3.14 moved the POSIX default to forkserver.

Both earlier concerns are still handled: the parallel claim holds the mutex across the check and the increment, and a supplied SeedSequence is copied from its full state. #1071 opened for the same issue in the meantime; the description notes how this relates and what it borrows. A re-review whenever you get the chance would be appreciated.

@thc1006

thc1006 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

@MateusStano @phmbressan a follow-up pass turned up a few more things worth fixing, so I have pushed them and would appreciate another look when you have time.

Since your reviews:

  • Component seeds: StochasticRocket._set_stochastic handed the same seed to the body and to every surface, motor, rail button and parachute, so a main and a drogue parachute drew the same cd_s and lag quantiles. Each component now gets its own child of the run's seed.
  • List sampling: dict_generator and _randomize_position sampled list-valued attributes (component positions among them) with the stdlib random.choice, which random_seed did not control. Both now draw through the model's seeded generator.
  • File safety: simulate() truncated the output files before the seed was validated, so a rejected seed destroyed a previous run's results. Validation runs first now.
  • Docs: corrected the seed helper's note about RandomState, since a 128-bit int does not fit its 32-bit seed.

I also marked the earlier threads resolved. The race and the SeedSequence copy are both fixed in the current code, and the dangling-files question checked out: the run writes only under tmp_path.

A few larger items from the same review are better as their own issues, so I opened #1075 (append continuation), #1076 (a full parallel test under spawn and forkserver) and #1077 (a seed for simulate_convergence), and linked them from the description. Thanks for the careful reviews.

@thc1006
thc1006 force-pushed the enh/reproducible-montecarlo-seeding branch from 5a9c119 to da2ba5c Compare July 20, 2026 08:45
@thc1006

thc1006 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

A quick note on the red CI here, so it isn't mistaken for a regression from this change: the failing jobs crash in test_flight_animation_export_gif (a VTK/PyVista off-screen GIF export) with Fatal Python error: Bus error. The same crash, at the same file and line, also hit develop's own Tests run a day ago (https://github.com/RocketPy-Team/RocketPy/actions/runs/29697106388), so it looks like a pre-existing flake in the off-screen rendering tests rather than anything this PR introduced. I checked the dependency set too: the green and red runs installed identical vtk, pyvista, matplotlib and pillow versions, and nothing in this branch touches the plotting or animation code.

Re-running usually clears it. Happy to help look at the flaky animation tests on their own if that would be useful.

Filed #1078 to track the flaky animation tests.

thc1006 added 15 commits August 15, 2026 15:42
`index` is what pairs an inputs row with its outputs row, and the custom fields
were merged over it, so a collector supplying one won:

    data_collector={"index": callback}

A collector returning a constant writes a row the completeness check rejects.
One returning a permutation does not:

    sim_idx 0 -> index 1
    sim_idx 1 -> index 0

    indices {0, 1}, each once, against a run of 2

Every check downstream compares the index multiset, so it sees a complete run
and reports success while the outputs sit on the wrong simulations. That is
worse than a corrupt row, which at least announces itself.

Three places, because one is not enough:

- `index` is a reserved key now, and collector keys have to be strings. A dict
  key can be anything hashable, and a non-string one would not survive the JSON
  round trip that reads these files back.
- `simulate()` checks again. The attribute is public and mutable, so a key
  added after construction would otherwise reach the logs unchecked. It runs
  before `__setup_files`, so a rejected run leaves the previous one intact.
- the run's own index is written after the custom fields rather than before.

Four tests. One of them exists to say why the first matters: a permutation
passes every other check, so a test asserting only that the row is malformed
would not have caught this.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Two boundaries on `random_seed` that the docstring implied more of than it
should. Both were raised on review.

What a seed reproduces is the sampled values. With `include_function_data=True`
a record also carries a `Function`'s signature hash and serialised source,
which describe the object rather than the value drawn for it, so a run under
spawn or forkserver writes different ones for the same inputs. The
cross-start-method test measured six such fields and filters exactly them.

And it is scoped to one environment. NumPy promises a stream only for the same
BitGenerator, seed, call sequence, build and machine, and reserves the right to
change what `default_rng` returns. A seed fixes the lineage of a run; it is not
an archive format that survives a version bump.

Also moves two imports in test_custom_sampler.py to the top of the file. They
arrived on develop with the cherry-pick of 23be0ba, which was the version
before that fix, and pylint exits 16 on them. RocketPy-Team#1111 does the same thing as part
of a wider change; this is here because it is what turns this branch's lint red.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The seed fixes the sampled inputs, not the flight that follows from them.
Randomness no stochastic model owns is outside the tree: a Sensor left on
seed=None takes fresh entropy per instance, and MultivariateRejectionSampler
draws from the stdlib random module. Both are seedable by the caller, so say
so rather than leaving the guarantee sounding wider than it is.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The previous wording reached for two examples that do not hold. Sensor noise
cannot affect a Monte Carlo flight because StochasticRocket.create_object does
not carry sensors onto the rocket it builds, and MultivariateRejectionSampler
resamples finished result files rather than running inside a flight.

The real gaps are already filed: the flight dictionary is drawn more than once
(RocketPy-Team#1090), append does not carry its seed lineage (RocketPy-Team#1075), and
simulate_convergence seeds neither its batches nor its bootstrap (RocketPy-Team#1077). Name
those, and say a convergence study is outside the guarantee.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
… still

Three gaps behind the reproducibility this adds.

A run seeded 42 continued with append=True and no seed silently starts a new
lineage, and the file it produces is valid on every structural check: the
indices stay unique and contiguous and the two logs stay in step. Nothing but
the roots can tell, so the capture now compares them and warns. Only within one
object; carrying the root in the files is RocketPy-Team#1075.

_nominal called itself a construction-time snapshot but cached the attribute by
reference, so writing through the wrapped object moved it while rebinding the
attribute did not. Containers are copied on the way in; anything else is held by
reference and the docstring now says which.

__setup_files truncated the three logs one after another, so a bad path or a
permission on the second emptied the first on the way to raising. They are
staged beside their destinations and moved into place once all three exist.

The 'not synchronized' warning was unreachable, guarded on not append inside the
branch that only runs when appending. It is now reachable on the append path,
which is where it was meant to fire.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
simulate() rejects True through _is_whole_number, because isinstance(True, int)
holds and a bool would quietly run one simulation. simulate_convergence checked
with a plain isinstance, so batch_size=True and max_simulations=True were read
as 1. tolerance had the same hole against isinstance(x, (int, float)).

A row in the error file carried an error when a simulation raised and nothing at
all when it was dropped because a peer crashed or the user interrupted, so the
last two read as a simulation that simply had no error. They now carry a status
of cancelled or interrupted. An empty payload still writes nothing: an interrupt
between two simulations has nothing to report, and a row saying otherwise is
what test_ctrl_c_between_rows_does_not_report_the_row_that_succeeded exists to
catch.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Each argument called dict_generator again and kept one key, so the flight took
rail length from the first draw, inclination from the second and heading from
the third, while last_rnd_dict, and so the row written to .inputs.txt, held only
the third. Measured on the shared fixture: logged inclination 85.60, flown
84.46. Addresses RocketPy-Team#1090.

One draw now serves all three, which makes the row a record of what flew rather
than of a fourth thing nobody used. The three models draw from independent
streams, so moving the flight draw ahead of the Flight call does not disturb
what the environment or the rocket sample.

StochasticRocket.create_object also now says what it carries onto the rocket it
builds. Sensors are not among them, so a sensor on the wrapped rocket cannot
reach a Monte Carlo flight.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Three holes in the append protocol, all of them invisible to a reader of the
rows.

A seed can be an ndarray, which numpy.random.SeedSequence accepts, and
comparing two roots holding one answered with an array rather than a verdict:
ValueError, the truth value of an array with more than one element is
ambiguous. [1, 2, 3] and (1, 2, 3) are the same seed and compared unequal. Roots
are now compared by a canonical fingerprint of the words they generate, with
n_children_spawned kept because __child_seed counts from it.

The lineage moved as soon as a root was captured, before any file was touched,
so a run that warned and then added nothing left the object believing the new
root had landed. The append after that one silently put its rows behind rows
from the first. Capturing and committing are now separate, and only a run that
added rows commits.

A checkpoint from before per-index seeding cannot be recognised by its indices:
the previous release numbered parallel runs from 0 as well, so a clean one of
those passed every structural check while its rows came from per-worker
entropy, shared component seeds and a different sampling order. Each run now
writes a manifest beside its output log naming the schema version, the sampling
scheme and the root, and an append refuses a non-empty checkpoint that has
none. The manifest also outlives the object, so the lineage check survives a
fresh interpreter, which the attributes alone could not do.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
os.replace is atomic for one file. Three of them were three atomic steps with
nothing between, so a failure on the second left the first already replaced by
an empty file, the untouched temporaries behind, and the destination narrowed
from 0644 to the 0600 a staged file opens at.

Each destination is now moved aside before its replacement goes in, anything
already installed is put back if a later one fails, the temporaries that never
landed are removed, and the mode of the log being replaced is carried onto the
file replacing it. BaseException, so a Ctrl-C rolls back too.

This is not a filesystem transaction and the docstring says so: a generation
directory swapped by one pointer would be the stronger guarantee, and would
change what the three public log paths mean.

Also drops the RocketPy-Team#1090 fix from this branch. RocketPy-Team#1126 is open against the same
function, adds the shared _sample_flight_inputs the fix wants, and closes the
second draw inside StochasticFlight.create_object that this branch left alone.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The row is only assembled once the flight is built, so anything raising inside
create_object or Flight itself left an error row carrying a traceback and
nothing about the inputs that produced it. A tuple initial_solution (RocketPy-Team#1109) gets
there on the first draw.

Each stochastic model fills its own last_rnd_dict as it goes, so what did get
drawn is already on hand. Both failure paths now recover it and mark the row
partial: the draws that never happened are absent rather than recorded as null.

That only reads true if those dicts hold the simulation in flight and nothing
else. Reseeding clears them, since a worker keeps its models across every index
it claims, and recording a pair clears them too, so a row already on disk cannot
be recovered again and blamed for a later failure.

_inputs_drawn_so_far is module level for the reason _record_simulation is: the
run paths are driven by stubs in the tests, which carry no private methods. The
stubs gained the four attributes the failure path now reads.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Windows honours only the read-only bit, so a file asked for 0644 reports 0666
there and the literal was testing the platform rather than the carry-over. Both
Windows legs went red on it. Reading the mode before the replacement and
comparing after says the same thing on either platform, and dropping the chmod
still turns it red.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
_record_simulation forgets the draw once the rows are on disk, and did it in a
way that could raise. A model without last_rnd_dict then took a simulation whose
inputs and outputs had just been written and reported it as a failure, with an
error row of its own. Best effort now: the write is the outcome, and the
bookkeeping after it says so rather than replacing it.

The manifest is named by appending rather than by replacing the suffix, so
run.txt and run.json stop describing themselves with one file.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Two things I got wrong in the previous commit, both found by review.

dict_generator builds a local dictionary and binds last_rnd_dict once, after
its whole attribute loop. It does not fill it as it goes, which is what I
claimed. So a model that raises part way through its own draw publishes
nothing, and the recovery is per model, not per field: the tuple
initial_solution in RocketPy-Team#1109 recovers the models that finished and nothing from
the one that failed. Two tests now drive a real StochasticEnvironment through a
sampler that raises mid-loop, rather than pre-filling last_rnd_dict on a stub
and proving only that the serializer works.

Telling a fresh publication from the previous simulation's was done by clearing
last_rnd_dict, which dict_generator documents as holding the last generated
dictionary. Seeding now keeps a reference to what each model was already
holding, and a model still holding it published nothing for this index. Nothing
clears public state, and _record_simulation goes back to writing the pair and
stopping there.

Also removes run.outputs.manifest.json and run.outputs.txt.manifest.json, which
were test output committed from the repository root: the deterministic helper
used fixed run.* paths in the working directory instead of tmp_path.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
One manifest holds one root. Warning and appending anyway left the file with
two lineages while the manifest named only the newer half, so the provenance it
reported was wrong rather than merely incomplete, and my own test wrote that
down as correct: after appending root 7 behind root 42 it called the next
append with root 7 'genuinely the same lineage', which the rows from 42 say it
is not.

An append now continues the root the logs already hold. Leaving random_seed out
means continue rather than begin, an explicit seed that matches is allowed, and
one that does not is refused before anything opens a file. A file can no longer
be given two lineages, so the manifest cannot misdescribe one.

The manifest is also validated in full rather than by scheme and version alone.
A document naming the right scheme with no usable root_state used to pass, and
bool("false") is True, so a string there read as a chosen seed. It is written
through a staged file with fsync and os.replace, since it now decides whether a
checkpoint may be continued at all.

The two committed-fingerprint attributes are gone: the manifest is the record,
and pylint pointed out that nothing read them any more.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
RocketPy-Team#1122 made dict_generator walk the declared stochastic inputs rather than the
whole instance, which is the right fix for RocketPy-Team#1109 and makes the collection-skip
this branch carried redundant. add_cp_eccentricity and add_thrust_eccentricity
run after __init__ has already built that list, so their values stopped being
sampled: four eccentricities became none.

They are declared as they are validated now. The component_collections
mechanism is gone, since walking the declared inputs never saw the collections
in the first place.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@thc1006
thc1006 force-pushed the enh/reproducible-montecarlo-seeding branch from 0f48613 to 81d3e58 Compare August 15, 2026 07:50
The manifest moved on the number of simulations asked for. A Ctrl-C before the
first new row still moved it, since the target was above the checkpoint, and a
target that was never reached claimed rows that are not there.

A run now opens a generation once its logs exist, so a fresh set belongs to its
root even if the first row is never written, and an append stays in the
generation it continues. The count is taken from the rows themselves, after the
completeness check rather than before it, and best effort: the run has finished
and the rows are on disk, so a count that cannot be taken leaves the previous
one for the next append to refuse on.

The manifest also says which run and which logs it describes. run_id,
committed_count and the two log names are recorded and required, so a manifest
that survives beside a different pair, or one hand-edited into something that
parses, is refused rather than trusted.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@Gui-FernandesBR

Copy link
Copy Markdown
Member

This is a quite large PR... I wonder if it would be easy and possible to split it into smaller PRs... If if becomes a too hard task, no problem, just give us more time so we can properly review it

@thc1006

thc1006 commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

TL;DR: splitting it. I am putting this back to draft now so nobody spends review time on the current head. Nothing gets thrown away, and I will link the series here before I rewrite the branch.

Thanks for saying it, and thanks for offering the alternative of just taking more time. Splitting is the better answer. You were right to ask, and I should have stopped adding to this head sooner.

One piece of good news about the size: a fair amount of what accumulated here has landed separately while this sat. The seeded list choice, the per-sampler CustomSampler streams, and the eccentricity declarations from #1167 and #1168 are all on develop now. I checked each against develop rather than assuming, so the split is smaller than 45 commits makes it look.

What I plan to lift out, each based on current develop, each green and revertible on its own:

#1054 then keeps only the random_seed API, the per-index child derivation, and the tests that cover those directly. I will leave #1126 where it is rather than duplicate it.

@ting-hong-shieh thanks for checking the head and reporting the conflict against develop; that saved me a step, and the rebase falls out of the split anyway.

I will ask for review on one at a time rather than sending five at once. No action needed from either of you until the first one is up.

@thc1006

thc1006 commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Two of the split are up and green: #1169, a stable nominal across reseeding, and #1170, one random stream per rocket component.

The series, which I will fill in as each goes up:

Neither of the two depends on the other, and they have been merged into a throwaway tree on develop and run there rather than trusted separately. #1169 is the smaller if only one gets a look. Two more issues came out of the work and are filed rather than folded in: #1171 on removing a declared eccentricity, and #1172 on one component wrapper being stored twice.

The head being split from is kept at thc1006/RocketPy@backup/pr-1054-cf99e872, so nothing here goes missing when this branch is rewritten.

thc1006 added a commit to thc1006/RocketPy that referenced this pull request Aug 16, 2026
_set_stochastic re-validates every declared input, and validation reads the
nominal off the wrapped object. create_object writes the sampled value back
onto that same object on purpose, so re-reading it on a reseed took one
simulation's output as the next one's nominal: a wind factor compounded
10 -> 8.576 -> 7.355 -> 6.308 under a single fixed seed, and a plain scalar
spec drifted the same way.

Read the nominal once and keep it. Containers are copied on the way in, so
writing through the wrapped object cannot reach it either. A component
position arrives through an injected getter, reads an attribute nothing
writes back to, and shares one name across every component, so those are
read live rather than cached.

Extracted from RocketPy-Team#1054.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
thc1006 added a commit to thc1006/RocketPy that referenced this pull request Aug 16, 2026
_set_stochastic handed the same seed to the rocket body and to every surface,
motor, rail button and parachute, so two components built from one spec drew
identical values: a main and a drogue with the same cd_s and lag spec drew the
same cd_s and the same lag, every time, and a study of both was a study of one
counted twice.

Air brakes were worse. They are built and sampled in create_object and were
not in the reseed at all, so their values came from wherever the generator had
been left rather than from the seed: 0.683, then 0.586, then 0.488 for one
seed asked three times.

Each component now takes its own child of a SeedSequence root, spawned in a
fixed order so one seed still reproduces the whole rocket. The collections are
named in one place and checked against create_object's own source, since the
collection no fixture populates is the one that gets missed.

Extracted from RocketPy-Team#1054.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
ting-hong-shieh added a commit to ting-hong-shieh/RocketPy that referenced this pull request Aug 16, 2026
simulate() returned normally after Ctrl-C. Both execution modes caught the
interrupt and neither re-raised it, so simulate() went on to
__terminate_simulation() and returned exactly as it does after a complete
study. A caller could not tell a partial run from a finished one without
opening the output file and counting rows.

Three more holes sat next to that one, found in review:

- __run_in_serial bound inputs_json inside the loop body, so Ctrl-C during
  the first keep_simulating() call reached the handler with nothing bound
  and the run died with UnboundLocalError from inside the cleanup.
- _append_simulation_record rolled its inputs row back on Exception, and
  KeyboardInterrupt does not derive from Exception, so Ctrl-C between the
  two appends left a one-sided record for the reload to disagree with.
- The parallel cleanup began only after every worker had started, so
  Ctrl-C during the startup loop left the already-started workers running
  with nobody signalling or joining them.

Bind inputs_json before the loop, roll back on BaseException, cover the
startup loop with the same cleanup path over a started-workers list, and
re-raise in both handlers. Catch the interrupt in simulate() so
__terminate_simulation() still runs before it leaves: it reloads the logs
through the file setters, and set_num_of_loaded_sims is what the
documented append=True continuation reads.

The shutdown join stays unbounded, and the docstring now says so: the
interrupt propagates once every worker finishes the simulation it is in
and exits on its own. A worker stuck inside one simulation blocks the
interrupt as it already blocked the run; killing it here could tear a
half-written row into logs it holds the mutex for, and the bounded fleet
shutdown belongs to RocketPy-Team#1054. The ordinary exception path is unchanged.

Add eight regression tests covering both modes, the early interrupt, the
preserved rows, the reload, an interrupted run continued with append=True,
the between-appends rollback, and the startup-loop cleanup. They run the
real __terminate_simulation and assert the state it produces; all eight
fail on develop and each one fails again when only its own fix is
reverted.
ting-hong-shieh added a commit to ting-hong-shieh/RocketPy that referenced this pull request Aug 16, 2026
simulate() returned normally after Ctrl-C. Both execution modes caught the
interrupt and neither re-raised it, so simulate() went on to
__terminate_simulation() and returned exactly as it does after a complete
study. A caller could not tell a partial run from a finished one without
opening the output file and counting rows.

Five more holes sat next to that one, found in review and in the
interrupt-point audit it prompted:

- __run_in_serial bound inputs_json inside the loop body, so Ctrl-C during
  the first keep_simulating() call reached the handler with nothing bound
  and the run died with UnboundLocalError from inside the cleanup.
- It also never cleared inputs_json after a successful append, so Ctrl-C
  landing in the progress print — or in the next keep_simulating() call —
  wrote the row that had just committed into the error file as though it
  never finished.
- _append_simulation_record rolled back on Exception, and
  KeyboardInterrupt does not derive from Exception, so Ctrl-C between the
  two appends left a one-sided record.
- Its rollback also only truncated the inputs file, so an interrupt inside
  either write left a torn partial row for the reload to die on with
  JSONDecodeError instead of the interrupt.
- The parallel cleanup began only after every worker had started, so
  Ctrl-C during the startup loop left the already-started workers running
  with nobody signalling or joining them.

Bind inputs_json before the loop and clear it after each committed append,
roll both files back on BaseException best-effort, cover the startup loop
with the same cleanup path over a started-workers list, and re-raise in
both handlers. Catch the interrupt in simulate() so
__terminate_simulation() still runs before it leaves: it reloads the logs
through the file setters, and set_num_of_loaded_sims is what the
documented append=True continuation reads.

The shutdown join stays unbounded, and the docstring now says so: the
interrupt propagates once every worker finishes the simulation it is in
and exits on its own. A worker stuck inside one simulation blocks the
interrupt as it already blocked the run; killing it here could tear a
half-written row into logs it holds the mutex for, and the bounded fleet
shutdown belongs to RocketPy-Team#1054. The ordinary exception path is unchanged.

Add ten regression tests covering both modes, the early interrupt, the
preserved rows, the reload, an interrupted run continued with append=True,
the between-appends rollback, the torn-write rollback, the committed row
staying out of the error file, and the startup-loop cleanup. They run the
real __terminate_simulation and assert the state it produces; all fail on
develop, and each fix was also reverted individually with only its own
test failing.
ting-hong-shieh added a commit to ting-hong-shieh/RocketPy that referenced this pull request Aug 16, 2026
simulate() returned normally after Ctrl-C. Both execution modes caught the
interrupt and neither re-raised it, so simulate() went on to
__terminate_simulation() and returned exactly as it does after a complete
study. A caller could not tell a partial run from a finished one without
opening the output file and counting rows.

Five more holes sat next to that one, found in review and in the
interrupt-point audit it prompted:

- __run_in_serial bound inputs_json inside the loop body, so Ctrl-C during
  the first keep_simulating() call reached the handler with nothing bound
  and the run died with UnboundLocalError from inside the cleanup.
- It also never cleared inputs_json after a successful append, so Ctrl-C
  landing in the progress print — or in the next keep_simulating() call —
  wrote the row that had just committed into the error file as though it
  never finished.
- _append_simulation_record rolled back on Exception, and
  KeyboardInterrupt does not derive from Exception, so Ctrl-C between the
  two appends left a one-sided record.
- Its rollback also only truncated the inputs file, so an interrupt inside
  either write left a torn partial row for the reload to die on with
  JSONDecodeError instead of the interrupt.
- The parallel cleanup began only after every worker had started, so
  Ctrl-C during the startup loop left the already-started workers running
  with nobody signalling or joining them.

Bind inputs_json before the loop and clear it after each committed append,
roll both files back on BaseException best-effort, cover the startup loop
with the same cleanup path over a started-workers list, and re-raise in
both handlers. Catch the interrupt in simulate() so
__terminate_simulation() still runs before it leaves: it reloads the logs
through the file setters, and set_num_of_loaded_sims is what the
documented append=True continuation reads.

The shutdown join stays unbounded, and the docstring now says so: the
interrupt propagates once every worker finishes the simulation it is in
and exits on its own. A worker stuck inside one simulation blocks the
interrupt as it already blocked the run; killing it here could tear a
half-written row into logs it holds the mutex for, and the bounded fleet
shutdown belongs to RocketPy-Team#1054. The ordinary exception path is unchanged.

Add ten regression tests covering both modes, the early interrupt, the
preserved rows, the reload, an interrupted run continued with append=True,
the between-appends rollback, the torn-write rollback, the committed row
staying out of the error file, and the startup-loop cleanup. They run the
real __terminate_simulation and assert the state it produces; all fail on
develop, and each fix was also reverted individually with only its own
test failing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

7 participants