Skip to content

fix(textarena_env): forward reset(seed=...) so episodes are reproducible - #1127

Merged
burtenshaw merged 9 commits into
openenv:mainfrom
akashjainn:fix/textarena-forward-reset-seed
Oct 27, 2026
Merged

burtenshaw merged 9 commits into
openenv:mainfrom
akashjainn:fix/textarena-forward-reset-seed

Conversation

@akashjainn

@akashjainn akashjainn commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #1102. Forward reset seeds and preserve Python RNG streams across interleaved sessions:

lock → install session RNG → reset/step → save stream → restore process RNG

Construction also takes the lock. Required CI installs TextArena 0.7.4. Main's NLTK and terminal-observation fixes are preserved.

Evidence

Before: global-state preservation and real Bandit interleaving regressions fail.
After: 41 TextArena/NLTK tests pass, including seeded trajectories, concurrency, and exception cleanup. Touched-file lint passes.

Merge Danger

Two-way; TextArena only. Serialization can delay other sessions. NumPy RNG games and unrelated code bypassing the lock remain outside this fix. No public signature change; RFC not required.


Note

Medium Risk
Changes threading and global RNG behavior in a multi-session server; correctness is heavily tested but games that use numpy RNG or TextArena bugs (e.g. Stratego reset) remain partially outside this fix.

Overview
TextArenaEnvironment.reset(seed=...) now forwards the seed to TextArena instead of ignoring it, so the same seed yields the same episode (e.g. Wordle secret word) across resets and environment instances.

Because TextArena seeds the process-global random and many games draw in step(), the wrapper adds a process-wide _RNG_LOCK, _reset_ta_env, and _session_rng so seeded reset/step stay atomic under concurrent sessions, per-session seeded streams continue through step, and the global RNG is restored so unseeded sessions are not perturbed.

CI test.yml pins textarena==0.7.4. test_textarena_environment.py adds integration tests for seed forwarding and variation, plus fake_textarena-based tests for concurrency, RNG restoration on failure, and Bandit step reproducibility.

Reviewed by Cursor Bugbot for commit b08a223. Bugbot is set up for automated code reviews on this repo. Configure here.

@burtenshaw burtenshaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM!

@bot-ci-comment

bot-ci-comment Bot commented Sep 9, 2026

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

cursor[bot]
cursor Bot previously requested changes Sep 9, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forwarding fixes single-session reproducibility, but the pinned TextArena 0.7.4 implementation calls process-global random.seed(seed) during reset. Because this wrapper advertises concurrent sessions, the one-line change leaks seeded state into other sessions and can race. Please either incorporate a serialized save/seed/reset/restore implementation with non-skipped TextArena coverage, or supersede this PR with the corrected preservation approach in #1078.

Open in Web View Automation 

Sent by Cursor Automation: Release

Comment thread envs/textarena_env/server/environment.py Outdated
cursor[bot]
cursor Bot previously requested changes Sep 11, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This head fixes the previously reported reset-vs-reset race: all wrapped resets now share a lock and seeded resets restore the process-global RNG state. Two merge gates remain.

  1. The lock is only taken by _reset_ta_env. A concurrent session can still execute self._ta_env.step(...) while another reset has temporarily called random.seed(seed), so any TextArena game that draws randomness during step() can consume or be perturbed by the seeded stream. Serialize every wrapped operation that can touch TextArena's process-global RNG (or provide dependency-backed proof that step paths cannot draw), and add a deterministic reset-vs-step regression.
  2. Every new dependency-backed test uses pytest.importorskip("textarena"), while required repository CI does not install TextArena. Relevant coverage therefore disappears instead of passing. Ensure the pinned TextArena dependency is installed in a required check and that these regressions are reported as executed.

The branch also conflicts with current main after the NLTK compatibility change. Rebase/merge main while preserving both fixes, rerun exact-head CI, and obtain fresh independent review; the prior approval predates this implementation.

Open in Web View Automation 

Sent by Cursor Automation: Release

@akashjainn

akashjainn commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor Author

This head fixes the previously reported reset-vs-reset race: all wrapped resets now share a lock and seeded resets restore the process-global RNG state. Two merge gates remain.

  1. The lock is only taken by _reset_ta_env. A concurrent session can still execute self._ta_env.step(...) while another reset has temporarily called random.seed(seed), so any TextArena game that draws randomness during step() can consume or be perturbed by the seeded stream. Serialize every wrapped operation that can touch TextArena's process-global RNG (or provide dependency-backed proof that step paths cannot draw), and add a deterministic reset-vs-step regression.
  2. Every new dependency-backed test uses pytest.importorskip("textarena"), while required repository CI does not install TextArena. Relevant coverage therefore disappears instead of passing. Ensure the pinned TextArena dependency is installed in a required check and that these regressions are reported as executed.

