Cover the submission path #67 actually fixed, and pin the seed derivation - #77
Merged
Merged
Conversation
Member
Author
|
The failing check here is not from this branch. Local run on this branch with |
Merged
…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>
thc1006
force-pushed
the
fix/close-seed-and-submission-gaps
branch
from
July 28, 2026 01:15
de068f1 to
cddc22f
Compare
The packer writes beside its own module, and the test drove it there. It wrote a fixed-name agent file into the installed package, then deleted every submission that had appeared since it started. Anyone evaluating an agent while the suite ran lost that submission, and an existing file of that name was overwritten and then removed. Redirect the module's __file__ into a temporary tree instead, and remove only the directory the test made. Read the result by suffix rather than assuming pickle, so #58 moving the container to JSON does not make this the test that has to be rewritten, in either merge order. Gate the slow class on the stack as well as the flag. Its imports only exist when rocketpy does, so BPC_RUN_SLOW_TESTS=1 without the stack reached setUpClass and raised NameError where the file promises a skip. Measured: one error and nine skips; now eleven skips. Compare the packed score against the run's own popped_count rather than requiring it to be positive. The example agent's aim, the pop geometry and the reward rule can all change while the packer is still correct, and what this covers is that the packer did not rewrite the result. Drop the -5 case. It set Gymnasium's private _np_random_seed by hand, which made a state no public call can produce look like a supported contract. Measured: reset(seed=-5) and seeding.np_random(-5) both raise, and the np_random setter writes exactly -1. The guard stays "< 0" because SeedSequence rejects every negative rather than only -1, and the test now pins that reason instead of a manufactured value. Say in _seed_sequence_to_int what the conversion actually costs. It read as if an int were another spelling of the same entropy; it is not, because default_rng(int) builds a fresh root sequence and drops the child's spawn_key. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.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.
Follow-up to #67, from a post-merge review of it. #67 fixed the crash; this covers the path it fixed and settles what its seed change actually means.
mainis still at v0.0.2 and #61 is still open, so this can land before the release rather than after.The coverage gap, with a number on it
#67's test stops at
sensor.to_dict(). The packer encodes the whole flight, and the failure reached the sensor through Flight, Rocket and the encoder's circular-reference handling.I measured what that costs. Reverting #67 with only the old coverage leaves the suite green; with the tests here it fails 8 of 11, including both that encode the real thing.
Two tests now run the actual boundary: one encodes
env._rocket_flightthroughRocketPyEncoder, the other drives the realpack_for_submissionand reads the file back.Both run a scenario 0 episode to termination. My first attempt stopped after the launch step and failed with
AttributeError: 'Flight' object has no attribute 't_final', becauset_finalis set bypost_process_simulation(). That is my test being unrealistic rather than a bug, sinceevaluate.pyalways runs to termination before packing, but it is worth knowing that the packer only works on a finished flight. They sit behindBPC_RUN_SLOW_TESTS, the gate the golden masters already use, and add about 10s.The seed derivation is now pinned
_seed_sequence_to_intdoes not preserve the stream the spawned child itself would produce.default_rng(child)mixes in the child'sspawn_key;default_rng(int)builds a fresh root. I verified that for all three children, andSeedSequence(**child.state)is what would preserve it.That was the deliberate trade for a seed with a JSON form, and I have corrected #67's description, which claimed the spawn tree was unchanged. Reproducibility and per-sensor separation still hold, and both shipped scenarios run at
noise_density: 0.0so nothing draws from these generators yet.Which is the actual risk: nothing would notice the derivation moving again. The three values are now fixed in a test. Narrowing to 32 bits or dropping the domain tag both stay reproducible and distinct, and both now fail.
Preserving #54's exact streams would need
SeedSequenceserialization support upstream, which is a separate change; I have raised the contract gap there.One review suggestion I did not take
The suggestion was to narrow
seed < 0toseed == -1, on the grounds that -1 is Gymnasium's sentinel.SeedSequencerejects every negative value:So
== -1turns an unexpected negative into a crash at launch, where< 0falls back to drawing entropy. There is a test for it, and making that change fails it.The unreachable branch is gone
Gymnasium's
np_random_seedinitializes rather than ever returningNone, so theNonearm could not run. If it ever had, it handed the sensorsseed=Noneand left them unseeded, which is a fairness problem rather than a coverage one.Noneand negatives now share the fallback.Testing
11 tests in the file, up from 4. Four mutations, each confirmed to apply first:
SeedSequenceagainseed == -1Full suite with
BPC_RUN_SLOW_TESTS=1,uv lock --checkand the CI-pinned ruff 0.15.20: 67 passed.