Skip to content

docs: fix Environment Builder client example to use .sync() bootstrap handle - #1129

Open
reny1cao wants to merge 2 commits into
huggingface:mainfrom
reny1cao:fix/1118-docker-client-context-manager
Open

docs: fix Environment Builder client example to use .sync() bootstrap handle#1129
reny1cao wants to merge 2 commits into
huggingface:mainfrom
reny1cao:fix/1118-docker-client-context-manager

Conversation

@reny1cao

@reny1cao reny1cao commented Sep 9, 2026

Copy link
Copy Markdown

Summary

The "Use Your Environment" example in the Environment Builder guide uses MyEnv.from_docker_image(...) directly in a with block, which fails with TypeError: '_BootstrapResult' object does not support the context manager protocol (or 'coroutine' object ... on 0.4.1). from_docker_image() / from_env() return a lazy bootstrap handle that must be awaited or chained with .sync() (#343, #959); the example predates that change and also references a MyEnv.from_hub() that does not exist on EnvClient. This PR fixes the docs (including the Quick Reference Card row and the skill reference copy of the page) and adds tests that exercise the documented bootstrap/context-manager patterns.

Fixes #1118

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

RFC Status

  • Not required (bug fix, docs, minor refactoring)
  • RFC exists: #___
  • RFC needed (will create before merge)

Changes

  • docs/source/getting_started/environment-builder.md: use .sync() on all constructor lines and the manual-cleanup block, replace from_hub() with from_env(...).sync() (both in the example and in the Quick Reference Card row), explain the lazy handle, and add the async equivalent with a link to the Async vs Sync guide.
  • .claude/skills/generate-openenv-env/references/openenv-docs-environment-builder.md: same edit applied to the skill's reference copy of this page. generate-openenv-env/SKILL.md instructs the agent to always read this file, so leaving it stale would keep generating the old API.
  • docs/source/guides/task-api.md: same one-token .sync() fix in the with LatexOCREnv.from_docker_image(...) example.
  • tests/test_core/test_generic_client.py: two tests in TestSyncBootstrapConstructors that exercise the documented sync and async bootstrap/context-manager patterns (from_docker_image(...).sync() + with, and await from_docker_image(...) + async with, each followed by reset()) through the real dispatch path with a fake websocket and mock provider, asserting a concrete StepResult, a single socket, and container release on exit.

No runtime code is changed.

Test Plan

  • Reproduced every variant against registry.hf.space/openenv-echo-env:latest with real Docker at ced9d73c. The corrected sync and async snippets both complete reset/step/state end to end.
  • PYTHONPATH=src:envs uv run pytest tests/test_core/test_generic_client.py -k documented -v: 2 passed.
  • bash .claude/hooks/test.sh: 1519 passed, 95 skipped.
  • ruff check, ruff format --check, usort check clean on touched files; python scripts/sync_env_docs.py --check clean.
  • Reviewers can verify with the same pytest command above, or by running the updated docs snippet against any built env image.

Out of scope (noted for follow-up)

  • Several env READMEs use env = X.from_docker_image(...) then env.reset() without .sync(), which raises AttributeError.
  • LocalDockerProvider.stop_container lets subprocess.TimeoutExpired escape close() when docker stop exceeds 10s (images using sh -c as PID 1 always hit the SIGTERM grace period).
  • Whether _BootstrapResult should support with / async with directly is an API decision left to maintainers.

Claude Code Review

/alignment-review was not run. Validation was done with bash .claude/hooks/test.sh plus the lint commands from CLAUDE.md; results are in the Test Plan above.


Note

Low Risk
Documentation and test-only changes; no production client or server behavior is modified.

Overview
Fixes #1118 by aligning Environment Builder and related docs with the lazy bootstrap API: from_docker_image() and from_env() must be resolved with .sync() (sync) or await (async) before using with / async with.

The “Use Your Environment” sections in environment-builder.md and the skill reference copy now chain .sync() on all client construction paths, swap the invalid MyEnv.from_hub(...) call for MyEnv.from_env(...).sync() (including the Quick Reference Card), and add prose plus an async example explaining why using the raw handle in a context manager raises TypeError. The Task API guide’s LatexOCREnv.from_docker_image(...) training loop gets the same .sync() fix.

Two regression tests in test_generic_client.py assert the documented sync (.sync() + with + reset()) and async (await + async with + reset()) paths return a concrete StepResult, use a single WebSocket, and stop the container on exit. No library/runtime code changes.

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

… handle (huggingface#1118)

`from_docker_image()` / `from_env()` return a lazy bootstrap handle
(`_BootstrapResult`) that must be awaited or chained with `.sync()`;
it is not a context manager. The "Use Your Environment" example in the
Environment Builder guide predates the async client migration and used
the handle directly in a `with` block, and also referenced a
`MyEnv.from_hub()` method that does not exist on `EnvClient`.

- Use `.sync()` on all constructor lines and in the manual-cleanup block
- Replace `from_hub()` with `from_env(...).sync()`, including the
  Quick Reference Card row at the top of the page
- Explain the lazy handle and show the async equivalent
- Apply the same `.sync()` fix to the Task API guide example
- Sync the generate-openenv-env skill's reference copy of this page so
  the environment generator stops reproducing the stale API
- Add unit tests that exercise the documented sync and async
  bootstrap/context-manager patterns through the real dispatch path

Fixes huggingface#1118
@reny1cao
reny1cao force-pushed the fix/1118-docker-client-context-manager branch from 1dd9f40 to a25e9f5 Compare September 9, 2026 06:37
@cursor
cursor Bot requested a review from burtenshaw September 9, 2026 13:04

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

Reviewed exact head 95cd5d349f9816986c816495a4b26a3cd842083c against current main f8433a56f2fde5a6f20765b491b8365fc123f7fa. The documented .sync() bootstrap sequence matches _BootstrapResult/SyncEnvClient behavior, and the merge-tree is conflict-free. No runtime, security, API, or alignment defect was found.

Before merge, refresh the branch and have a maintainer approve the external-branch workflows: Tests, Package CI, and docs CI never ran (action_required) at this head. Two small coverage/docs follow-ups are also worthwhile during refresh: assert the synchronous usage path opens exactly one socket (as the async companion test does), and keep the generated-agent skill copy aligned with the public guide's Async-vs-Sync cross-reference. These are not grounds for a changes-requested review, but missing exact-head CI is a hard merge gate. burtenshaw is already requested.

View PR

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.

[docs] getting_started/environment-builder#use-your-environment issues

1 participant