ENH: reproducible Monte Carlo via per-simulation-index seeding - #1054
ENH: reproducible Monte Carlo via per-simulation-index seeding#1054thc1006 wants to merge 45 commits into
Conversation
Codecov Report❌ Patch coverage is
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. 🚀 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
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. |
`dict_generator` walks the whole instance, so `parachutes` and `air_brakes` were drawn from as ordinary lists. Since this branch started seeding list choices from the model's own generator, that draw moved every later one, and `StochasticRocket.dict_generator` discards it a few lines further down. Attaching a main and a drogue changed the sampled mass under a fixed seed, which is the property this branch exists to establish. One component does not show it: `integers(1)` has a single outcome and NumPy returns it without consuming any state, so the test covers 0, 1 and 2. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
A worker killed outright sets no error event. If it died holding the shared lock, its siblings never return either, so `any(is_alive())` stayed true and the unbounded wait never reached the exit-code check below it. The wait now also ends on a non-zero exit code. Only the unbounded one: the shutdown grace period is bounded already and must not be cut short. `type(value) in (int, np.integer)` is False for every NumPy integer, because `type(np.int64(3))` is `np.int64`. It was written that way to keep `True` out, which `isinstance` lets through, so both are now checked explicitly. `number_of_simulations` is the total to reach when appending, not a batch to add. Below the checkpoint it ran nothing and returned success, leaving a file with more simulations than the caller asked for. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The unmarked test took the platform default and its docstring claimed that gated spawn on macOS and forkserver on 3.14. Neither is true: multiprocess hard-codes fork on every POSIX platform, macOS and 3.14 included, with a `#FIXME: spawn` still beside the darwin branch. So the shipped parallel path was gated on fork everywhere except Windows, and the failure message named the stdlib start method rather than the one that made the workers. The thorough test already covers all three through the real path and takes 26 s against 89 s for the rest of the directory, so it is unmarked now and the default-taking one is deleted rather than corrected. Its assertions were a subset. The start-method list is asked of multiprocess as well. That import is at module level behind a try/except because it happens while tests are collected, where importorskip would take the module down instead of skipping it. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
SeedSequence keeps a sequence entropy by reference, and the capture stored that
reference, so a caller who passed a list and later edited it changed the child
seeds of a run that had already read the seed. The docstring called it an
immutable snapshot, which it was only for an int.
entropy = [1, 2, 3]
mc.simulate(2, random_seed=entropy)
entropy[0] = 999999 # moved every index of that run
Deep-copied on the way in now, with spawn_key made a tuple while it is there.
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Each worker got the full grace to itself, and twice over, once after terminate and once after kill. Eight stubborn workers could therefore hold the parent for sixteen grace periods rather than two, which is a 5 s promise turning into 80 s. The deadline is shared now, so the wait costs the same whatever the fleet size. Every worker is still joined, so exit codes are still reaped. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
…meet The assertion checked one fleet against the grace itself, so its slack had to cover the machine's timer granularity. On Windows that is about 15 ms against a 200 ms grace, and the job failed at 0.213s under a 0.21s bound. Comparing a fleet of six against a fleet of one carries the same granularity on both sides, so it cancels. Six against twice one leaves roughly half the bound spare on the Windows numbers, and the per-worker grace it replaced would grant six times as much. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The refusal message suggested re-running "or renumber the file down by one", while the comment two lines above it said the fix is to re-baseline rather than retry. The comment was right. Renumbering lines the indices up and leaves the seeds behind. Those rows came from the old sequential scheme, so a renumbered file would carry rows 0..n-1 that this release's per-index derivation would never have produced for those indices, and appending onto it would join two different seedings without saying so. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
A failure after sampling wrote the inputs to .errors.txt with no traceback,
while a worker writes {index, ...inputs, error: traceback}. The file the run
tells the user to read named which inputs failed and not why.
Both paths now build the row through one helper. Three further points on that
path:
- inputs_json is cleared once the pair is on disk, so a failure inside
print_update_status() reports itself rather than reporting an already
committed row as one that never finished.
- sim_idx is bound before the loop, so a failure on the first iteration has an
index to record.
- the re-raise is bare, so the handler's own line does not join the traceback.
KeyboardInterrupt keeps its own handler: an interrupt is not a failure with a
traceback worth recording, and it still logs the inputs that did not finish.
The append docstring said only that results are appended. It now says
number_of_simulations is the target total rather than a number to add, that a
lower value is refused, and that the root seed is not stored in the files.
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Four places where handling an error could lose it or misreport it. The worker did not clear its payload once the input/output pair was on disk, so a failure in the progress call after it wrote the same sampled inputs to the error log. One simulation then appeared in the logs and in the failures. The serial path was fixed for this last round; this is its other half. The serial error write had no guard. An unwritable error file raised OSError in place of the exception it was recording. The worker already treats its own reporting as best effort, and now so does this, with a warning rather than silence. Same for the inputs kept on Ctrl-C: an interrupt should not become a crash because the file could not be opened. _bring_the_fleet_down said it would not raise over the failure being handled, but only the event was guarded. The two waits under it were not. The parallel parent re-raised with `raise error`, which adds its own line to the traceback. Bare, as the serial path already does. Four tests, each pinned by reverting the line it covers. The changelog entry said only that runs are reproducible. It now carries the migration: fixed-seed samples change, serial log indices move to zero-based to match the parallel path, and old checkpoints cannot be resumed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
_wait_for_workers joined every worker for 0.1s before rechecking the event, so
one round cost the fleet size times that. Measured against stubs that never
finish, with the event set part way through a round:
1 worker 0.10 s
8 workers 0.80 s
32 workers 3.20 s
100 workers 10.01 s
One sleep per round instead. is_alive() at the top of the loop already reaps,
and the join sweep at the end still runs, so nothing is left unreaped.
The grace period test asserted how many times a worker had been joined, which
was the old mechanism rather than the behaviour. It now measures how long the
window lasted. The wait can only overshoot its deadline, never undershoot it,
so a floor well under the timeout holds on a coarse clock.
Two fleet sizes, both against the same ceiling, since the point is that neither
depends on the count. Reverting the loop fails the 24-worker case and leaves the
single-worker one passing, which is the control.
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Two ways the reporting could still replace what it was reporting. warnings.warn raises when the caller has turned RuntimeWarning into an error, which is how a strict test or application run is configured. So the guard around the error-file write was best effort only under the default filter: default filter caller sees Boom: injected -W error caller sees RuntimeWarning: could not be written Both reporters now go through one helper that overrides the filter for its own warning and swallows anything the warning machinery raises. The parallel handler's cleanup was already best effort, but `finally` runs the same cleanup again on the way out and did so raw. A failure there landed on the caller instead of the exception being re-raised. On the second: `finally` also runs after a clean run, where nothing is in flight and raising would have been fine. Warning in both cases is the simpler rule, and a cleanup failure is still reported either way. Two tests, each pinned by reverting the line it covers. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The manager lock was released raw in `finally` at all three sites. A proxy that dies while the lock is held then hands the caller its own BrokenPipeError, and the failure it interrupted survives only as context: before BrokenPipeError: [Errno 32] Broken pipe after Boom: the real simulation failure One context manager owns the lifecycle now. Release is best effort only while another exception is on its way out; with nothing in flight a failed release is the news and still raises, so a run does not continue against a dead manager. The close path ran its wait and forced stop raw before checking the workers, so a cleanup failure reached the caller before anything read the exit codes. Those two are best effort as well. `_fail_if_a_worker_did_not_finish` stays raw: it is the verdict on the run, not housekeeping. `error_event.is_set() or crashed` asked the proxy first, so an unreachable manager threw before the crash list was read and "exited with 7" was lost. The query is now a helper that reports an unavailable event as a worker failure and names it alongside the crashes. Five tests, each pinned by reverting the line it covers. One of them is the other direction: a release that fails on its own must still raise. A note on the probe that found this. A mutex failing on every release is the wrong model, because the first clean claim fails on its own release before there is anything to mask. The stub releases cleanly a set number of times first. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The Notes said an interrupted run can be loaded and continued. Not every one can: the two logs have to hold the same simulations as a complete run of indices from zero, and a parallel stop can leave one worker's index missing while a later one is already written. That checkpoint is refused rather than repaired, which is RocketPy-Team#1075, and the test for it is already there. `random_seed` said the sampled inputs are identical across execution modes. What is identical is the mapping from index to inputs. A worker takes the log lock once its simulation is done, so the rows land in completion order and the file can differ run to run. Someone diffing two runs byte for byte would read that as reproducibility being broken. No test for the second one on purpose. The suite already reads the logs into a dict keyed by index rather than comparing them as text, which is the same claim from the other side, and a test asserting the order does differ would pass or fail on luck. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
`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>
0f48613 to
81d3e58
Compare
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>
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
idraws the same stochastic parameters whether the run is serial or parallel and however many workers it uses. That is what.inputs.txtrecords 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:
.inputs.txtis not the row that was flown: logged inclination 85.60, flown 84.46 on the shared fixture. Every draw is deterministic under a fixed seed, so this costs the log as a transcript rather than the seeding. Not fixed here: BUG: sample StochasticFlight inputs once per simulation (#1090) #1126 is open against the same method, adds the shared_sample_flight_inputsthe fix wants, and also closes the second draw insideStochasticFlight.create_objectthat a Monte-Carlo-only fix leaves alone. I had written my own and took it back out rather than have two of them.append=Truedoes not continue the same seeded run #1075append=Trueis a resume, but not a repair. A run records the root, the count and which logs it wrote, an append continues that root or is refused, and the whole manifest is validated first, so a checkpoint from another lineage or another release no longer reads as continuable. What is left of MonteCarloappend=Truedoes not continue the same seeded run #1075 is filling holes: the next index still comes from a row count rather than from a plan, so a checkpoint with a gap is refused rather than repaired.simulate_convergence()cannot reproduce a study from a seed #1077simulate_convergenceseeds neither its batches nor its bootstrap resampling, so a convergence study is outside this guarantee.#1091 (
Parachutepressure noise on the process-globalnp.random) was the other one. It is closed: #1134 gaveParachutea per-instance RNG andStochasticParachutenow derives its seed from this tree.#1109 was the third one, and it is fixed on
developnow: #1122 madedict_generatorwalk the inputs a model declares rather than every attribute on it, so a tupleinitial_solutionis no longer read as a distribution. The issue is still open because a pull request intodevelopcannot 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
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: the manifest carries the root, the count and the log names now, and an append checks all of them. What is left is claiming indices from a plan instead of counting on from the end, so a checkpoint with a hole is refused rather than filled.simulate_convergence()cannot reproduce a study from a seed #1077:simulate_convergence()has no seed, so a convergence study is not reproducible.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.trueor1.0passed for index1because both compare equal to it. Every row now has to be an object with a plain non-negativeintindex, the two files have to agree on the exact set, and an interrupted run may be short but not corrupt.UnboundLocalErrorover 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.finallywhether or notacquire()had returned, so a manager that died during acquire raised a second error over the first.n_workerswas 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, theSeedSequencecopy preserving the full.state, the O(1) child equal tospawnbit-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.pyruns 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 bareStochasticAirBrakesgives 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.pycovers 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
slowand pull-request CI skips those, so it gated nothing. I said earlier that the weekly run would still cover it; that was wrong.Scheduled Teststriggers onscheduleand onpushtomaster, with nopull_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:
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.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:
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:
multiprocessis an optional extra and was imported inside the parallel path, which runs after both logs have been emptied. An install withoutrocketpy[monte-carlo]lost its results on the way to theImportError.Generatorrejection advisedrng.bit_generator.seed_seq, which NumPy grew in 1.25 while this package declarednumpy>=1.13, so the advice raisedAttributeErroron versions it claimed to support. The floor moves to 1.17, whichdefault_rnghas needed all along.Also: the custom sampler fixture built a
Generatorinreset_seedand dropped it whilesample()drew from the process-globalnp.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.SeedSequencetakesarray_like[ints], soentropycan 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_spawnedstays in it because__child_seedcounts from there. Onlyint,Noneand a differentinthad 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_seedout 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, acommitted_countand 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 whoseseed_chosenis 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.replaceis 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 aKeyboardInterrupt. It is still not a filesystem transaction and the docstring says so.A bool is an int.
simulaterefuses one through_is_whole_number;simulate_convergencechecked with a plainisinstance, sobatch_size=Trueran one simulation.tolerancehad 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_objectorFlightitself left a traceback with nothing about the inputs behind it.dict_generatorbuilds a local dictionary and bindslast_rnd_dictonce, 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 clearslast_rnd_dict, whichdict_generatordocuments 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
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 apogee and impact velocity within a tolerance and still passes), andrandom_seedis 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_convergenceis #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
idraws the same stochastic parameters whether the run is serial 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. The gaps are filed rather than grown into this PR:
.inputs.txtis not the row that was flown. BUG: sample StochasticFlight inputs once per simulation (#1090) #1126 is open against the same method and fixes it, including the second draw insideStochasticFlight.create_objectthat a Monte-Carlo-only fix leaves alone. I wrote my own, found theirs, and took mine back out.append=Truedoes not continue the same seeded run #1075: the root, the count and the log identity are persisted and checked. What is left is claiming indices from a plan rather than counting on from the end.simulate_convergence()cannot reproduce a study from a seed #1077:simulate_convergencepasses no seed to its batches and norandom_stateto the bootstrap, so a study is not reproducible.#1091,
Parachutepressure noise drawn from the process-globalnp.random, wasopen when this PR was written. It is closed now: #1134 gave
Parachutea per-instance RNG andStochasticParachutefeeds it a seed derived from this tree, so parachute deployment is inside the guarantee.Until those land, two runs agreeing on
.inputs.txtdoes not prove they flew the same thing. Once they do, the promise can be restated in terms of results.