Give sensors JSON-safe seeds so submissions can be packed again - #67
Conversation
A launched rocket currently cannot be packed for submission. pack_for_submission runs the flight through RocketPyEncoder, the flight carries the rocket, the rocket carries its sensors, and a sensor hands back the seed it was built with. Since #54 that seed is a SeedSequence, which the encoder cannot serialize, so packing raises TypeError: Object of type SeedSequence is not JSON serializable. Both scenarios are affected even though their sensors are noise free, because the object is stored regardless. Derive a plain 128-bit int from each spawned child instead. The domain separation and the per-sensor decorrelation are unchanged, and with noise-free sensors the trajectories and scores are identical, which both golden masters confirm. Also stop Gymnasium's unknown-seed marker from crashing the launch: np_random_seed is -1 when np_random was assigned directly, and SeedSequence rejects negative entropy. Draw the entropy from the generator in that case. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
thanks for the fix |
|
Post-merge review of this one. The crash it fixed is fixed; two things it left open are now closed in #77, and one suggestion I looked at and did not take. The stream really did change. Accepting the change rather than reverting it, because both shipped scenarios run at The test stopped one layer short of the failure. This PR's test encodes While writing that I hit One suggestion I did not take, in case it comes up again: narrowing So that change converts an unexpected negative into a crash at launch, where The |
…tion #67 stopped submissions crashing, but its test stops at sensor.to_dict() while the packer encodes the whole flight. Reverting #67 with only that coverage in place leaves the suite green on nine of eleven checks; with the tests here it fails eight, including the two that encode the real thing. Three gaps closed. The production boundary is now exercised: one test encodes env._rocket_flight through RocketPyEncoder, another drives the real pack_for_submission and reads the file back. Both run a scenario 0 episode to termination, because that is the state the packer is called in. An unfinished flight has no t_final and its to_dict() raises AttributeError, which says nothing about whether a submission can be packed. They sit behind BPC_RUN_SLOW_TESTS, the gate the golden masters already use. The seed derivation is pinned to fixed values. Converting a spawned child to an int does not preserve the stream the child itself would produce, since default_rng(child) mixes in its spawn_key while default_rng(int) builds a fresh root. That was the deliberate trade for a seed with a JSON form, and both shipped scenarios run at noise_density 0.0 so nothing draws from these generators yet, which is exactly why nothing would notice the derivation moving again. Narrowing to 32 bits or dropping the domain tag both stay reproducible and distinct, and both now fail. The unreachable branch is gone. Gymnasium's np_random_seed property initializes rather than ever returning None, so the None arm could not run; if it ever had, it handed the sensors seed=None and left them unseeded, which is a fairness problem rather than a coverage one. None and negative values now share the fallback that draws entropy from the generator. On the review suggestion to narrow this to seed == -1: SeedSequence rejects every negative value, not just -1, so that change turns an unexpected negative into a crash at launch instead of a usable seed. There is a test for it now, and making that change fails it. Two RNG contracts are pinned separately: a known seed leaves np_random untouched, and the unknown-seed path consumes exactly four uint32. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
…-gaps Cover the submission path #67 actually fixed, and pin the seed derivation
A competitor cannot pack a submission on
developright now. Worth looking at before the next release.What happens
pack_for_submissionserializes the flight withRocketPyEncoder. The flight carries the rocket,Rocket.to_dict()includes its sensors, and a sensor hands back whatever seed it was constructed with. Since #54 that seed is aSeedSequence, and the encoder has no handler for it, so it falls through to the stdlib encoder and raises.Both scenarios are affected. Their sensors are noise free, but the object is stored either way, so packing fails after any launch.
I reproduced it by running the real
pack_for_submissionon scenario 0 against currentdevelop.Why nothing caught it
The sensor tests check observation streams, and the golden masters check trajectories. Neither goes near
RocketPyEncoderorpack_for_submission, and there was no submission-serialization test on either side of the submodule. ActiveRocketPy's side is symmetrical:Sensor.to_dict()stores the seed unchanged, so any non-JSON seed leaks into the public serialization path.The change
Each spawned child is converted to a plain 128-bit int before it reaches a sensor. The domain tag and the per-sensor decorrelation are unchanged, and the streams stay reproducible.
Correction to what I wrote here originally: this is not representation-only.
default_rng(child)uses the child'sspawn_key, whiledefault_rng(int)builds a fresh rootSeedSequence, so the exact bit streams differ. I checked all three children and none match. Preserving them exactly would mean serializingchild.stateand rebuilding withSeedSequence(**state), which needs encoder support upstream. With both shipped scenarios atnoise_density: 0.0the generator is never drawn from, so nothing changes today, and both golden masters confirm that. It does mean the sensor streams from #54 are not the streams a noisy scenario would get, which is worth settling before scenario 2 ships.With noise-free sensors the RNG is never drawn from, so trajectories and scores are unaffected. Both golden masters pass unchanged, which is the evidence for that.
While in the same lines: Gymnasium reports
np_random_seed == -1whennp_randomwas assigned directly rather than seeded, andSeedSequencerejects negative entropy, so that path raisedValueErrorat launch. The entropy now comes from the generator when the seed is unknown.Tests
tests/test_submission_serialization.pycovers the gap: sensor seeds are plain ints, each sensor'sto_dict()survives the encoder, the same seed replays while a different one diverges and no two sensors share a seed, and a directly assigned generator no longer crashes the launch.Full suite with
BPC_RUN_SLOW_TESTS=1: 49 passed plus 10 subtests, both golden masters included.Related: this is the interaction between #54 and the ActiveRocketPy update in #60. Follow-ups from the same review are in #64.