Fix OpenEnv examples broken by async-first client API#6194
Fix OpenEnv examples broken by async-first client API#6194qgallouedec wants to merge 11 commits into
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5762271623
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def __init__(self): | ||
| self.client = OpenSpielEnv(base_url=env_url) | ||
| # OpenEnv's client API is async-first; .sync() exposes blocking reset()/step(). | ||
| self.client = OpenSpielEnv(base_url=env_url).sync() |
There was a problem hiding this comment.
Update docker-mode bootstrap for async OpenEnv constructors
This only wraps clients created from an already resolved URL. In the default --env-mode docker-image path above, _bootstrap = OpenSpielEnv.from_docker_image(...) is now a coroutine under the async-first API, so _bootstrap.base_url fails before this environment is ever constructed; the same pattern exists in the docker-hub path and in the Sudoku script. Please update those bootstrap paths to await/run the async constructor (or otherwise derive a URL from the provider) rather than only adding .sync() here.
Useful? React with 👍 / 👎.
|
I'd prefer to fix this via this via a detector on the OpenEnv side huggingface/OpenEnv#874 |
|
I agree with Ben. Also linking here his PR in TRL (#6138) so we can have everything tracked |
|
Ok as you want, you're leading this, but remember that in the meantime, all examples are broken |
|
With the release of OpenEnv 0.4.0 we won't need this PR. The server now detects whether the client is syn/async so no need for That said, there is still a separate issue mentioned here: from_docker_image() remains async in 0.4.0, so the TRL docker-image bootstrap paths that do _bootstrap = Env.from_docker_image(...); _bootstrap.base_url can still fail. #6194 does not fix that. so I'd open a new PR. |
|
in the case of Agent World Model env in OpenEnv, sync env reset is a bottleneck so that sampler and trainer have to wait for reset. for example, max inflight tasks are 392, it takes ~15 minutes to reset. In my opinion, it needs to implement env reset in async. It's simple to implement this.
Should this be avoided by design (simplicity)? |
|
#6329 solving the |
AmineDiro
left a comment
There was a problem hiding this comment.
LGTM.
Pythonic clients usually could either have : sync/async methods or have a different SyncClient or AsyncClient concrete classes that have different methods. But that's something to discuss with the OpenEnv team
|
closed by #6329 |
OpenEnv made
EnvClient.reset()/.step()async-first (huggingface/OpenEnv#343), so calling them synchronously now returns a coroutine ('coroutine' object has no attribute 'observation'). Every TRL OpenEnv example still used the sync pattern.Fix: append
.sync()at client construction (returns a blockingSyncEnvClient).Covers all
examples/scripts/openenv/*.py, the 3 OpenEnv notebooks, anddocs/source/openenv.md.Fixes #4952.
We should be able to leverage very easily the asynchronous mode be now the matter is just to fix the integration.
Note
Low Risk
Documentation and example-only changes; no TRL trainer or library runtime logic modified.
Overview
Restores TRL OpenEnv examples after OpenEnv made
reset()andstep()async-first (calling them without awaiting returned coroutines and broke training with errors like missing.observation).Client construction now appends
.sync()everywhere synchronousenvironment_factory/rollout_funccode talks to the server—Echo, TextArena (Wordle/Sudoku), BrowserGym, OpenSpiel (Catch), and CARLA, including reconnect paths incarla_vlm_gemma.py.Docs (
openenv.md) add a note on async vs sync, link the OpenEnv async/sync guide, and update all connection snippets to use.sync().Notebooks (BrowserGym FunctionGemma, Wordle, Sudoku) match the same pattern with short inline comments.
Reviewed by Cursor Bugbot for commit a5d9fc9. Bugbot is set up for automated code reviews on this repo. Configure here.