The branch also conflicts with current main after the NLTK compatibility change. Rebase/merge main while preserving both fixes, rerun exact-head CI, and obtain fresh independent review; the prior approval predates this implementation.

Open in Web View Automation 
Sent by Cursor Automation: Release

  1. step() now takes _RNG_LOCK too, and test_seed_window_excludes_other_session_operations covers reset, step and construct racing a seeded reset (17d8892).
  2. CI installs textarena==0.7.4 in the test job. Run 34570914785 shows all 15 textarena tests executed and passing on 3.11 and 3.12, none skipped (d7e04e3).

cursor[bot]
cursor Bot previously requested changes Sep 12, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At exact head d7e04e30, the prior lock-coverage and CI-coverage findings are fixed: step() now takes _RNG_LOCK, and required Python 3.11/3.12 CI installs TextArena 0.7.4 and executes all 15 TextArena tests. One behavioral blocker remains.

_reset_ta_env(seed) restores the process RNG state from before reset, discarding TextArena's seeded post-reset RNG continuation. step() then merely takes the lock and draws from whichever process-global state happens to exist. For games such as Bandit that draw during step(), equal seeds plus equal actions are therefore not reproducible across sessions.

I reproduced this against this exact head with the PR's fake TextArena shape: two instances reset with seed=1234 both produced the same reset draw (0.9664535356921388), but their identical first step produced different draws (0.5312232213736315 vs 0.4530922472517809); the seed-1234 continuation should have been 0.4407325991753527.

Please virtualize seeded RNG state per session: under the process lock, restore that session's state before each RNG-using wrapped operation, persist its updated state afterward, and restore the process state on exit (including exceptions), or adopt another design that preserves the full seeded trajectory. Add a same-seed reset-plus-step trajectory regression and an interleaved-session regression. Then rerun exact-head CI and obtain fresh independent approval; the current human approval is for commit 5586da89, before the concurrency implementation.

Open in Web View Automation 

Sent by Cursor Automation: Release

@akashjainn

Copy link
Copy Markdown
Contributor Author

At exact head d7e04e30, the prior lock-coverage and CI-coverage findings are fixed: step() now takes _RNG_LOCK, and required Python 3.11/3.12 CI installs TextArena 0.7.4 and executes all 15 TextArena tests. One behavioral blocker remains.

_reset_ta_env(seed) restores the process RNG state from before reset, discarding TextArena's seeded post-reset RNG continuation. step() then merely takes the lock and draws from whichever process-global state happens to exist. For games such as Bandit that draw during step(), equal seeds plus equal actions are therefore not reproducible across sessions.

I reproduced this against this exact head with the PR's fake TextArena shape: two instances reset with seed=1234 both produced the same reset draw (0.9664535356921388), but their identical first step produced different draws (0.5312232213736315 vs 0.4530922472517809); the seed-1234 continuation should have been 0.4407325991753527.

Please virtualize seeded RNG state per session: under the process lock, restore that session's state before each RNG-using wrapped operation, persist its updated state afterward, and restore the process state on exit (including exceptions), or adopt another design that preserves the full seeded trajectory. Add a same-seed reset-plus-step trajectory regression and an interleaved-session regression. Then rerun exact-head CI and obtain fresh independent approval; the current human approval is for commit 5586da89, before the concurrency implementation.

Open in Web View Automation 
Sent by Cursor Automation: Release

Fixed in 9dd0f6a.

Each session now keeps its own random state. A seeded reset captures the state TextArena leaves behind instead of discarding it, and step() runs under a context manager that swaps that state in, runs the wrapped call, saves the advanced state back, and restores the process state on exit, including on exceptions. Unseeded sessions draw from the process RNG as before, and an unseeded reset clears the saved state.

  1. Same-seed reset-plus-step trajectory: test_seeded_trajectory_is_reproducible_across_sessions checks two sessions agree through three steps and that the draws equal random.Random(1234)'s own stream, so the continuation after the reset draw is the 0.4407325991753527 you computed.
  2. Interleaved sessions: test_seeded_trajectory_survives_interleaved_sessions runs an unseeded session, a differently seeded session and a bare random.random() between every step of the seeded session, and asserts the trajectory is unchanged and the process RNG state is untouched by the seeded steps.
  3. test_failed_seeded_step_restores_rng_and_continues_stream covers the exception path, and test_unseeded_reset_returns_session_to_process_rng pins that unseeded sessions still use the process RNG.
  4. Against the real dependency, test_bandit_seeded_trajectory_is_reproducible_across_sessions plays the same six buttons on two Bandit-v0 sessions at seed 1234 with a third unseeded session acting between every step, and asserts identical reward histories.

The four trajectory tests fail on d7e04e3 and pass on 9dd0f6a.

One boundary worth stating: SpellingBee, IteratedStagHunt and Sokoban draw from numpy.random rather than random, so they sit outside this lock in either design. That is unchanged from main and I would rather handle it separately than widen this PR.

