Repository navigation
fix(textarena_env): forward reset(seed=...) so episodes are reproducible - #1127
Conversation
|
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. |
There was a problem hiding this comment.
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.
Sent by Cursor Automation: Release
There was a problem hiding this comment.
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.
- The lock is only taken by
_reset_ta_env. A concurrent session can still executeself._ta_env.step(...)while another reset has temporarily calledrandom.seed(seed), so any TextArena game that draws randomness duringstep()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. - 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.
Sent by Cursor Automation: Release
|
There was a problem hiding this comment.
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.
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.
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>
c690a25 to
90b7a6b
Compare
|
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:
This would be a change in TextArena, not OpenEnv, however. Willing to file this as an issue and complete in a follow-up. |
|
@sergiopaniego could you take a look when you have a moment? cc @burtenshaw since you approved the earlier version. |
|
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
left a comment
There was a problem hiding this comment.
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.
|
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:
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. |
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.


Summary
Fixes #1102. Forward reset seeds and preserve Python RNG streams across interleaved sessions:
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
randomand many games draw instep(), the wrapper adds a process-wide_RNG_LOCK,_reset_ta_env, and_session_rngso seeded reset/step stay atomic under concurrent sessions, per-session seeded streams continue throughstep, and the global RNG is restored so unseeded sessions are not perturbed.CI
test.ymlpinstextarena==0.7.4.test_textarena_environment.pyadds integration tests for seed forwarding and variation, plusfake_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.