Skip to content

docs(tutorial): migrate wordle-grpo from rollout_func to environment_… - #601

Merged
burtenshaw merged 5 commits into
huggingface:mainfrom
sergiopaniego:feature/migrate-wordle-grpo-env-factory
May 6, 2026
Merged

docs(tutorial): migrate wordle-grpo from rollout_func to environment_…#601
burtenshaw merged 5 commits into
huggingface:mainfrom
sergiopaniego:feature/migrate-wordle-grpo-env-factory

Conversation

@sergiopaniego

Copy link
Copy Markdown
Member

Summary

docs/source/tutorials/wordle-grpo.md was a port of TRL's Wordle notebook taken before that notebook was migrated to the environment_factory path in TRL #5235 (commit c0e3fb0c). As a result the OpenEnv version still used rollout_func + generate_rollout_completions(), which TRL now keeps only for edge cases like external agent servers (e.g. NeMo-Gym). TRL's canonical OpenEnv integration guide explicitly recommends environment_factory as the default path.

This PR re-ports the tutorial cell-by-cell from the updated upstream notebook. The new tutorial:

  • Defines a WordleEnv class with a guess() tool method and self.reward / self.done state, passed via environment_factory=WordleEnv to GRPOTrainer.
  • Uses a plain reward_func(environments, **kwargs) that reads env.reward.
  • Drops the rollout_func / generate_rollout_completions machinery (~275 lines of glue code + prose).
  • Keeps the Open in Colab badge pointing at the TRL notebook — the upstream notebook is the single source of truth; OpenEnv's Sphinx version is a rendered mirror.

