Make the walkthrough seed opt-in: unseeded FrotzEnv is stochastic (v4.0) - #88
Make the walkthrough seed opt-in: unseeded FrotzEnv is stochastic (v4.0)#88abahgat wants to merge 2 commits into
Conversation
Co-authored-by: MarcCote <660004+MarcCote@users.noreply.github.com>
Builds on the opt-in walkthrough-seed change: - Emit ImplicitRandomSeedWarning once per env, at the start of the first episode played without an explicit seeding choice (reset() or, since the env is playable right after construction, a bare step()). Warning only in reset() missed step-without-reset callers, while warning at load time would flag correct usage such as FrotzEnv(rom) followed by reset(use_walkthrough_seed=True) — this covers both. The warned flag is set before warning so users running with -W error get a single exception on a fully constructed env rather than one per reset forever, and stacklevel points the warning at the caller's code. - Treat any direct call to seed() as an explicit seeding choice, including seed() without arguments (a deliberate request for a time-dependent seed); it suppresses the warning for later episodes. - Validate seeds fit in a signed 32-bit int. The emulator receives the seed as a C int, so e.g. seed=2**32-1 (a common shape, like np.random.randint(2**32)) silently wrapped to the -1 time-dependent sentinel, making an explicitly seeded env stochastic. - reset(use_walkthrough_seed=True) on a game with no walkthrough seed warns (UnsupportedGameWarning) and proceeds with the environment's configured seed. The warning previously claimed a time-dependent seed was used, which was wrong whenever an explicit seed was set; it now reports the actual behavior, and a test pins it. - Preserve seed bookkeeping in copy(): a fork no longer silently counts as explicitly seeded, and its docstring clarifies that reset() on a fork does not reproduce a use_walkthrough_seed=True episode. - Strengthen tests. The prior reproducibility check could not catch a broken use_walkthrough_seed flag: 905's walkthrough is RNG-independent (any seed reaches max score) and time seeds have one-second resolution, so back-to-back seeded resets matched by accident. The walkthrough-seeded episode is now cross-validated against an env explicitly constructed with walkthrough_seed. Also new: constructor- episode determinism/stochasticity (load-time seeding was untested), step-without-reset warning, once-per-env warning, seed range/type validation, fallback-seed behavior, and set_state() restoring RNG across envs (the invariant keeping parallel get_valid_actions workers sound now that they no longer share a walkthrough seed). - Docs: correct the get_state/set_state state-tuple docstrings (9 elements, not 7), drop the inaccurate 'equivalent to seed(); reset()' claim (the flag is episode-only), document that reset(True) does not modify the stored seed, and add a README note that time-dependent seeds have one-second resolution (parallel envs created in the same second play identical episodes — pass distinct explicit seeds). - Make the repo's own tests seed explicitly where seeding is not what they test.
| if use_walkthrough_seed: | ||
| if self.walkthrough_seed is None: | ||
| msg = ("No walkthrough seed is known for game '{}'," | ||
| " using the environment's seed instead.").format(self.story_file.decode()) |
There was a problem hiding this comment.
Open question: when the caller explicitly requests use_walkthrough_seed=True but the game has no walkthrough seed, this warns and proceeds with the environment's configured seed. I kept that behavior from the original version (with the message corrected to say what actually happens), but I wonder if it should raise ValueError instead: the caller explicitly asked for a specific deterministic setup, and silently getting different behavior is the same trap #84 is about. Since this is a new API in a major release there's no compatibility cost, and callers can check env.walkthrough_seed is None to handle it gracefully. Happy to change it if you agree.
There was a problem hiding this comment.
That's a great point. I agree, it is better to raise when possible. I'm not a big fan of warning messages since most of the time, they can easily go undetected. So, +1 for raising a ValueError.
| like 2**32-1 (e.g. from np.random.randint(2**32)) would silently wrap to | ||
| the -1 "time-dependent" sentinel, making an explicitly seeded env stochastic. | ||
| ''' | ||
| if seed is None: |
There was a problem hiding this comment.
Open question: the -1 sentinel makes the emulator seed itself from time(0) & 0x7fff (see os_random_seed in dumb_init.c), i.e. one-second resolution and a 15-bit space. Unseeded envs created in the same second play identical episodes, which particularly affects parallel and vectorized runs, arguably the main audience for stochastic-by-default. I've documented the caveat in the README for now, but a stronger fix would be resolving seed=None on the Python side (e.g. from os.urandom) into a concrete random seed and passing that down, which also makes the episode's seed inspectable after the fact. That changes the meaning of the -1 sentinel though, so I left it out of this PR. Thoughts?
There was a problem hiding this comment.
Great catch. I like your proposed idea, dealing with it on the Python does make things more reproducible, we should also probably display which seed was obtained from Python's RNG and used to seed the z-machine. If you can implement this, that would be awesome.
|
@microsoft-github-policy-service agree |
|
@abahgat great PR, thank you for spending time on this. |
Builds on the Copilot agent's work in #86 (its commit is included unchanged) and supersedes that PR with fixes I found while reviewing it.
The main change on top of #86 is when the warning fires. Warning in
reset()misses callers thatstep()right after construction, since the env is playable without a reset. But warning at load time would flag correct usage like constructing unseeded and immediately resetting withuse_walkthrough_seed=True, which is exactly whattools/test_games.pydoes. SoImplicitRandomSeedWarningnow fires once per env at the start of the first episode played without an explicit seeding choice, whether that episode starts withreset()or a barestep(). The warned flag is set before warning so that-W errorusers get one exception on a fully constructed env instead of a constructor abort, and a directseed()call counts as an explicit choice.Smaller fixes, detailed in the commit message:
int.seed=2**32-1(thinknp.random.randint(2**32)) used to wrap silently to the-1time sentinel, turning an explicitly seeded env stochastic.reset(use_walkthrough_seed=True)on a game with no walkthrough seed claimed it fell back to a time-dependent seed. It actually uses the env's configured seed; the message now says so and a test pins it.copy()no longer marks the fork as explicitly seeded as a side effect of how it passes the seed along.get_state/set_statedocstrings described a 7-element tuple (it has 9), and the tutorial claimedreset(use_walkthrough_seed=True)is equivalent toseed(); reset()when it's episode-only.The tests needed the most work. The original reproducibility check couldn't actually fail: 905's walkthrough doesn't consume RNG, so any seed reaches max score, and back-to-back seeded resets match by accident because time seeds have 1-second resolution. I checked by mutating the code so the flag is silently ignored, and the old suite still passed 10/10. The suite now cross-validates the walkthrough-seeded episode against an env explicitly constructed with
walkthrough_seed, comparing RNG registers and world-state hash, and that mutation fails. Also added coverage for constructor-time seeding (previously untested), the step-without-reset warning, andset_state()restoring RNG registers across envs, which is what keeps the parallelget_valid_actionsworkers correct now that they don't share a walkthrough seed.Side note: while testing this I noticed
905.z5's walkthrough seed is0, as are 51 of the 57 bindings, so the test data directly exercises theseed=0bug the oldseed or bindings.get(...)logic had.I left two design questions as inline comments (fallback warn-vs-raise, and whether
seed=Noneshould resolve Python-side instead of using the 1-second-resolution time seed). Both can wait.Fixes #84, supersedes #86.