feat(cli): add openenv serve for local uvicorn#668
Conversation
- Load app/host/port from openenv.yaml and run uvicorn with env cwd on sys.path - Prepend repo src when the env path sits under an OpenEnv clone - Register serve on the Typer CLI; document usage in README - Add echo_env models.py stub required by validate_env_structure - Add tests for serve (mocked uvicorn + echo /health subprocess) - Stabilize usort/ruff import blocks in grid_world and julia env tests
|
Hi @false200! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Greptile SummaryThis PR implements
Confidence Score: 3/5Not safe to merge without fixing the integration test deadlock and the in-process global-state mutations that corrupt test isolation. The serve command calls os.chdir and sys.path.insert before handing off to uvicorn; since CliRunner runs in-process, these mutations persist across every subsequent unit test in the suite. Separately, the integration test calls proc.stdout.read() on a live subprocess pipe before proc.terminate(), hanging the suite indefinitely whenever the server fails to start within the deadline. src/openenv/cli/commands/serve.py (global-state mutations) and tests/test_cli/test_serve.py (missing CWD/sys.path teardown in unit tests, deadlock in integration test). Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI as openenv CLI
participant Serve as serve()
participant FS as Filesystem
participant UV as uvicorn
User->>CLI: openenv serve env_path [--port] [--host]
CLI->>Serve: invoke serve()
Serve->>FS: validate_env_structure(env_path)
FS-->>Serve: OK / FileNotFoundError
Serve->>FS: open openenv.yaml - yaml.safe_load()
FS-->>Serve: manifest dict
Serve->>Serve: validate app, runtime fields
Serve->>Serve: _find_repo_src_for_openenv() - sys.path.insert()
Serve->>Serve: sys.path.insert(env_root)
Serve->>Serve: os.chdir(env_root)
Serve->>UV: uvicorn.run(app_spec, host, port, reload)
UV-->>User: HTTP server listening
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if not ok: | ||
| out = proc.stdout.read() if proc.stdout else "" | ||
| pytest.fail(f"/health never OK (last error={last_exc!r}): {out}") |
There was a problem hiding this comment.
proc.stdout.read() deadlocks when the server never becomes healthy
When the 60-second deadline expires with ok = False, line 125 calls proc.stdout.read() on a pipe connected to a still-running process. Because nothing has terminated the subprocess yet (proc.terminate() is in the finally block, which only executes after control leaves the try body), the read blocks indefinitely — pytest.fail is never reached and neither is the finally cleanup. Terminate the process before reading its output.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_cli/test_serve.py
Line: 124-126
Comment:
**`proc.stdout.read()` deadlocks when the server never becomes healthy**
When the 60-second deadline expires with `ok = False`, line 125 calls `proc.stdout.read()` on a pipe connected to a still-running process. Because nothing has terminated the subprocess yet (`proc.terminate()` is in the `finally` block, which only executes *after* control leaves the `try` body), the read blocks indefinitely — `pytest.fail` is never reached and neither is the `finally` cleanup. Terminate the process before reading its output.
How can I resolve this? If you propose a fix, please make it concise.| repo_src = _find_repo_src_for_openenv(env_path_obj) | ||
| if repo_src is not None: | ||
| repo_src_str = str(repo_src.resolve()) | ||
| if repo_src_str not in sys.path: | ||
| sys.path.insert(0, repo_src_str) | ||
|
|
||
| env_root = str(env_path_obj.resolve()) | ||
| if env_root not in sys.path: | ||
| sys.path.insert(0, env_root) | ||
|
|
||
| os.chdir(env_root) |
There was a problem hiding this comment.
sys.path.insert and os.chdir are irreversible global side effects. Because typer.testing.CliRunner runs the command in the same process (not a subprocess), every unit test that calls runner.invoke with serve will permanently move the test runner's CWD to env_root and prepend entries to sys.path. Tests that depend on the original CWD or a clean path (including subsequent tests in this very module) can silently break or import the wrong module version. The cleanest fix is to exec a child process so all global-state mutations are isolated, or add an autouse fixture that saves and restores os.getcwd() and sys.path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/openenv/cli/commands/serve.py
Line: 107-117
Comment:
**Global process-state mutation breaks in-process test isolation**
`sys.path.insert` and `os.chdir` are irreversible global side effects. Because `typer.testing.CliRunner` runs the command in the *same* process (not a subprocess), every unit test that calls `runner.invoke` with `serve` will permanently move the test runner's CWD to `env_root` and prepend entries to `sys.path`. Tests that depend on the original CWD or a clean path (including subsequent tests in this very module) can silently break or import the wrong module version. The cleanest fix is to exec a child process so all global-state mutations are isolated, or add an autouse fixture that saves and restores `os.getcwd()` and `sys.path`.
How can I resolve this? If you propose a fix, please make it concise.- Autouse fixture restores cwd and sys.path after in-process CliRunner invokes - Terminate subprocess before reading stdout when /health deadline exceeded (Greptile P1)
meta-cla
Bot
added
the
CLA Signed
|
Issue 3: host vs Docker Hi @Darktex, Happy to add a one line note in |
1 participant
Related to meta-pytorch/OpenEnv#613
Adds an
openenv servecommand that readsopenenv.yaml(app,port,runtime), validates the env layout, sets cwd andsys.pathlike a normal env run, optionally prepends the OpenEnv reposrc/when the env lives under a clone, and starts the app with uvicorn. Documentsopenenv serve .in the README. Adds a minimalenvs/echo_env/models.pysovalidate_env_structurepasses. Includes CLI tests (mocked uvicorn + optional subprocess/healthon echo). Includes small import-order fixes intest_grid_world.pyandtest_julia_env.pysousort/ruff formatstay stable.Summary
Introduces
openenv serveas the supported way to run a FastAPI OpenEnv app locally from an environment directory, aligned with manifest-driven deployment and existing validation.Type of Change
Alignment Checklist
Before submitting, verify:
.claude/docs/PRINCIPLES.mdand this PR aligns with our principles.claude/docs/INVARIANTS.mdand no invariants are violated/pre-submit-pr(orbash .claude/hooks/lint.shand tests) and addressed all issues(Dev-only CLI; does not change WebSocket/MCP contracts or agent-facing APIs.)
RFC Status
Test Plan
Lint (matches CI):
uv run usort format src/ tests/ && uv run ruff format src/ tests/→ no diff;uv run usort check src/ tests/ && uv run ruff check src/ tests/Targeted tests:
PYTHONPATH=src:envs uv run pytest tests/test_cli/test_serve.py -vManual: from
envs/echo_env(or any valid env withruntime: fastapi):openenv serve . --host 127.0.0.1 --port <port>→GET /healthreturns 200.Claude Code Review
N/A