Net: 633 → 358 lines. The tutorial body is bit-identical to the TRL notebook (only change: the Jupyter-only leading ! stripped from the pip install block, since it's rendered inside a ```bash fence rather than a code cell).

Tracked follow-up in .claude/advocacy-comms-plan.md: automate the sync from the upstream TRL notebook at docs build time so this tutorial does not drift again the next time TRL updates the recipe.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation
  • New environment
  • Refactoring

Alignment Checklist

Before submitting, verify:

  • 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 /pre-submit-pr (or bash .claude/hooks/lint.sh and tests) and addressed all issues

(Docs-only change, no Python runtime code edited. Verified via uv run sphinx-build -b html docs/source docs/_build/html: build succeeded with 95 warnings, all pre-existing in envs/repl_env/README.md and unrelated to this PR; zero warnings on docs/source/tutorials/wordle-grpo.md.)

RFC Status

  • Not required (docs)
  • RFC exists
  • RFC needed

Test Plan

  • Diffed the ported content against the upstream notebook — bit-identical except the !pippip strip (1-char difference: 13664 → 13663 chars).
  • uv run sphinx-build -b html docs/source docs/_build/html succeeds; the new page renders at tutorials/wordle-grpo.html with all 15 code blocks syntax-highlighted and no broken internal links.
  • Cross-checked the WordleEnv class against TRL's examples/scripts/openenv/wordle.py and the TRL OpenEnv guide — the class signature, tool docstring shape, reward_func signature, and GRPOTrainer kwargs all match.
  • Confirmed the memory / CLAUDE-level tracked issue (project_trl_integration.md): "wordle-grpo.md still on rollout_func path" is what this PR closes.

@Darktex

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Apr 21, 2026
@greptile-apps

greptile-apps Bot commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR re-ports the Wordle GRPO tutorial from the updated upstream TRL notebook, migrating from the legacy rollout_func + generate_rollout_completions() path to the canonical environment_factory pattern. The result is a net -275 line reduction with the tutorial body now bit-identical to the upstream notebook (minus the notebook-only ! pip prefix), and the Colab badge URL corrected to point at the runnable Colab link rather than the raw GitHub blob.

Confidence Score: 5/5

Safe to merge — docs-only change, no Python runtime code modified, all remaining findings are P2 style/formatting suggestions.

The migration is mechanically correct: WordleEnv correctly encapsulates reward state (aligning with the 'rewards inside environment' invariant), reset() has no docstring so it is not exposed as an agent tool (preserving the agent-cannot-reset invariant), and the reward_func simply reads env.reward. The Colab badge URL fix is a genuine improvement. All open comments are P2 style nits that do not affect rendered correctness or runtime behavior.

No files require special attention.

Important Files Changed

Filename Overview
docs/source/tutorials/wordle-grpo.md Tutorial successfully migrated from rollout_func to environment_factory; 275 lines of glue code removed; three minor style/formatting nits (heading level, bare prose sections, in-loop import)

Sequence Diagram

sequenceDiagram
    participant T as GRPOTrainer
    participant EF as environment_factory
    participant WE as WordleEnv
    participant M as LLM (Qwen3)
    participant RF as reward_func

    T->>EF: Instantiate WordleEnv per rollout
    EF->>WE: reset()
    WE-->>T: initial_observation (str)
    loop up to max_completion_length
        T->>M: Generate completion
        M-->>T: tool_call: guess(word)
        T->>WE: guess(word)
        WE-->>T: feedback (str) + sets self.reward / self.done
        alt env.done == True
            T-->>T: Break loop
        end
    end
    T->>RF: reward_func(environments)
    RF-->>T: [env.reward for env in environments]
    T->>T: GRPO policy update
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: docs/source/tutorials/wordle-grpo.md
Line: 46-47

Comment:
**Inconsistent heading hierarchy**

`### Log in to Hugging Face` is a level-3 heading nested under `## Install dependencies`, but the login step is functionally a top-level section (it's not a sub-step of installing deps). Every other major section in the document uses `##`. Readers skimming the ToC may miss this step or assume it's an optional sub-item of the install block.

```suggestion
## Log in to Hugging Face
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: docs/source/tutorials/wordle-grpo.md
Line: 234

Comment:
**Bare prose blocks need heading markers**

"Show memory stats before training" (line 234) and "Show memory stats after training" (line 253) are bare text floating between code blocks — they render without any visual hierarchy and don't appear in the Sphinx ToC. Every other section in this document uses an `##` heading. This also applies to "And train!" (line 247).

```suggestion
### Show memory stats before training
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: docs/source/tutorials/wordle-grpo.md
Line: 336

Comment:
**`import re` inside loop body**

`re` is imported inside the `else` branch on every iteration of the loop. Tutorial code models best practices — move it to the top of the cell with the other imports.

```suggestion
                import re  # noqa: PLC0415
```
Or better, hoist it to the top of the code block alongside `import json`.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "docs(tutorial): migrate wordle-grpo from..." | Re-trigger Greptile

Comment thread docs/source/tutorials/wordle-grpo.md
Comment thread docs/source/tutorials/wordle-grpo.md
Comment thread docs/source/tutorials/wordle-grpo.md
sergiopaniego added a commit to sergiopaniego/OpenEnv that referenced this pull request Apr 21, 2026
Three mechanical fixes flagged by greptile that improve the rendered Sphinx
version without changing meaning:

- Promote `### Log in to Hugging Face` to `##` — it's a top-level step
  alongside install / prompt / env definition, not a sub-step of install.
  The `###` level in the upstream TRL notebook is an artifact of how
  cells render; in Sphinx it affects the ToC hierarchy.

- Promote the three bare prose lines inside "Create the GRPOTrainer and
  start training" (memory-before, train, memory-after) to `###` headings
  so they appear in the ToC and render with a visual hierarchy instead
  of floating between code blocks.

- Hoist `import re` in the `play_wordle` snippet from inside the else
  branch (import-per-iteration) to the top of the cell alongside
  `import json`. Tutorial code models best practices.

These are Sphinx-specific polish; the upstream TRL notebook still reads
the same way.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

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

Note: This is an automated review by Claude Code, not a human review.


Summary

This PR migrates the Wordle GRPO tutorial from the rollout_func + generate_rollout_completions() path to the environment_factory path, mirroring upstream TRL #5235. The net result is a ~275-line reduction. The motivation is sound and the structure is cleaner. A few issues need addressing before approval.

Tier 1 Findings

  • Wrong import in tutorial code snippet (docs/tutorials/wordle-grpo.md, WordleEnv definition block): The tutorial imports from textarena_env import TextArenaAction, TextArenaEnv. The actual package in this repo lives under envs/textarena_env/ and its __init__.py exports these symbols from openenv-namespaced internals. Readers following the tutorial will need either from openenv.textarena_env import ... or the correct install path. The pip install line (pip install -Uq trl[vllm] git+https://huggingface.co/spaces/openenv/wordle trackio) installs from a HuggingFace Space, not from meta-pytorch/OpenEnv, so textarena_env may or may not be importable as a bare top-level module depending on what that Space's package exposes. This needs clarification: either document that this package name is correct for the Space-installed version, or fix the import to match what users will actually have after the pip install.

  • Missing newline at end of file: The diff ends with \ No newline at end of file on the final line of docs/tutorials/wordle-grpo.md. Lint/editor tooling expects a trailing newline.

  • reset() return type inconsistency: WordleEnv.reset() is annotated -> None | str and returns self._last_full_feedback (a str). However result = self.client.reset() returns a StepResult[TextArenaObservation], and result.observation.messages[0].content can raise IndexError if the messages list is empty on reset. This is a latent runtime bug in the tutorial code.

Tier 2 Findings

ALIGNMENT FLAG: WordleEnv.reset() exposed via environment_factory returns a non-None initial observation, which is then passed as a user message in the inference play_wordle() function. The environment_factory pattern means TRL calls reset() on the environment — this is fine for the training orchestration side. However the tutorial WordleEnv class is a thin wrapper that makes reset() callable by the model in the play_wordle() inference loop (turn 0 path calls env.reset() directly). The invariant is that agents cannot call reset. In the inference code this is benign (there's no trainer), but the class design blurs the line and could be cargo-culted into a context where it matters.

  • Principle at stake: "Agents cannot reset" (INVARIANTS.md, Security Invariants §1; PRINCIPLES.md Key Decisions)
  • The concern: WordleEnv merges training-orchestration (reset) and agent-facing (guess) methods into one class. A reader learning from this tutorial may replicate this pattern in a real OpenEnv environment and accidentally expose reset as a tool.
  • Suggested reviewer: @Darktex

Verdict

Two Tier 1 fixes are required (missing newline, IndexError risk on messages[0]) and one needs clarification (import path vs. pip-installed package name). The Tier 2 flag is low-severity for a docs-only change but worth a quick human sign-off given it touches the "agents cannot reset" invariant in example code. Recommend comment with fixes before approval.


Automated review by Claude Code | Learn more

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

Note: This is an automated review by Claude Code, not a human review.


Alignment Review

Automated Checks

  • Lint: PASS for this file. Pre-existing failures in envs/carla_env/ are unrelated.
  • Debug code: CLEAN.

Tier 1: Issues to Fix

  • Index error on empty messages list — In WordleEnv.reset(), result.observation.messages[0].content will raise IndexError when messages is empty. Per TextArenaObservation in envs/textarena_env/models.py, messages defaults to []. Guard it: result.observation.messages[0].content if result.observation.messages else "".
  • Unverifiable install URL — The pip install block pulls from git+https://huggingface.co/spaces/openenv/wordle (an HF Space URL, not a standard package index/git repo). No other tutorial in this repo installs this way; the existing example in tutorial/examples/wordle.py uses the checked-in textarena_env or a Docker image. This install path is untested and may produce an unusable package.
  • prompt variable undefined in inference section — The inference play_wordle function uses prompt (the system instruction) but prompt is only defined in an earlier, disconnected training block. In a standalone run it is out of scope. Make it a parameter or define it locally.
  • Missing EOF newline — File ends without a trailing newline (visible in the diff). Fix for repo style consistency.

Tier 2: Alignment Discussion

ALIGNMENT FLAG: guidance split between tutorial and example script

  • Principle at stake: "Be hands-on: provide ready-to-use implementations" (PRINCIPLES.md).
  • Concern: tutorial/examples/wordle.py still uses rollout_func + generate_rollout_completions, while the updated tutorial now teaches environment_factory=WordleEnv. Readers who read the tutorial and then run the example script will see contradictory patterns. Should tutorial/examples/wordle.py be migrated in this PR or tracked as a follow-up?
  • Suggested reviewer: @Darktex

Summary

  • 4 mechanical issues to fix.
  • 1 alignment point for human review.

Automated review by Claude Code | Learn more

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

Note: This is an automated review by Claude Code, not a human review.


Alignment Review Report

Automated Checks

  • Lint: PASS — docs-only change, no Python modified, ruff/usort clean.
  • Debug code: CLEAN — no debug artifacts introduced by this PR.

Tier 1: Fixes Required

  • docs/tutorials/wordle-grpo.md (MkDocs-served, lines 149/289/412) — Stale rollout_func content left behind. This PR only updates docs/source/tutorials/wordle-grpo.md (Sphinx). The docs/tutorials/wordle-grpo.md file is the one actually served at the GitHub Pages site via mkdocs.yml (nav: line 111, exclude_docs: | source/). It still contains the old rollout_func / generate_rollout_completions / rollout_func=rollout_func content. The fix is either: (a) apply the same migration to docs/tutorials/wordle-grpo.md, or (b) make it a symlink / include of the Sphinx source, or (c) remove it from the MkDocs nav if Sphinx is now the canonical build. This is the core blocking issue.

  • docs/source/tutorials/wordle-grpo.md (last line) — Missing newline at end of file. The diff shows \ No newline at end of file at line 868. Add a trailing newline.

Tier 2: Alignment Discussion

None identified. The environment_factory pattern keeps the reward inside the environment (self.reward set in WordleEnv.guess()), which is consistent with the Rewards inside environment principle (RFC 002). The reset() method is called only by the TRL trainer infrastructure — the agent only sees the guess() tool — which is consistent with the Agents cannot reset invariant (RFC 001 / INVARIANTS.md §Security). The environment_factory kwarg is a TRL-side parameter and OpenEnv's own src/ API is not touched.

Summary

  • 2 mechanical issues to fix (stale MkDocs copy, missing EOF newline)
  • 0 alignment points for human review

The content of the migration itself is correct and well-written. Once docs/tutorials/wordle-grpo.md is updated (or the MkDocs nav is pointed at the Sphinx source), this is ready to merge.


Automated review by Claude Code | Learn more

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

Note: This is an automated review by Claude Code, not a human review.


Alignment Review Report

Automated Checks

  • Lint: PASS (for this PR's file) - pre-existing failures in envs/carla_env/ are unrelated to this change
  • Debug code: CLEAN - no debug artifacts introduced

Tier 1: Fixes Required

  • docs/tutorials/wordle-grpo.md (end of file) - File is missing a trailing newline (\ No newline at end of file in diff). Please add one.

Tier 2: Alignment Discussion

None identified. The environment_factory pattern is correctly aligned with OpenEnv principles:

  • reward_func reads env.reward rather than computing rewards externally, satisfying the "rewards inside environment" invariant.
  • reset() is correctly exempted from tool auto-discovery per the documented rule; the tutorial prose makes this explicit.
  • No simulation controls are exposed to the agent.

Minor Notes

  • The Colab badge URL fix (raw GitHub link -> proper colab.research.google.com/github/... URL) is correct and a genuine improvement.
  • PR body references .claude/advocacy-comms-plan.md as a tracked follow-up but that file does not exist in the repo. Consider either creating it or removing the reference from the PR description to avoid confusion.
  • The new install command (pip install trl[vllm] git+https://huggingface.co/spaces/openenv/wordle) installs the env from an HF Space rather than from the main OpenEnv repo. Worth a brief prose note explaining why (hosted Space is the canonical distribution for this env).

Summary

  • 1 mechanical issue to fix (missing EOF newline)
  • 0 alignment points requiring human review

Automated review by Claude Code | Learn more

sergiopaniego added a commit to sergiopaniego/OpenEnv that referenced this pull request Apr 24, 2026
Addresses the one mechanical nit raised across multiple review passes
on huggingface#601: the migrated tutorial was missing an EOF newline. Keeps the
file bit-identical to the upstream TRL notebook in every other
respect so the planned auto-sync does not drift.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@sergiopaniego

Copy link
Copy Markdown
Member Author

Some comments have been applied. Others are ignored since my idea is to keep the file identical to https://github.com/huggingface/trl/blob/main/examples/notebooks/openenv_wordle_grpo.ipynb

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

Note: This is an automated review by Claude Code, not a human review.


Docs tutorial migration from rollout_func to environment_factory — good intent, two concrete bugs need fixing before merge.

Tier 1: Fixes Required

  • docs/tutorials/wordle-grpo.md (new WordleEnv.reset()) — IndexError on empty messages list. The implementation unconditionally accesses result.observation.messages[0].content on the StepResult returned by self.client.reset(). TextArenaObservation.messages is typed as List[TextArenaMessage] with default_factory=list, so it can be empty at episode start. This will raise IndexError on any environment that emits the initial observation in prompt rather than messages[0]. The safe pattern from the old tutorial was to use observation.prompt for the initial text. Fix:

    def reset(self, **kwargs) -> None | str:
        result = self.client.reset()
        obs = result.observation
        # Use prompt field for initial text; fall back to first message if present
        initial_text = obs.prompt or (obs.messages[0].content if obs.messages else "")
        self._last_full_feedback = initial_text
        self.reward = 0.0
        self.done = False
        return self._last_full_feedback or None

    The same issue exists symmetrically in guess(): result.observation.messages[0].content is accessed unconditionally after each step, which will also raise if messages is empty on a terminal step.

  • docs/tutorials/wordle-grpo.md (install block) — The pip install line pip install -Uq trl[vllm] git+https://huggingface.co/spaces/openenv/wordle trackio installs from an HF Space URL that does not exist in the OpenEnv repo or any known public URL as of this diff. The old tutorial installed from git+https://github.com/meta-pytorch/OpenEnv.git. If the openenv/wordle Space is not yet public or the package is not pip-installable from that URL, readers will get an install failure immediately. This needs either a working URL or a note that the Space must be duplicated first.

  • docs/tutorials/wordle-grpo.md (GRPOConfig) — gradient_checkpointing_kwargs={"use_reentrant": False} was silently dropped from the new config. With PyTorch < 2.2, omitting this with gradient_checkpointing=True triggers a UserWarning about use_reentrant defaulting to True, and in some setups causes training instability. It should be restored for robustness.

Tier 2: Alignment Discussion

ALIGNMENT FLAG: WordleEnv exposes reset() as a public method on the same object that provides the guess() tool

  • Principle at stake: INVARIANTS.md §Security Invariants — "Agents cannot access reset/simulation controls"; "MCP tools must not expose simulation control to agents"
  • The concern: The tutorial's WordleEnv class bundles reset() (simulation control) and guess() (agent tool) in the same object. The PR description says environment_factory only auto-discovers non-reset public methods as tools, which is correct for the training path. However, the tutorial does not explain this distinction to readers, who may copy this pattern and accidentally expose reset() if they implement their own environment_factory adapter without understanding that the trainer's tool-discovery logic is what protects the boundary — not the class design itself. A brief comment in the WordleEnv class or surrounding prose clarifying that reset() is NOT exposed as a tool (only guess() is, because it has a docstring and is not named reset) would prevent cargo-culting that violates the invariant.
  • Suggested reviewer: @Darktex

Correct improvements worth highlighting

  • dtype="auto"torch_dtype="float32": the old tutorial used the wrong kwarg name for AutoModelForCausalLM.from_pretrained.
  • max_completion_length 8 → 1024: far more reasonable for Wordle guesses.
  • Colab badge URL corrected to the launch URL.

Automated review by Claude Code | Learn more

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

Note: This is an automated review by Claude Code, not a human review.


Alignment Review Report

Automated Checks

  • Lint: PASS (lint failures are pre-existing in carla_env, unrelated to this docs-only PR)
  • Debug code: CLEAN

Tier 1: Fixes Required

  • Install command may be broken (pip install block): git+https://huggingface.co/spaces/openenv/wordle is not a standard pip-installable URL - HF Spaces host FastAPI apps, not Python packages. If the upstream TRL notebook uses this, it presumably resolves correctly there, but readers following this tutorial verbatim may get a pip error. Recommend verifying the install command actually works, or noting that this requires a pip-compatible HF Space repo layout.

  • Bare except Exception in inference code (play_wordle function): The except Exception as e: print(...); break swallows all errors silently. For a tutorial, this hides bugs from learners. At minimum, the error should be re-raised or the caught exception types should be narrowed.

  • Missing gradient_checkpointing_kwargs: The old config had gradient_checkpointing_kwargs={"use_reentrant": False} which is important for compatibility with newer PyTorch. The new config silently drops it. If this is intentional (TRL now sets a safe default), it should be noted with a comment.

Tier 2: Alignment Discussion

ALIGNMENT FLAG: reset() callable from inference-side play_wordle

  • Principle at stake: INVARIANTS.md - "Agents cannot reset" / "The Gym-like API is NOT accessible to the agent being trained."
  • The concern: In the inference section, play_wordle calls env.reset() directly on WordleEnv. WordleEnv is a TRL adapter (not an OpenEnv Environment subclass), and this is post-training inference, not training. However, the tutorial teaches readers a pattern where reset() is called from the "agent loop" function. This is a teaching anti-pattern relative to OpenEnv's invariant: new contributors reading this tutorial may carry the pattern into OpenEnv environment authoring where it would violate the invariant.
  • Suggested reviewer: @Darktex

Summary

  • 3 mechanical issues to fix (install URL validity, bare exception, dropped gradient_checkpointing_kwargs)
  • 1 alignment point for human review (reset() accessibility pattern in tutorial code)

Overall the migration direction is correct and the diff is a meaningful simplification. The Colab badge fix (GitHub blob -> colab.research.google.com/github/) is a genuine bug fix. The core concern is whether the install command is actually runnable.


Automated review by Claude Code | Learn more

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

Note: This is an automated review by Claude Code, not a human review.


Alignment Review

Automated Checks

  • Lint: PASS (pre-existing failures in carla_env are unrelated to this PR)
  • Debug code: CLEAN

Tier 1: Fix Required

Install URL needs verification (docs/source/tutorials/wordle-grpo.md, install block)

The PR replaces the old pip install ... git+https://github.com/meta-pytorch/OpenEnv.git ... with:

pip install -Uq trl[vllm] git+https://huggingface.co/spaces/openenv/wordle trackio

The existing in-repo tutorial/examples/wordle.py still references burtenshaw/wordle (Space URL: https://burtenshaw-wordle.hf.space), while the new tutorial targets the openenv org (https://openenv-wordle.hf.space). If huggingface.co/spaces/openenv/wordle is not yet live, the tutorial is broken from the first code cell. Please confirm the Space exists and is pip-installable before merging, or add a fallback note.

Tier 2: Alignment Discussion

ALIGNMENT FLAG: WordleEnv teaches a non-OpenEnv API shape

  • Principle at stake: "Minimize lifecycle deltas" (PRINCIPLES.md) + Gymnasium API signature invariant (INVARIANTS.md §API Invariants 1)
  • The concern: WordleEnv.reset() returns None | str and state is held in plain attributes (self.reward, self.done) rather than an Observation Pydantic model. This is the correct TRL environment_factory contract, but readers may adopt it as the canonical pattern for building OpenEnv environments. A single-sentence callout noting "this is the TRL wrapper shape, not the OpenEnv server shape" would prevent confusion.
  • Suggested reviewer: @Darktex

What's Good

  • The Colab badge fix (raw GitHub URL → actual Colab launch URL) is a genuine bug fix.
  • Reward computation stays inside the environment (env.reward read by reward_func), which aligns with the "rewards inside environment" principle (RFC 002).
  • Net -274 lines is a real readability improvement; the upstream-sync rationale in the PR body is solid.
  • reset() does not appear in the MCP/tool surface — agent isolation invariant is not violated.

Automated review by Claude Code | Learn more

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

Note: This is an automated review by Claude Code, not a human review.


Alignment Review Report

Automated Checks

  • Lint: PASS (lint failures in carla_env are pre-existing and unrelated to this PR; docs/source/tutorials/wordle-grpo.md is a Markdown file)
  • Debug code: CLEAN

Tier 1: Fixes Required

  • Wrong file path: The diff targets docs/source/tutorials/wordle-grpo.md, but that path does not exist in the repository. The actual file is at docs/tutorials/wordle-grpo.md. The PR either modifies a file in a non-existent path (which may mean the change was never applied to the correct location) or the repo has a docs/source/ tree that only exists in a Sphinx build output that is not checked in. The current stale docs/tutorials/wordle-grpo.md still contains the old rollout_func code. This must be reconciled before merge: either update docs/tutorials/wordle-grpo.md, or confirm that docs/source/tutorials/wordle-grpo.md is the canonical rendered-source path and add it to the repo.

  • tutorial/04-training.md still uses rollout_func: /home/davidet/OpenEnv/tutorial/04-training.md is a copy of the old Wordle tutorial and still contains the full rollout_func + generate_rollout_completions pattern (lines 149, 412). If the intent of this PR is to migrate the canonical tutorial, this file should also be updated or explicitly noted as deprecated.

  • tutorial/examples/wordle.py still uses rollout_func: /home/davidet/OpenEnv/tutorial/examples/wordle.py (lines 474, 521) is the runnable example script accompanying the tutorial and still uses rollout_func. A reader following the linked .py script will get conflicting guidance. Either migrate this file too or add a comment/deprecation notice.

  • envs/finqa_env/README.md also uses rollout_func: /home/davidet/OpenEnv/envs/finqa_env/README.md (lines 159, 167) shows a rollout_func snippet for TRL integration. This is a separate environment, but if rollout_func is being deprecated in favor of environment_factory, this example should be updated or a note added.

  • pip install source is unverifiable: The new install line is pip install -Uq trl[vllm] git+https://huggingface.co/spaces/openenv/wordle trackio. Installing from a Hugging Face Space URL (git+https://huggingface.co/spaces/openenv/wordle) is unusual and that Space does not appear to be a standard Python package hosted on the Hub. Readers may not be able to reproduce this install. Verify the Space has a valid setup.py/pyproject.toml and is intended as a pip-installable package, or replace with a standard PyPI or GitHub URL.

  • from textarena_env import TextArenaAction, TextArenaEnv missing module origin: The new code imports from textarena_env import TextArenaAction, TextArenaEnv (no envs. prefix) after installing from the Space URL above. The old code used from envs.textarena_env import .... Readers need to know whether pip install git+https://huggingface.co/spaces/openenv/wordle places a top-level textarena_env package on their path, or if this is a bare module name that only works inside the Space container. This should be documented or the import should match what the install step produces.

  • play_wordle inference block has fragile JSON tool-call parsing: The new inference demo (around line 833 in the diff) manually parses tool calls from model output using generated_text.index("{") / rindex("}"). This will raise a ValueError on models that do not output a JSON object (caught by the bare except Exception), but the silent break means users get no indication of parse failure. A clearer fallback message would improve the tutorial experience.


Tier 2: Alignment Discussion

ALIGNMENT FLAG: Reward read from mutable environment attribute, not returned from step

  • Principle at stake: "Rewards inside environment" (PRINCIPLES.md, RFC 002) — domain reward knowledge must be encapsulated inside the environment boundary
  • The concern: The reward_func in the new tutorial reads env.reward directly from a public attribute that was mutated inside WordleEnv.guess(). This is fine from an encapsulation standpoint, but it diverges from the canonical OpenEnv pattern where reward is returned as a field on the Observation object from step(). Future readers may adopt this attribute-mutation pattern for their own environments and bypass OpenEnv's Observation.reward convention. It is worth a note clarifying that this is a TRL environment_factory adapter pattern, not the general OpenEnv reward convention.
  • Suggested reviewer: @Darktex

Summary

  • 5 mechanical issues to fix (path mismatch, sibling files not migrated, fragile install command, import clarity, inference parse robustness)
  • 1 alignment point for human review (reward-via-attribute pattern vs canonical Observation.reward)

The migration of the core training loop from rollout_func to environment_factory is correct and the new tutorial is substantially simpler. The primary blocker is the file-path discrepancy and the fact that sibling files (tutorial/04-training.md, tutorial/examples/wordle.py) still present the old API.


Automated review by Claude Code | Learn more

sergiopaniego and others added 3 commits May 6, 2026 14:04
…factory

The tutorial was a port of TRL's examples/notebooks/openenv_wordle_grpo.ipynb
taken before that notebook was migrated to the environment_factory path in
TRL PR #5235 (commit c0e3fb0c). As a result the OpenEnv version still used
rollout_func + generate_rollout_completions(), which is kept in TRL only for
edge cases like external agent servers (e.g. NeMo-Gym). TRL's recommended
path for OpenEnv is now environment_factory.

Re-port from the updated upstream notebook cell-by-cell. The new tutorial:

- Defines a WordleEnv class with a guess() tool method and self.reward / self.done
  state, passed via environment_factory=WordleEnv to GRPOTrainer.
- Uses a plain reward_func(environments, **kwargs) that reads env.reward.
- Drops the rollout_func / generate_rollout_completions machinery (~275 lines).
- Keeps the Colab badge pointing at the TRL notebook (single source of truth).

Net: 633 -> 358 lines, bit-identical to the TRL notebook (only change: the
Jupyter-only leading `!` stripped from the pip install block, since it's
rendered inside a bash fence rather than a code cell).

Tracked follow-up: automate the sync from the upstream TRL notebook so the
OpenEnv tutorial does not drift again the next time TRL updates the recipe.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three mechanical fixes flagged by greptile that improve the rendered Sphinx
version without changing meaning:

- Promote `### Log in to Hugging Face` to `##` — it's a top-level step
  alongside install / prompt / env definition, not a sub-step of install.
  The `###` level in the upstream TRL notebook is an artifact of how
  cells render; in Sphinx it affects the ToC hierarchy.

- Promote the three bare prose lines inside "Create the GRPOTrainer and
  start training" (memory-before, train, memory-after) to `###` headings
  so they appear in the ToC and render with a visual hierarchy instead
  of floating between code blocks.

- Hoist `import re` in the `play_wordle` snippet from inside the else
  branch (import-per-iteration) to the top of the cell alongside
  `import json`. Tutorial code models best practices.

These are Sphinx-specific polish; the upstream TRL notebook still reads
the same way.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the one mechanical nit raised across multiple review passes
on huggingface#601: the migrated tutorial was missing an EOF newline. Keeps the
file bit-identical to the upstream TRL notebook in every other
respect so the planned auto-sync does not drift.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@sergiopaniego
sergiopaniego force-pushed the feature/migrate-wordle-grpo-env-factory branch from 6ab30c1 to d0347e0 Compare May 6, 2026 12:05
Reverts the four formatting changes introduced in the greptile review
commit: heading level on 'Log in to HF' (### not ##), bare-text memory
stats labels (no ### prefix), and import re back inside the else branch.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@burtenshaw
burtenshaw dismissed stale reviews from Darktex, Darktex, Darktex, and Darktex May 6, 2026 12:28

Dismissed stale automated review after maintainer confirmed the tutorial migration is safe to merge.

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

Approved per maintainer confirmation that the Wordle GRPO migration is safe to merge.

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

Approved per maintainer confirmation after required checks passed.

@burtenshaw
burtenshaw merged commit 33976ba into huggingface:main May 6, 2026
7 of 9 checks passed
@sergiopaniego
sergiopaniego deleted the feature/migrate-wordle-grpo-env-factory branch May 6, 2026 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants