Add generic Gymnasium environment - #168
Conversation
meta-cla
Bot
added
the
CLA Signed
|
✅ Deployment succeeded for
Nice work! Wait for a code review and we're ready to go. You can iterate locally or validate fixes by running |
1 similar comment
|
✅ Deployment succeeded for
Nice work! Wait for a code review and we're ready to go. You can iterate locally or validate fixes by running |
| @@ -0,0 +1,326 @@ | |||
| """Tests for the generic Gymnasium environment integration.""" | |||
There was a problem hiding this comment.
I would suggest skip generic Prefix. We can say OpenAI Gymnasium, or just Gymnasium is also cool probably..
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code (alignment-reviewer agent), not a human review. The account posting this is shared with the human maintainer.
Perfect! Now I have all the context I need. Let me compile my review.
PR #168 Review: Add generic Gymnasium environment
Previous Review Summary
Review by @shaginhekvs: Details not provided in the PR context, but review was marked as COMMENTED (not APPROVED).
Automated checks: Deployment to HuggingFace succeeded for the gym_env environment.
TIER 1: Bugs and Critical Issues
🔴 CRITICAL: Using deprecated HTTPEnvClient instead of WebSocket-based EnvClient
Location: src/envs/gym_env/client.py:14
from core.http_env_client import HTTPEnvClient
class GymEnvironment(HTTPEnvClient[GymAction, GymObservation]):Issue: The codebase has migrated to WebSocket-based communication (commit 85c47c6). According to INVARIANTS.md:74, HTTP is being deprecated in favor of WebSocket-only:
Note: We are in the process of deprecating HTTP (see PR #252) in favor of WebSocket-only, but we are still transitioning and both protocols are currently available.
Impact:
- This environment uses a communication pattern that is being phased out
- Inconsistent with reference implementation (
echo_envusesEnvClient) - Will need to be rewritten when HTTP support is removed
Fix Required: Change to use EnvClient from core.env_client with WebSocket support, following the pattern in envs/echo_env/client.py:31.
🔴 BUG: Import path violations
Location: Multiple files
Issue 1: src/envs/gym_env/client.py:12-14
from core.client_types import StepResult
from core.http_env_client import HTTPEnvClient
from .models import GymAction, GymObservation, GymStateIssue 2: src/envs/gym_env/server/app.py:10
from core.env_server import create_appProblem: Import paths are incorrect. Should use openenv.core instead of core for in-repo imports, or support both in-repo and standalone patterns like echo_env does.
Reference: envs/echo_env/client.py:16-28 shows the correct dual-import pattern:
try:
# In-repo imports (when running from OpenEnv repository)
from openenv.core.client_types import StepResult
...
except ImportError:
# Standalone imports (when environment is standalone with openenv from pip)
from openenv.core.client_types import StepResult
...Fix Required: Add try/except import pattern for both in-repo and standalone usage.
🟡 Lint failures (cannot verify without uv installed)
The lint check hook failed because uv is not installed:
Error: 'uv' is not installed or not in PATH
Action Required: Run uv run ruff format src/envs/gym_env/ tests/envs/test_gym_environment.py --check locally to verify formatting compliance.
🟢 Debug code check: PASSED
No problematic debug code found in the new files.
TIER 2: Alignment with Principles and Invariants
⚠️ ALIGNMENT CONCERN: Deviation from canonical environment structure
Reference: PATTERNS.md:5-20 defines canonical structure
Issue: The environment is placed in src/envs/gym_env/ instead of envs/gym_env/.
Observed structure:
src/envs/gym_env/
├── __init__.py
├── client.py
├── models.py
├── README.md
└── server/
├── __init__.py
├── app.py
├── gymnasium_environment.py
├── requirements.txt
├── Dockerfile
└── additional_env_parameters.yaml
Expected structure (per PATTERNS.md and reference implementation):
envs/gym_env/
├── __init__.py
├── client.py
├── models.py
├── openenv.yaml # Missing!
├── pyproject.toml # Missing!
└── server/
Missing files:
openenv.yaml- Environment manifestpyproject.toml- Dependencies for standalone usage
Question for maintainer: Is the src/envs/ location intentional, or should this follow the standard envs/ pattern used by echo_env?
⚠️ Type safety patterns not fully followed
Issue: The GymState class extends the base State but adds additional fields:
@dataclass
class GymState(State):
env_id: str = "Unknown"
render_mode: Optional[str] = None
max_steps: Optional[int] = None
seed: Optional[int] = None
episode_length: int = 0
total_reward: float = 0.0Observation: The reference echo_env uses the base State class directly without extension:
# echo_env/client.py:98-111
def _parse_state(self, payload: Dict) -> State:
return State(
episode_id=payload.get("episode_id"),
step_count=payload.get("step_count", 0),
)Analysis: This is not necessarily wrong - adding domain-specific state is reasonable for Gym environments. However, it's inconsistent with the simpler reference pattern.
Suggestion: Consider whether all the additional state fields are necessary, or if some could be moved to metadata in the observation.
📋 MINOR: Dockerfile comments reference wrong environment
Location: src/envs/gym_env/server/Dockerfile:1-2
# Dockerfile for Atari Environment
# This image provides Atari 2600 games via the Arcade Learning Environment (ALE)Issue: Copy-paste error - comments mention "Atari" but this is the generic Gym environment.
Fix: Update comments to reference the generic Gymnasium environment.
📋 MINOR: Inconsistent YAML file usage
Location: src/envs/gym_env/server/additional_env_parameters.yaml
Issue: File exists but is empty. The app.py loads it if present:
yaml_param_file_path = os.getenv("ADDITIONAL_PARAMETERS_YAML_FILE")
additional_params = {}
if yaml_param_file_path and os.path.exists(yaml_param_file_path):
with open(yaml_param_file_path, "r") as f:
additional_params = yaml.safe_load(f)Suggestion: Either:
- Add example content/documentation to the YAML file
- Remove it if not needed
- Add a comment explaining its purpose
✅ POSITIVE: Excellent serialization handling
The _to_serializable() method in gymnasium_environment.py:346-368 handles numpy types, nested structures, and edge cases comprehensively. This is well-designed for dealing with diverse Gymnasium observation/action spaces.
✅ POSITIVE: Comprehensive action space support
The environment handles multiple Gymnasium action space types (Discrete, MultiDiscrete, MultiBinary, Box, Tuple, Dict, Text) with proper conversion logic. This makes it truly generic.
✅ POSITIVE: Good test coverage
The test file includes tests for:
- Discrete action spaces (CartPole, Taxi)
- Continuous action spaces (BipedalWalker, Pendulum, MountainCar, LunarLander)
- Client parser validation
- Both Box and Discrete action space metadata
Summary
Critical Issues (Must Fix)
- Replace HTTPEnvClient with WebSocket-based EnvClient - The codebase is migrating away from HTTP
- Fix import paths - Use
openenv.coreand add dual-import pattern for standalone support - Run lint checks - Verify
ruff formatcompliance
Alignment Concerns (Review Needed)
- Directory structure - Clarify if
src/envs/vsenvs/is intentional - Missing manifest files - Add
openenv.yamlandpyproject.tomlper canonical pattern - Dockerfile comments - Fix copy-paste error from Atari environment
Recommendations
- Follow the
echo_envreference implementation more closely for consistency - Consider simplifying
GymStateor documenting why extensions are needed - Document or remove the empty
additional_env_parameters.yamlfile
Overall Assessment: This is a valuable addition that brings generic Gymnasium support to OpenEnv. However, it uses deprecated patterns (HTTP client) and deviates from the canonical structure. The core implementation is solid, but architectural alignment is needed before merge.
Recommended Action: Request changes to address Tier 1 issues and clarify Tier 2 alignment concerns with maintainers.
Automated review by Claude Code | Learn more about OpenEnv's agentic workflow
Dismissing automated approval due to bug in review bot. The original review either had blank content or approved despite finding blocking issues. Please disregard this approval.
Greptile SummaryAdds generic Gymnasium (OpenAI Gym) environment integration to OpenEnv, enabling support for numerous RL environments through a client-server architecture. Key Changes:
Issues Found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as GymEnvironment<br/>(Client)
participant HTTP as HTTP/FastAPI
participant Server as app.py
participant Wrapper as GymnasiumEnvironment
participant Gym as Gymnasium Env
Note over Client,Gym: Initialization
Server->>Wrapper: Create GymnasiumEnvironment(env_id, render_mode, max_steps, seed)
Wrapper->>Gym: gym.make(env_id, render_mode)
Gym-->>Wrapper: env instance
Wrapper->>Wrapper: Initialize action/observation space metadata
Note over Client,Gym: Reset Episode
Client->>HTTP: POST /reset
HTTP->>Wrapper: reset()
Wrapper->>Wrapper: _consume_seed()
Wrapper->>Gym: env.reset(seed)
Gym-->>Wrapper: observation, info
Wrapper->>Wrapper: _make_observation()
Wrapper-->>HTTP: GymObservation
HTTP-->>Client: StepResult with observation
Note over Client,Gym: Execute Action
Client->>HTTP: POST /step {action}
HTTP->>Wrapper: step(GymAction)
Wrapper->>Wrapper: _convert_action(action)
Wrapper->>Gym: env.step(converted_action)
Gym-->>Wrapper: obs, reward, terminated, truncated, info
Wrapper->>Wrapper: Update state (step_count, total_reward)
Wrapper->>Wrapper: _make_observation()
Wrapper-->>HTTP: GymObservation
HTTP-->>Client: StepResult with reward, done
Note over Client,Gym: Query State
Client->>HTTP: GET /state
HTTP->>Wrapper: state property
Wrapper-->>HTTP: GymState
HTTP-->>Client: Current environment state
Note over Client,Gym: Cleanup
Client->>HTTP: POST /close
HTTP->>Wrapper: close()
Wrapper->>Gym: env.close()
Gym-->>Wrapper: closed
Wrapper-->>HTTP: success
HTTP-->>Client: success
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Dockerfile for Atari Environment | ||
| # This image provides Atari 2600 games via the Arcade Learning Environment (ALE) |
There was a problem hiding this comment.
style: Header comments still reference Atari instead of Gym/Gymnasium.
| # Dockerfile for Atari Environment | |
| # This image provides Atari 2600 games via the Arcade Learning Environment (ALE) | |
| # Dockerfile for Gym Environment | |
| # This image provides generic Gymnasium (OpenAI Gym) environment support |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/envs/gym_env/server/Dockerfile
Line: 1:2
Comment:
**style:** Header comments still reference Atari instead of Gym/Gymnasium.
```suggestion
# Dockerfile for Gym Environment
# This image provides generic Gymnasium (OpenAI Gym) environment support
```
How can I resolve this? If you propose a fix, please make it concise.| # Local build: docker build -t envtorch-base:latest -f src/core/containers/images/Dockerfile . | ||
| # docker build -f src/envs/atari_env/server/Dockerfile -t atari-env:latest . | ||
| # | ||
| # CI/CD build: docker build --build-arg BASE_IMAGE=ghcr.io/meta-pytorch/openenv-base:latest \ | ||
| # -f src/envs/atari_env/server/Dockerfile -t atari-env:latest . |
There was a problem hiding this comment.
style: Example commands in comments still reference atari_env instead of gym_env.
| # Local build: docker build -t envtorch-base:latest -f src/core/containers/images/Dockerfile . | |
| # docker build -f src/envs/atari_env/server/Dockerfile -t atari-env:latest . | |
| # | |
| # CI/CD build: docker build --build-arg BASE_IMAGE=ghcr.io/meta-pytorch/openenv-base:latest \ | |
| # -f src/envs/atari_env/server/Dockerfile -t atari-env:latest . | |
| # Local build: docker build -t openenv-base:latest -f src/core/containers/images/Dockerfile . | |
| # docker build -f src/envs/gym_env/server/Dockerfile -t gym-env:latest . | |
| # | |
| # CI/CD build: docker build --build-arg BASE_IMAGE=ghcr.io/meta-pytorch/openenv-base:latest \ | |
| # -f src/envs/gym_env/server/Dockerfile -t gym-env:latest . |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/envs/gym_env/server/Dockerfile
Line: 7:11
Comment:
**style:** Example commands in comments still reference `atari_env` instead of `gym_env`.
```suggestion
# Local build: docker build -t openenv-base:latest -f src/core/containers/images/Dockerfile .
# docker build -f src/envs/gym_env/server/Dockerfile -t gym-env:latest .
#
# CI/CD build: docker build --build-arg BASE_IMAGE=ghcr.io/meta-pytorch/openenv-base:latest \
# -f src/envs/gym_env/server/Dockerfile -t gym-env:latest .
```
How can I resolve this? If you propose a fix, please make it concise.| # --- Runtime environment with defaults --- | ||
| # These ENV lines set defaults but still allow runtime overrides | ||
| ENV GYM_ENVIRONMENT_ID=${GYM_ENVIRONMENT_ID} | ||
| ENV ADDITIONAL_PARAMETERS=${ADDITIONAL_PARAMETERS_YAML_FILE} |
There was a problem hiding this comment.
logic: ENV variable name mismatch - setting ADDITIONAL_PARAMETERS but the code expects ADDITIONAL_PARAMETERS_YAML_FILE.
| ENV ADDITIONAL_PARAMETERS=${ADDITIONAL_PARAMETERS_YAML_FILE} | |
| ENV ADDITIONAL_PARAMETERS_YAML_FILE=${ADDITIONAL_PARAMETERS_YAML_FILE} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/envs/gym_env/server/Dockerfile
Line: 41:41
Comment:
**logic:** ENV variable name mismatch - setting `ADDITIONAL_PARAMETERS` but the code expects `ADDITIONAL_PARAMETERS_YAML_FILE`.
```suggestion
ENV ADDITIONAL_PARAMETERS_YAML_FILE=${ADDITIONAL_PARAMETERS_YAML_FILE}
```
How can I resolve this? If you propose a fix, please make it concise.| if yaml_param_file_path and os.path.exists(yaml_param_file_path): | ||
| with open(yaml_param_file_path, "r") as f: | ||
| additional_params = yaml.safe_load(f) |
There was a problem hiding this comment.
logic: yaml.safe_load() returns None for empty YAML files, which will cause **additional_params to fail on line 40.
| if yaml_param_file_path and os.path.exists(yaml_param_file_path): | |
| with open(yaml_param_file_path, "r") as f: | |
| additional_params = yaml.safe_load(f) | |
| if yaml_param_file_path and os.path.exists(yaml_param_file_path): | |
| with open(yaml_param_file_path, "r") as f: | |
| loaded_params = yaml.safe_load(f) | |
| additional_params = loaded_params if loaded_params else {} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/envs/gym_env/server/app.py
Line: 30:32
Comment:
**logic:** `yaml.safe_load()` returns `None` for empty YAML files, which will cause `**additional_params` to fail on line 40.
```suggestion
if yaml_param_file_path and os.path.exists(yaml_param_file_path):
with open(yaml_param_file_path, "r") as f:
loaded_params = yaml.safe_load(f)
additional_params = loaded_params if loaded_params else {}
```
How can I resolve this? If you propose a fix, please make it concise.
|
Closing as a stale, currently unmergeable PR. Reopen if someone wants to revive it on top of current main. |
6 participants
This PR introduces a generic Gymnasium (OpenAI Gym/Gymnasium) environment server and client integration to OpenEnv.