BUG: accept the seed type a Monte Carlo worker is handed - #1181
Open
thc1006 wants to merge 1 commit into
Open
Conversation
6 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1181 +/- ##
===========================================
+ Coverage 84.57% 85.14% +0.56%
===========================================
Files 131 131
Lines 17527 17533 +6
===========================================
+ Hits 14824 14929 +105
+ Misses 2703 2604 -99 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
thc1006
force-pushed
the
bug/accept-the-seed-monte-carlo-hands-over
branch
from
August 17, 2026 20:08
ffbb39d to
d1e61e6
Compare
A parallel run spawns a SeedSequence per worker and passes it to environment, rocket and flight. _sampler_seed then fed it to SeedSequence(entropy=...), which takes an int or a sequence of ints, so the first worker raised TypeError before drawing anything. The call was reached only from the custom sampler reset until RocketPy-Team#1117 added the list-choice generator, which every model goes through. A real two-worker run passes at d21abde^ in 2.32s and does not finish on develop: the worker's own error path raises UnboundLocalError on inputs_json, so the parent never learns it died and the run hangs. The children of one root share their entropy and differ by spawn_key, so the value is folded through generate_state rather than read off entropy, which would put every worker on one sampler stream. Nothing is consumed, and an int or None seed keeps the stream it had. The fold lives in rocketpy.tools, since the component streams and the per-index seeding both need the same one and three copies would drift on width and word order. _sampler_seed does its own final fold through it as well rather than repeating the four lines. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
thc1006
force-pushed
the
bug/accept-the-seed-monte-carlo-hands-over
branch
from
August 17, 2026 21:39
d1e61e6 to
7b2217a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Parallel Monte Carlo does not run on
developright now. It hangs, and I think it has since 12 August.Pull request type
Checklist
ruff check/ruff format --check,pylint) has passed locallyCHANGELOG.md— no action needed; an LLM workflow auto-updates it after merge. Worth knowing that it has not run since CI: run the changelog job for pull requests from forks #1112, which I wrote up in BUG: the changelog workflow stopped running, and CHANGELOG.md is 37 merged pull requests behind #1173.Current behavior
A two-worker run over a real
Flight, bisected:__run_in_parallelspawns aSeedSequenceper worker and passes it down, so_set_stochasticreceives one of those rather than an int:_sampler_seedthen hands it toSeedSequence(entropy=...), which takes an int or a sequence of ints, so the first worker dies before it draws anything:Two things kept this quiet.
_sampler_seedis mine, from #1102, and until recently it was only reached from the custom sampler reset loop, so a model without custom samplers never went near it. #1117 added the list-choice generator atstochastic_model.py:157, which every model goes through on every reseed. That turned a path almost nobody took into the one all of them take. Nothing wrong with the #1117 change; my function should have taken the type it can now be given.The hang on top of the error is a separate problem in the worker's own handler, and I have that in a follow-up rather than in here.
Nothing in the test suite calls
_set_stochasticwith aSeedSequence, and no pull request job runs a real parallel Monte Carlo, which is why the matrix stayed green through all of it.New behavior
_sampler_seednormalizes what it is given before building anything from it.The value is folded through
generate_staterather than read off.entropy. The children of one root share their entropy and differ only byspawn_key, so reading the entropy would have quietly put every worker on the same sampler stream, which is worse than the crash it replaces:An int or
Noneseed goes through untouched, so no fixed-seed baseline moves.stochastic_calistounder seed 42 still readsmass=14.906007947 radius=0.063501935, same asdevelop.The fold itself lives in
rocketpy.tools. The component streams in #1170 need the same one and the per-index seeding will too, and three copies would drift on width and word order.tests/unit/simulation/test_monte_carlo_parallel_runs.pyruns a real serial and parallel Monte Carlo. Both together take under six seconds and it is not marked slow, so a pull request gets the signal that was missing here. It builds its ownMonteCarloontmp_pathrather than retargeting the fixture's:filenameis a plain attribute and the three log paths are settled in__init__, so assigning it would leave the test writing into the working directory.Breaking change
Additional information
Verification, on a clean tree:
The four
tests/unit/test_sensitivity.pyfailures on my machine are a missingstatsmodelsand fail the same way on an untoucheddevelop.Each mechanism is pinned by a mutation, and each leaves a control standing:
return seed.entropytest_two_workers_do_not_share_a_sampler_streamThe second row is the reason that test exists. Reading
.entropyunbreaks everything else and silently collapses the workers onto one stream, so the suite would have gone green on a fix that is worse than the bug.Merged with #1169, #1170 and my follow-up into a throwaway tree on
developand run there as well, since green on separate bases says nothing about the combination.