TextArenaEnvironment.reset accepted a seed and never passed it to the
wrapped TextArena env, so seeded runs were not reproducible.

TextArena seeds and draws from the process-global random module, and the
server runs several sessions per process, so each reset's seed-and-draw
runs under a process lock to keep it atomic. A seeded reset still leaves
the global RNG in a known state for other sessions; the proper fix for
that is a per-instance RNG in TextArena.

Fixes openenv#1102

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@akashjainn
akashjainn force-pushed the fix/textarena-forward-reset-seed branch from c690a25 to 90b7a6b Compare October 14, 2026 23:56
@akashjainn

akashjainn commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor Author

Coming back to this, removing the complexity that was added from chasing bot feedback, opting for the one-line seed forwarding with a process lock around reset so seed-and-draw is atomic. This still fixes the issue referenced in #1102 as is. Steps still draw outside the lock however. I propose a fix to this by creating a per-instance generator in TextArena, which would allow:

  • No lock, because there's no shared state to protect.
  • No save-and-restore of the process RNG, because a seeded reset no longer changes anything another session reads.
  • No per-session state swapping around step(), because the game's own generator already carries the seeded stream through every step.

This would be a change in TextArena, not OpenEnv, however. Willing to file this as an issue and complete in a follow-up.

@akashjainn

Copy link
Copy Markdown
Contributor Author

@sergiopaniego could you take a look when you have a moment? cc @burtenshaw since you approved the earlier version.
TLDR: forward the seed in reset() plus a lock around the reset call. It fixes #1102 (same seed gives the same word every time, where main gives a different one each reset).

@burtenshaw

Copy link
Copy Markdown
Collaborator

Fixed review blockers at b08a223: restored per-session RNG continuation and exception-safe process-state restoration; serialized construction/reset/step; required CI installs TextArena 0.7.4. Preserved main's fixes. Real Bandit and global-state regressions failed before; 41 TextArena/NLTK tests pass now. Please re-review; no review dismissal or merge bypass.

@burtenshaw burtenshaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 086f8fb. The earlier RNG/coverage change requests are addressed: construction/reset/step share the lock; seeded reset retains the post-reset stream; steps swap and advance per-session state and restore process state even on exceptions. Main's NLTK fixes are preserved.

Validation: 41 TextArena/NLTK tests passed locally with real TextArena 0.7.4; an additional eight-session concurrent Bandit probe (20 steps each) matched serial trajectories and preserved process RNG state. Required CI logs confirm all 41 tests passed without skips on both Python 3.11 and 3.12. Changed-file Ruff/import-sort/diff checks pass. No new code blockers found.

Scope remains Python's module-level random within wrapped operations. NumPy RNGs (e.g. SpellingBee and IteratedStagHunt) and arbitrary external threads that do not acquire this lock are not isolated; these are existing limitations, not a universal reproducibility guarantee.

The existing inline thread is already resolved. This review does not dismiss the prior Cursor change requests. The branch currently reports BEHIND; refresh and validate the resulting exact head before merging.

@burtenshaw

Copy link
Copy Markdown
Collaborator

Re-reviewed refreshed head b35ea50 (main 8d9d66c). Clean merge; TextArena implementation/tests unchanged by refresh, including main's NLTK and terminal-observation fixes.

All three prior change requests checked:

  • Global reset races and reset-vs-step: construction, reset and step acquire the shared RNG lock; deterministic reset/step/construct exclusion regressions pass.
  • Seeded continuation: reset retains the post-reset state; step installs/advances each session's stream and restores process state in finally. Same-seed, interleaving, real Bandit and reset/step exception regressions pass.
  • Skipped dependency coverage: required Python 3.11/3.12 test workflow explicitly installs textarena==0.7.4. Local refreshed-head run with that version: 41 passed, zero skipped (Python 3.13.14). Changed-file Ruff/format and diff checks pass. Repository-wide format hook reports unrelated existing files.

No new code blocker found. Scope remains Python random used by wrapped operations; NumPy and unrelated threads bypassing this lock remain outside isolation. Five inspected current-head workflows approved to run; current-head green CI is still required before merge.

@cursor please re-review the outstanding change request. The direct review-request API returned HTTP 422 (not a repository collaborator), so the automation owner needs to trigger re-review. No reviews dismissed or protections bypassed.

@burtenshaw
burtenshaw dismissed stale reviews from cursor[bot], cursor[bot], and cursor[bot] October 27, 2026 10:25

Superseded by the implemented per-session RNG virtualization and dependency-backed coverage. Independent reassessment of b35ea50 confirms 41 TextArena/NLTK tests pass without skips, and the fresh human approval documents concurrent Bandit trajectories and process RNG preservation. Original findings are addressed; branch freshness and final CI remain separate merge checks.

@burtenshaw
burtenshaw merged commit 99608b9 into openenv:main Oct 27, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

textarena_env: reset() accepts a seed and discards it, so runs are not reproducible

2 participants