BUG: accept the seed type a Monte Carlo worker is handed - #1181
Merged
Gui-FernandesBR merged 2 commits intoSep 9, 2026
Merged
Gui-FernandesBR merged 2 commits into
Gui-FernandesBR merged 2 commits into
Conversation
6 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1181 +/- ##
===========================================
+ Coverage 89.96% 90.53% +0.56%
===========================================
Files 131 131
Lines 17527 17533 +6
===========================================
+ Hits 15769 15874 +105
+ Misses 1758 1659 -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
2 times, most recently
from
August 17, 2026 21:39
d1e61e6 to
7b2217a
Compare
6 tasks
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 25, 2026 15:58
7b2217a to
5a572d3
Compare
6 tasks
Calling the result the int a SeedSequence can be rebuilt from reads as a round trip of the entropy and spawn key. It is neither: the helper derives a 128-bit seed and the state it came from cannot be read back out. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Contributor
Author
|
Pushed 044ffd9, wording only. Calling the result the int a |
Gui-FernandesBR
approved these changes
Sep 9, 2026
Gui-FernandesBR
pushed a commit
to thc1006/RocketPy
that referenced
this pull request
Sep 9, 2026
RocketPy-Team#1181 landed the same helper on develop, with the same body, so merging develop in left tools.py defining it twice a hundred lines apart. Git had no conflict to report: the two copies were added at different points in the file, so the second simply shadowed the first, and pylint would have failed the branch with E0102 rather than anything explaining why. Develop's copy is kept, its docstring being the fuller of the two. What this branch still adds on its own is _seed_sequence_from, which is left where it was, and the test in test_tools.py now covers the surviving definition. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Gui-FernandesBR
pushed a commit
that referenced
this pull request
Sep 9, 2026
* BUG: give each StochasticRocket component its own random stream _set_stochastic handed the same seed to the rocket body and to every surface, motor, rail button and parachute, so two components built from one spec drew identical values: a main and a drogue with the same cd_s and lag spec drew the same cd_s and the same lag, every time, and a study of both was a study of one counted twice. Air brakes were worse. They are built and sampled in create_object and were not in the reseed at all, so their values came from wherever the generator had been left rather than from the seed: 0.683, then 0.586, then 0.488 for one seed asked three times. Each component now takes its own child of a SeedSequence root, spawned in a fixed order so one seed still reproduces the whole rocket. The collections are named in one place and checked against create_object's own source, since the collection no fixture populates is the one that gets missed. Extracted from #1054. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * DOC: shorten the comments on the component seeding Measured against the register the repository uses: inline comments in flight.py average 5.6 words and none of its docstrings run longer than the code they describe. The three added here were four to seven lines of prose where a line would do, and the seed helper carried seven lines of docstring over two lines of code. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * BUG: leave the rocket body's stream where it was, and isolate each collection Moving the body to child zero broke every fixed-seed baseline for mass, radius and the body inputs, and nothing about the nested-component fix needed that. The body keeps the seed as given now: stochastic_calisto under seed 42 reads mass=14.906007947 on develop and the same here. Components were also addressed by one global traversal index, so adding a fin moved every motor, rail button, parachute and air brake. Each collection has a root of its own now, spawned from the same seed, so an unrelated component in one of them leaves the others where they were. The source scan compares the two sets both ways. A collection left in the reseed after create_object stops using it still spawns a child and moves every stream after it, which the subset check let through. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * TST: count the reseeds, and cover two air brakes on one spec The source scan reads create_object for a literal loop over self.collection, so a helper, a local alias or a getattr would hide a collection from it. Counting what each entry actually receives is the check that survives a refactor, and it is the only one that fails when an entry is reseeded twice. The air brakes are a plain list and take a different route through the reseed than the positioned collections, so two of them on one spec are worth their own case. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * TST: stop the isolation test from storing one wrapper twice stochastic_calisto already holds the stochastic_nose_cone fixture, so adding it again put the test into the state #1172 describes: one wrapper in two entries, its position overwritten, and two reseeds landing on the same object. The assertion looked at a different collection and passed anyway. It adds the deterministic nose now, so add_nose builds a wrapper of its own. Nothing pinned the body keeping the seed as given either. Reproducibility and seed uniqueness both hold with the body on a spawned child, so neither would have noticed it going back there. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * DOC: say how a rocket's components are seeded The change moves every fixed-seed component baseline and nothing in the user documentation said how components are seeded at all, before or after. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * DOC: say what separate streams do and do not promise Two independent streams are not made to consume the same draws; they can still land on equal values, and a specification with no spread always will. The text promised unequal results, which is a stronger claim than spawning gives. It also said each kind of component is spawned separately. The unit is the collection: a nose cone, the fins and the tail share one root. And a stream belongs to one wrapper, so storing one twice or sharing it between rockets is outside what this establishes. That is #1172. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * DOC: say that the reset builds the tree, not the add A rocket resets itself while being constructed, when it holds no components yet, so a parachute added afterwards keeps the generator it was built with until the next reset. The text read as though attaching a component gave it a stream, which is only true once something resets the rocket, and a Monte Carlo is what does that. Two wrappers sharing a CustomSampler seed_group are also one stream on purpose. Separate component streams are not meant to take that apart, so the note says so rather than leaving it to be discovered. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * DOC: correct what Monte Carlo and a shared sampler group actually do A serial MonteCarlo run never resets the rocket, and a parallel one resets each worker once rather than once per simulation, so the text saying a run resets the rocket for you was wrong for both. Per-simulation reset is the Monte Carlo seeding work, not this change. CustomSampler.seed_group already documents that a group belongs to one model and that the last to seed it wins. Saying two components sharing one stay one stream on purpose read as a guarantee this does not make. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * DOC: the parallel path does not get as far as building the tree Saying it resets each worker once reads as though it works and only the grain differs. It hands the model a SeedSequence where an integer is wanted, so it stops before the tree exists, which the PR already records as the Monte Carlo seeding work rather than this change. The two air brake test also says what it is not: both are added with one controller because the rocket keeps a single one, which is #1172. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * TST: pin every word of the seed, and say what append-only means Dropping the fourth word left all 33 tools tests passing: the checks were that the high bits are not zero, that the low word matches, that two children differ and that reading twice agrees, none of which a 96 bit truncation breaks. It compares against the integer rebuilt from all four words now, and that mutation fails. A collection's stream is addressed by where its name falls in the two tuples read end to end, so appending to the first moves every name in the second. The comment said append rather than reorder, which reads as though appending to either one is safe. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * BUG: take the seed type a parallel run hands the rocket A parallel run spawns a SeedSequence per worker and passes it down, and SeedSequence does not take another one as entropy, so rooting the collections from it raised TypeError. It was unreachable until now: the base _set_stochastic refuses the same type one frame earlier, so a worker never got this far. Once that is fixed the call here is the next one to fail, which is why it is fixed in the same series rather than left for whoever hits it. Copied from the full state rather than spawned from directly. spawn() advances the counter of an object the caller still holds, and a second use of the same seed would then build the components a different tree. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * MNT: drop the _seed_sequence_to_int this branch no longer needs to add #1181 landed the same helper on develop, with the same body, so merging develop in left tools.py defining it twice a hundred lines apart. Git had no conflict to report: the two copies were added at different points in the file, so the second simply shadowed the first, and pylint would have failed the branch with E0102 rather than anything explaining why. Develop's copy is kept, its docstring being the fuller of the two. What this branch still adds on its own is _seed_sequence_from, which is left where it was, and the test in test_tools.py now covers the surviving definition. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> Co-authored-by: Gui-FernandesBR <guilherme.fernandes@maggu.ai> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.