ENH: seed Monte Carlo per simulation index for worker-invariant inputs - #15
ENH: seed Monte Carlo per simulation index for worker-invariant inputs#15thc1006 wants to merge 4 commits into
Conversation
MonteCarlo seeded the stochastic models once per worker (parallel) or once before the loop (serial), so the generated inputs depended on how many workers ran and which worker drew which simulation. Two runs of the same seed with different worker counts produced different inputs. Seed per simulation index instead: spawn one SeedSequence child per index from the run's root seed, and reseed the stochastic models from child_seeds[i] before simulation i. SeedSequence.spawn is invariant to the spawn count, so index i always maps to the same child, including in append mode. Each index seed is split three ways so the environment, rocket and flight do not share a stream. Inputs are now identical across serial, parallel(2) and parallel(N) for a fixed seed, and a split append run reproduces a single run. This changes the numbers a fixed seed produces (the per-model decorrelation, plus a serial index that now counts from 0 like the parallel path already did), so stored baselines regenerate. Adds tests/unit/simulation/test_montecarlo_determinism.py covering serial reproducibility, worker-invariance, append reproducibility, and the None-seed path. Marked slow (each simulation rebuilds a full rocket) per the existing test_monte_carlo_simulate convention.
|
Let's pull this from RocketPy once RocketPy-Team#1054 is merged |
for sure |
|
I'd prefer to wait for them to reduce conflicts when pulling changes from RocketPy in the future. Also, we'll probably stay with serial monte-carlo for now |
|
Marking this draft to match what you asked for on this PR: wait for RocketPy#1054 and pull the implementation back through the usual sync. It is still open, at 8 commits and 9 files, so there is nothing to pull yet. Leaving it open and unmarked was costing review attention it should not be: this branch is one commit from July 18 and is now the older, narrower version of work that moved upstream. Two things worth recording before it sits, since they are true of this branch and would matter if anyone picked it up later:
Neither reaches BPC today, since it runs Monte Carlo with One thing that does not depend on this PR and is worth its own issue at some point: |
`ruff format --check .` is one of the Linters jobs and it covers this file, so the unformatted example in it fails that job on develop today. Splitting it out of the seeding change because it has nothing to do with it. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Replaces the earlier draft of this change with the implementation that is now on RocketPy#1054, so the two carry the same code and a later sync from upstream is a no-op rather than a conflict to resolve. Simulation index i draws its seed from its own child of the run's root, derived before that simulation runs, so index i maps to the same inputs whether the run is serial or parallel and however many workers it uses. The child is built by extending the root's spawn_key, which is how SeedSequence.spawn derives it, so no per-index list is materialised or sent to a worker. Beyond the seeding itself, the upstream version carries the review rounds this draft predates: each stochastic model holds its nominal values steady instead of reading them back off an object that create_object mutates; air brakes are in the reseed and a source scan keeps a later collection from missing it; CP and thrust eccentricity are reapplied after each reseed; the parallel index claim holds the mutex across the check and the increment; a worker preserves the failure that killed it rather than raising over it; the parent bounds its wait and checks exit codes; and the completeness check rejects a log it cannot read rather than skipping past it. Four things stay ours, since they are what the fork is for: simulate returns the results dict, each Flight gets the stochastic flight's max_time, max_time_step and min_time_step, set_processed_results stays disabled with its TODO, and StochasticRocket keeps add_linear_generic_surface and volume. The rocket file was merged three ways against the shared RocketPy base rather than overwritten. Local run: ruff clean, pylint rocketpy/ tests/ docs/ exits 0, 1640 unit tests and 152 integration tests pass, and the start-method gate passes under fork, spawn and forkserver. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Missed one of ours when taking the upstream file. The serial run redraws a progress line with a carriage return, so this fork routes its interrupt and error messages through _SimMonitor.reprint rather than print, and a plain print lands on top of that line. The two monitor stubs in the log-integrity tests gain a reprint for the same reason, which is the only place those tests differ from the upstream copy. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
@zuorenchen I have redone what is on this branch. It now carries the same code as RocketPy#1054 instead of our own version of the feature, so the sync you asked to wait for should be close to a no-op rather than a conflict. I measured first: Still draft, and the timing is still yours. The body has the details, including what the earlier draft on this branch was missing. One thing worth your call: the test that exercises the real parallel path under spawn and forkserver is marked slow, and neither our PR CI nor RocketPy's runs slow tests. So it passes on my machine and gates nothing. |
2 participants
Still a draft, and still yours to decide when it moves. You asked on 18 July to
wait for RocketPy#1054 and pull the implementation back through the usual sync,
so what I have done here is change the method rather than the timing: instead of
carrying our own version of this feature, the branch now carries the same code
that is on RocketPy-Team#1054.
Why that answers the conflict worry
The concern was that writing this ourselves makes a future pull from RocketPy
painful. I measured how far apart the two really are:
They are nearly the same files, and they share history at RocketPy v1.11.0. So
rather than replaying commits or writing a parallel implementation, I took the
upstream files and put our fork-specific changes back on top:
simulatereturns the results dictFlightgets the stochastic flight'smax_time,max_time_stepandmin_time_stepset_processed_resultsstays disabled with its TODOStochasticRocketkeepsadd_linear_generic_surfaceandvolume_SimMonitor.reprintrather than
print, so they do not land on the progress lineI had the first four and missed the fifth. What caught it was comparing the set
of definitions in each file before and after rather than reading the diff:
reformatting makes almost every changed line look like a deletion, so a diff is
close to useless for this, while a lost function shows up immediately.
stochastic_rocket.pywas merged three ways against the shared RocketPy baserather than overwritten, so nothing of ours was dropped. When RocketPy-Team#1054 lands, the
sync should be close to a no-op instead of a conflict.
What the earlier draft was missing
That draft predates two review rounds on RocketPy-Team#1054, and those rounds found real
defects in it. Carried over now:
back off an object that
create_objectmutates on purpose. Without this afactor like
wind_velocity_x_factorcompounds from one simulation to the next,and serial and parallel disagree because the drift depends on how many
simulations a worker happened to run first
seeding test passed because no fixture had one, which is how it stayed hidden.
A source scan over
create_objectnow keeps a collection added later frommissing it the same way
bound once to the generator as it stood at add time
releases the mutex it actually took
checks exit codes rather than trusting
join()Verification
Ran the same things the Linters and Pytest workflows run:
The formatting of the Copilot instructions is a separate commit.
ruff format --check .covers that file and it fails on develop today, which is unrelated tothis change and did not belong inside it.
Two things worth knowing before this moves
The
--runslowgate is where the real parallel path gets exercised under spawnand forkserver, and neither PR CI here nor on RocketPy runs slow tests. So the
21 passes above are from my machine, not from a merge gate. Worth deciding
whether we want a job for it.
ff11f459, the original commit on this branch, has noSigned-off-by. I haveleft it alone rather than rewrite pushed history; say the word if you would
rather I rebase it.