ENH: reproducible Monte Carlo via per-simulation-index seeding - #1054
ENH: reproducible Monte Carlo via per-simulation-index seeding#1054thc1006 wants to merge 14 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1054 +/- ##
===========================================
+ Coverage 82.18% 83.66% +1.48%
===========================================
Files 122 128 +6
Lines 16355 16700 +345
===========================================
+ Hits 13441 13972 +531
+ Misses 2914 2728 -186 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The seeding logic is unit-tested in The lines codecov still shows uncovered are all in the parallel path: |
0d37ed6 to
761c092
Compare
phmbressan
left a comment
phmbressan
left a comment
There was a problem hiding this comment.
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?
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>
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>
9c020b6 to
3e22729
Compare
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>
3e22729 to
6bf8bb6
Compare
|
@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 |
6bf8bb6 to
c529d0a
Compare
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>
|
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:
Both earlier concerns are still handled: the parallel claim holds the mutex across the check and the increment, and a supplied |
c529d0a to
e5ba940
Compare
|
@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:
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 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 |
5a9c119 to
da2ba5c
Compare
|
A quick note on the red CI here, so it isn't mistaken for a regression from this change: the failing jobs crash in 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. |
|
I opened #1071 for the same issue before spotting this one, and have closed it in favor of this PR, yours is the more complete solution (per-component child seeds, list-valued attribute sampling, int seeds for custom |
for sure go ahead thx!!! |
MonteCarlo seeded the stochastic models per worker in parallel mode (from a fresh, unseeded SeedSequence) and once at construction in serial mode, so the sampled inputs depended on the execution mode and the worker count, and parallel runs were not reproducible run to run. Add a keyword-only random_seed to simulate() (SPEC 7 style: accepts an int, a SeedSequence, or a Generator; None keeps the previous fresh-entropy behavior). Spawn one child seed per simulation index from that root and reseed the stochastic models from child_seeds[i] before simulation i. SeedSequence.spawn is prefix-stable, so index i maps to the same seed regardless of which worker runs it, making the inputs identical across serial, parallel(2) and parallel(N). Each index seed is split three ways so the environment, rocket and flight draw from independent streams rather than sharing one. The serial index field now counts from 0 to match the parallel path. Both changes alter the numbers a fixed seed produces, so stored baselines regenerate. Adds tests/unit/simulation/test_monte_carlo_determinism.py: serial reproducibility, worker invariance (serial == parallel(2) == parallel(4)), and the None-seed path. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The reproducible-seeding change added __root_seed_sequence and __seed_simulation plus the per-index serial and parallel seeding, but the only tests that reached them ran a full Monte Carlo and were marked slow, so the coverage job (which does not pass --runslow) never executed them. Add fast unit tests that drive the two helpers directly: every supported random_seed type normalizes to the same root stream, None draws fresh entropy, existing SeedSequence/Generator/BitGenerator objects are reused rather than copied, and each child seed splits three ways so environment, rocket and flight get independent streams. Move the end-to-end simulate reproducibility tests into tests/integration, next to the existing Monte Carlo simulate test. The serial reproducibility run now lives in the non-slow suite; only the fork-based worker-invariance test stays slow, and it imports multiprocess lazily like the library does. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
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>
…128-bit ints Each simulation index is seeded from its own child of the run's root seed. Building that child by extending the captured root spawn_key is bit-identical to root.spawn(number_of_simulations)[index] but O(1) in time and memory, so a worker reconstructs any index from a small root state instead of the full spawned list being materialized and pickled to every process. Hand each model a 128-bit int rather than a SeedSequence: a plain int is the seed type accepted alike by numpy.random.default_rng, RandomState and the stdlib random.Random (which rejects a SeedSequence with a TypeError from Python 3.11), so a custom sampler whose reset_seed documents an int keeps working; all four uint32 words are combined by value (not via tobytes) so the seed is byte-order independent and keeps the full 128-bit pool instead of collapsing to 32 bits. The random_seed docstring now lists the accepted types and notes the seeding is informed by SPEC 7 while keeping immutable seed-snapshot semantics. The unit tests assert the SeedSequence copy preserves full .state (an entropy-only copy would fail), the O(1) child equals spawn bit-for-bit -- including a root whose child counter has advanced and indices past 2**32 -- and each model receives a distinct 128-bit int. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
…rator dict_generator drew list-valued attributes with the stdlib random.choice, which reads from an unseeded global Random instance, so random_seed did not make those attributes reproducible. Draw the index from this model's seeded numpy generator instead. Indexing (not numpy.random.choice) also avoids coercing a heterogeneous list -- Function objects, paths, arrays -- to a single dtype. Adds a unit test that a list-valued attribute is reproducible under a fixed seed and that heterogeneous entries are returned unchanged. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The existing worker-invariance test stubs the module-level Flight and so reaches workers only under fork. Add a test that the per-index seed derived in a worker matches the main process under every available start method (fork, spawn, forkserver), using a top-level picklable target and small picklable arguments so it is valid under spawn/forkserver -- Python 3.14's POSIX default -- without relying on inherited parent state. It runs in ordinary CI. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
StochasticModel list-valued attributes were sampled with the stdlib random.choice (an unseeded global) and StochasticRocket._randomize_position did the same for list-valued component positions, so random_seed did not govern either. Both now draw the index through the model's seeded generator via a shared _random_choice helper -- indexing, not numpy.random.choice, so heterogeneous objects (Function, paths, arrays) stay intact. StochasticRocket._set_stochastic also handed the same seed to the rocket body and every surface, motor, rail button and parachute, so components sampling the same distribution drew identical values (a main and a drogue parachute got the same cd_s and lag quantiles). Each component is now reseeded from its own spawned child of a SeedSequence root, in a fixed order, so they stay independent and reproducible under random_seed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
…lper simulate() set up (and, for append=False, truncated with w+) the input/output/error files before the seed was validated, so passing a rejected seed such as a Generator destroyed a previous run's results on the way to raising a TypeError. The seed is now captured and validated before __setup_files runs. Moved _seed_sequence_to_int to rocketpy.tools so the stochastic models can share it, and corrected its docstring: a 128-bit int is accepted by default_rng and random.Random, but the legacy RandomState caps a single-int seed at 2**32-1, so the earlier 'accepted by RandomState' claim was wrong. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
da2ba5c to
e88f4df
Compare
|
@MateusStano a summary of where this got to, since I pushed the last of it without saying so and there is no reason you would know it is ready. TLDR: both things you raised are fixed and the threads are resolved. Whenever you have time. Your two points, and what they turned into: The race between The Both threads are marked resolved above. Since your review I also found three more while writing tests for it, and they are in the same branch:
Ten checks green. There is no hurry from my side. Flagging it because the last push was 27 July with nothing said, so it has been sitting looking like it still needs work when it does not. |
wuisabel-gif
left a comment
wuisabel-gif
left a comment
There was a problem hiding this comment.
I reviewed the latest changes and ran the relevant Monte Carlo unit and integration tests locally; they pass. I did not find any blocking issues with the seed derivation, worker index claiming, repeated SeedSequence handling, list sampling, or component seed separation.
One small documentation suggestion: it may be worth clarifying that parallel runs are reproducible by simulation index, while the physical order of records in the output file may still depend on worker completion order. Other than that, the implementation and test coverage look good to me.
_set_stochastic re-validates every kwarg, and validation read the nominal back off self.obj. StochasticEnvironment.create_object writes the sampled value onto that same object on purpose, so the next reseed took the last simulation's result as the new baseline and a factor like wind_velocity_x_factor compounded from one simulation to the next. Serial and parallel runs then disagreed, because the drift depends on how many simulations a worker happened to run before that index. Capture the nominal once, when the model is built, and read it from there. Custom getters pass straight through: they read a component's own attribute rather than one of self.obj's, and every component's position arrives under the one name "position", so caching those would collide. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Two things create_object samples were left out of the per-simulation reseed. Air brakes were never in the reseed loop at all, so they drew from wherever the generator had been left rather than from the simulation index. Every seeding test passed because no fixture had an air brake, which is exactly how it stayed hidden. The collections are now declared in one place and walked from there, and a test scans create_object's source so a collection added later cannot quietly miss the reseed. CP and thrust eccentricity were validated once, at add time, against the generator as it stood then. Reseeding replaced the generator but not those values. Keep the specs as given and reapply them after each reseed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
In the worker: sim_idx and inputs_json are bound before the try. A failure in the index claim used to raise UnboundLocalError inside the error handler, so nothing was written and nothing was printed and the run ended with no record of what went wrong. The handler now writes a JSON line either way, since _read_log_file parses that file with json.loads. Reporting a failure is best effort and must never replace the failure it is reporting. Setting the shared event can raise on its own once the manager has gone, and so can taking the mutex or writing the file. All of it is guarded, the mutex is released if it was taken, and the original exception is what leaves the worker. The worker re-raises so its exit code says it died. In the parent: join() returns None however a child ended, so the shared event was the only signal a run had. A worker can leave without setting it: SystemExit, os._exit, a segfault in a native extension, a target that will not unpickle under spawn, or its own error handler failing. Check the exit codes too. Workers are started inside the try, so a start() that fails part way through the fleet does not leave the running ones with nobody to reap them. After the run, check that every index this run claimed left exactly one input row and one output row. Neither file shows this on its own: the rows look well formed, and reading them back keyed by index hides a duplicate behind the row that overwrote it. A row cut off mid-write is reported as the index that went missing rather than failing to parse, which is what actually happened to it. A run stopped with Ctrl-C is exempt, since both run paths catch it, keep what they have and return, and being short is the point rather than a fault. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The existing tests cover the seed arithmetic everywhere and the real loop under fork. Neither reaches multiprocess.Process, __sim_producer, the manager proxies or pickling the stochastic object graph anywhere but fork, and spawn is what Windows and macOS run, and forkserver is Python 3.14's POSIX default. Serial, two workers and four workers are compared per index under each available start method. Object identity is stripped before comparing: a Function's signature hash and its serialised source encode the object rather than the value drawn for it, and a child that re-imported the module cannot agree with the parent about those. Six fields differ across the boundary on a real run and all six are these. The fixtures are built so the properties can actually 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; this one sets a wind that is actually blowing. A bare StochasticAirBrakes gives every parameter a standard deviation of zero, so it gets one that varies. The assertions check the eccentricities and the air brake are among the compared fields, or stripping identity could quietly empty the comparison. Also covers the parent-side checks: a run missing an input row, a run missing an output row, a row cut off mid-write, appending onto an earlier run, and a run stopped with Ctrl-C. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
Pushed four commits. Review turned up six problems with the seeding change and they are all fixed, plus four more I found going back over my own fix. The six:
Going back over that, four more:
Worth flagging about the fixtures, since four of these were invisible to a green test suite: the shared Locally: full suite green apart from four pre-existing @MateusStano the three threads from your review are resolved. Happy to split any of this into a separate PR if that is easier to look at. |
Assigning to montecarlo._MonteCarlo__evaluate_flight_inputs and friends trips pylint's invalid-name, which exits 16 and fails the Linters job even though the score is 10.00. monkeypatch.setattr takes the name as a string, so the check does not fire, and it puts the original back afterwards instead of leaving the instance patched for whatever runs next. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
A review of the seeding change turned up failure paths where a run that went wrong could still be reported as a success. Six of them, all in the machinery around the simulations rather than in the seeding itself. The parent waited for every worker with an unbounded join, in the order they were started. 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 ever reached, and Ctrl-C hung on the same join a second time. The wait is bounded and gives up as soon as the event is set. Shutdown signals the whole fleet before waiting on any of it, then falls back to kill, so a worker that ignores the first signal does not keep the others, the manager and the open files alive behind it. The completeness check accepted a corrupt file. Rows it could not parse were skipped, rows carrying no index or an index outside the run were ignored, and JSON true or 1.0 passed for the 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 is allowed to be short but not to be corrupt. Both run paths cleared the current payload after the call that can be interrupted rather than before it. In the serial path Ctrl-C on the first lap reached the handler with it unbound, so the interrupt surfaced as an UnboundLocalError, and between laps it still held the row that had just been written. In the worker the same ordering meant a claim that failed on a later lap reported the simulation that had just succeeded. The normal write path also released the mutex in finally whether or not acquire had returned. The error record kept either the inputs or the traceback, never both, so every failure after sampling left no traceback in the file the run points the user at. 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. All argument checking happens before any file is touched, and number_of_simulations is checked too. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Pull request type
Current behavior
MonteCarlo.simulate()seeds the stochastic models per worker in parallel mode (from a fresh, unseedednp.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_seedtosimulate(). From that root, simulation indexiis seeded from its own child of the root seed, derived before simulationiruns, so indeximaps 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:
iis built by extending the root'sspawn_key, which is exactly howSeedSequence.spawnderives it, sochild(i)is bit-identical toroot.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.int, not aSeedSequence. An int is the seed typenumpy.random.default_rngand the stdlibrandom.Randomboth accept (aSeedSequenceraisesTypeErrorinrandom.Randomsince Python 3.11), so a custom sampler whosereset_seeddocuments an int keeps working. All fouruint32words are combined by value, so the seed is byte-order independent and keeps the full 128-bit pool rather than collapsing to 32 bits.StochasticModel.dict_generatordrew list attributes with the stdlibrandom.choice(an unseeded global instance), sorandom_seeddid not govern them. It now draws the index from the model's own seeded generator, which also avoidsnumpy.random.choicecoercing a heterogeneous list (Function, paths, arrays) to a single dtype.random_seedis a seed, not a live RNG: it takes an int, a numpy integer, a sequence of ints, or aSeedSequence, withNone= fresh entropy so existing behavior is unchanged unless you pass a seed. A suppliedSeedSequenceis 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 statefulrng: aGenerator/BitGeneratoris not accepted, because reducing it to its underlyingSeedSequencewould ignore how far it has been consumed. Passrng.bit_generator.seed_seqto 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
keep_simulating()+increment(). Near the end of a run two workers could both passcount < nand 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.SeedSequencewas returned as-is, andspawn()advances its child counter, so passing the same object twice was not reproducible. It is now copied from its full state, andGenerator/BitGeneratorare 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_stochasticgave 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 identicalcd_sandlagquantiles. Each component now gets its own child of the run's seed, in a fixed order, so they stay independent and reproducible.dict_generatorandStochasticRocket._randomize_positionsampled list-valued attributes (component positions included) with the stdlibrandom.choice, whichrandom_seeddid not govern. Both now draw the index through the model's seeded generator, via a shared_random_choicehelper.simulate()set up (and, forappend=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.RandomState, and moved it torocketpy.toolsso 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 moderndefault_rng(or the stdlibrandom.Random) takes it fine; one built onRandomStatewould need to reduce it. SinceRandomStateis 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:
append=Truedoes not continue the same seeded run #1075:append=Truedoes not yet continue the same seeded run (the root is not persisted, and the next index comes from a row count).simulate_convergence()cannot reproduce a study from a seed #1077:simulate_convergence()has no seed, so a convergence study is not reproducible.What this PR does and does not promise
The guarantee here is over the sampled inputs: for a given root seed,
simulation index
idraws the same stochastic parameters whether the run isserial or parallel and however many workers it uses. That is what
.inputs.txtrecords and what the tests compare.
It is deliberately not a guarantee about the whole trajectory yet, because two
built-in random sources sit outside the seed tree this PR builds. I found both
while going back over this change and filed them rather than growing the PR
further:
Parachutetakes its pressure noise from the process-globalnp.random.Flightadds that noise to the pressure it hands the trigger, soit can move the deployment time and the descent with it. The shared fixtures
already use non-zero noise, so this is on the ordinary path, not an opt-in.
MonteCarlocalls_randomize_rail_length,_randomize_inclinationand
_randomize_heading, and each one draws the whole flight dictionaryagain. The
Flightgets rail length from the first draw, inclination from thesecond and heading from the third, while the row written to
.inputs.txtholds the third. Measured on the shared fixture, the logged inclination is
85.60 while the flight used 84.46.
Until those land, two runs agreeing on
.inputs.txtdoes not prove they flewthe same thing. Once they do, the promise can be restated in terms of results.
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:
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.
rows with no index or an index outside the run were ignored, and JSON
trueor
1.0passed for index1because both compare equal to it. Every row nowhas to be an object with a plain non-negative
intindex, the two files haveto agree on the exact set, and an interrupted run may be short but not corrupt.
than before it. Serial Ctrl-C on the first lap surfaced as an
UnboundLocalErrorover the interrupt, and between laps the handler stillheld 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.
finallywhether or notacquire()had returned, so a manager that died during acquire raised asecond error over the first.
n_workerswas validated after the logs were opened"w+", so asking for aworker 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
SeedSequencecopy preserving the full.state, theO(1) child equal to
spawnbit-for-bit including a root whose counter hasadvanced and indices past 2**32, the 128-bit width, and the parallel index claim.
tests/integration/simulation/test_monte_carlo_determinism.pyruns the realparallel 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
StochasticAirBrakesgives every parameter a standarddeviation 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.pycovers what the run isallowed to call a success and how the fleet comes down when it is not.
One caveat worth flagging: the start-method test is marked
slow, andpull-request CI skips slow tests, so it is not a merge gate today. That matches
how
test_inputs_are_worker_invariantis already marked, but it does mean thespawn and forkserver paths are only exercised on the weekly run. Happy to drop
the marker on a short version of it, or add a job that passes
--runslowforthis file, whichever you prefer.
Breaking change
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_simulatechecks apogeeand impact velocity within a tolerance and still passes), and
random_seedisopt-in.
Partially addresses #1053. The per-index seeding for serial and parallel runs is
here;
append=Truecontinuing the same seeded stream is #1075, and the runtimerandom sources are #1090 and #1091. I would rather leave #1053 open until those
land than close it on a guarantee that only covers the sampled inputs.