Skip to content

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

Open
akashjainn wants to merge 5 commits into
huggingface:mainfrom
akashjainn:fix/textarena-forward-reset-seed
Open

fix(textarena_env): forward reset(seed=...) so episodes are reproducible#1127
akashjainn wants to merge 5 commits into
huggingface:mainfrom
akashjainn:fix/textarena-forward-reset-seed

Conversation

@akashjainn

@akashjainn akashjainn commented Sep 9, 2026

Copy link
Copy Markdown

Summary

TextArenaEnvironment.reset() accepted a seed and dropped it, so episodes were never reproducible even though the signature said they were. This forwards it to TextArena's own reset, which has always taken one.

Fixes #1102.

Type of Change

  • Bug fix

Alignment Checklist

  • I have read .claude/docs/PRINCIPLES.md and this PR aligns with our principles
  • I have checked .claude/docs/INVARIANTS.md and no invariants are violated
  • I have run bash .claude/hooks/lint.sh and tests and addressed all issues

Reproducibility is something the project already optimises for, and the Gymnasium-style reset/step/state API is a stated principle. A seed parameter that is accepted and ignored works against both.

RFC Status

  • Not required (bug fix, docs, minor refactoring)

Test Plan

Before, on Wordle-v0, reading the secret word off TextArena's own game state:

six resets at seed=1234: clock, sugar, month, price, flame, steam

After:

seed=1234 x6 : slope, slope, slope, slope, slope, slope -> 1 distinct
seed=999  x3 : cheap, cheap, cheap                      -> 1 distinct
no seed   x6 : trick, twist, taste, start, stage, curve -> 6 distinct
stable across instances : True

slope matches what the reporter independently got when they tried forwarding the seed.

Three tests added to tests/envs/test_textarena_environment.py:

  • a fixed seed gives one episode and a different seed gives another
  • the same seed survives constructing a fresh environment
  • unseeded resets still vary, so forwarding does not accidentally pin them

The first two fail without the change; the third passes either way and is there to catch an over-broad fix.

PYTHONPATH=src:envs pytest tests/envs/test_textarena_environment.py   5 passed
PYTHONPATH=src:envs pytest tests/                                     1415 passed, 121 failed
                                                                      (121 identical on main; unrelated missing env deps)
ruff format --check / ruff check on both changed files                clean

Notes

The issue asked whether forwarding is correct for every wrapped TextArena game, or whether raising on a non-None seed would be safer. seed: Optional[int] = None is on Env.reset and on Wrapper, ObservationWrapper, ActionWrapper and RenderWrapper, so it is safe through the whole chain, and determinism holds on Wordle, GuessTheNumber, Hangman, Crosswords, Sudoku and FifteenPuzzle. So forwarding rather than raising.

__init__ still resets unseeded. That call only exists to leave the environment valid before the first step(), and reset() replaces the state it creates, so seeding it would mean widening the constructor signature for no behaviour change. Happy to do it if you would rather the constructor took a seed.

@caiotheodoro offered to open a PR for this in the issue. I had it written before picking the issue up and thought the multi-game determinism check was worth having either way, but I am glad to close this if they would rather carry it.

Games that draw from random inside step() are not covered by the lock and
can still interleave with a seeded reset elsewhere. Wordle draws only at reset.

Related: #1078 takes the same approach but locks only seeded resets, which
leaves the interleaving open, and seeds numpy directly, which rejects seeds
above 2^32.


Note

Medium Risk
Changes concurrent-session RNG behavior behind SUPPORTS_CONCURRENT_SESSIONS; correctness depends on lock coverage for all TextArena games that touch random during reset/step.

Overview
Fixes ignored reset(seed=...) so TextArena episodes are reproducible: reset() now calls a new _reset_ta_env(seed) that forwards the seed to the wrapped game (including the initial post-construct reset).

Because TextArena uses the process-global random module, the PR adds a _RNG_LOCK around reset and step. Seeded resets save and restore the global RNG state so one client’s seed does not perturb unseeded sessions; step() is locked too for games that draw during steps (e.g. Bandit).

CI installs textarena==0.7.4 for tests. test_textarena_environment.py gains Wordle-based seed/reproducibility tests, RNG-leak and cross-session isolation checks, concurrency tests, and a fake_textarena fixture so lock/seed logic runs without the optional package.

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

TextArenaEnvironment.reset() accepts a seed, as every Environment does, and
called self._ta_env.reset(num_players=...) without it. The seed had no effect
and nothing reported that, so a caller reading the signature would reasonably
believe an episode was reproducible when it was not.

Six resets at seed=1234 on Wordle-v0 gave six different secret words
(clock, sugar, month, price, flame, steam). With the seed forwarded they give
"slope" every time, and a fresh environment at the same seed gives it too.

TextArena's own reset takes seed on Env and on Wrapper, ObservationWrapper,
ActionWrapper and RenderWrapper alike, so forwarding is safe through the
wrapper chain OpenEnv builds. Checked determinism on Wordle, GuessTheNumber,
Hangman, Crosswords, Sudoku and FifteenPuzzle: unseeded resets vary, a fixed
seed is stable, and different seeds differ.

__init__ still resets unseeded. It only exists to leave the env in a valid
state before the first step(), and reset() replaces that state, so threading a
seed through the constructor would widen the change without changing behaviour.

Fixes huggingface#1102

@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 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

env.full_observations = {}

self._ta_env.reset(num_players=self.num_players)
self._ta_env.reset(num_players=self.num_players, seed=seed)

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 call mutates process-global RNG state in TextArena 0.7.4 (SinglePlayerState.__init__State.__init__random.seed(seed)). A concurrent or subsequent unseeded session therefore observes the seeded stream. Serialize all underlying resets with a process-wide lock and save/restore RNG state for seeded calls; add coverage that proves seeded resets neither perturb nor race an unseeded session.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in: 11bd842

All underlying resets, including the one in __init__, now go through
_reset_ta_env. It holds a process-wide lock so seed and draw are atomic, and
restores the prior random state after a seeded reset. Unseeded resets take
the same lock, so they cannot land inside another session's seed window.

… sessions

reset() accepted a seed and dropped it, so seeded episodes were not
reproducible (huggingface#1102). Forwarding alone is not enough: TextArena applies the
seed with random.seed() on the process-global RNG and then draws the episode
from it, and this environment serves concurrent sessions on separate threads.
A bare forward leaks the seeded stream into unseeded sessions and lets another
session's reset steal the seeded draw.

Route every underlying reset, including the one in __init__, through a helper
that holds a process-wide lock and restores the prior RNG state after a seeded
reset. Seeded and unseeded episodes now behave independently.

Fixes huggingface#1102.

@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
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 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

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