Add maze environment and example - #106
Conversation
|
Hi @VivekSil! 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! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
meta-cla
Bot
added
the
CLA Signed
There was a problem hiding this comment.
Pull Request Overview
This PR integrates a Maze game environment with the OpenEnv framework, providing both server-side and client-side implementations with Docker support.
- Implements MazeEnvironment wrapper that exposes the Maze game through OpenEnv's Environment interface
- Adds HTTP client and server infrastructure for remote maze environment interaction
- Includes example scripts demonstrating both automated and human-interactive maze solving
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| src/envs/maze_env/server/mazearray.py | Defines the maze layout as a numpy array |
| src/envs/maze_env/server/maze_environment.py | Wraps Maze game to implement OpenEnv Environment interface |
| src/envs/maze_env/server/maze.py | Core Maze implementation with coordinate system (col, row) |
| src/envs/maze_env/server/app.py | FastAPI application exposing maze environment over HTTP |
| src/envs/maze_env/server/init.py | Package exports for server components |
| src/envs/maze_env/server/Dockerfile | Container configuration for maze environment server |
| src/envs/maze_env/models.py | Data models for actions, observations, and state |
| src/envs/maze_env/client.py | HTTP client for connecting to maze environment server |
| src/envs/maze_env/init.py | Package exports for client components |
| src/envs/maze_env/README.md | Documentation for maze environment usage |
| examples/maze_simple.py | Example demonstrating automated maze navigation |
| examples/maze_human.py | Example demonstrating human-interactive maze solving |
| .github/workflows/docker-build.yml | Adds maze-env to CI/CD Docker build workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
✅ 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 |
|
✅ 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 |
|
✅ 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 |
|
✅ 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 |
|
✅ 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 |
|
✅ 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 |
|
✅ 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 |
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.
I can see from the diff in the user message. Let me analyze the code from the diff provided and perform the review:
PR #106 Alignment Review: Add Maze Environment and Example
Previous Review Summary
The Copilot reviewer provided a general overview but didn't identify specific bugs or alignment issues. Key points noted:
- Integration of Maze environment with OpenEnv framework
- Server-side and client-side implementations with Docker support
- MCP client, HTTP server, and example scripts included
- 15 comments generated (not shown in detail)
Tier 1: Bugs & Critical Issues
🔴 CRITICAL: Missing Copyright Headers
File: src/envs/maze_env/server/mazearray.py:1
The file is missing the required Meta copyright header that all other files in the PR have:
# Copyright (c) Meta Platforms, Inc. and affiliates.
# All rights reserved.
#
# This source code is licensed under the BSD-style license found in the
# LICENSE file in the root directory of this source tree.🔴 BUG: Incorrect Observation Model - Missing done Field
Files:
src/envs/maze_env/models.py:26-30src/envs/maze_env/server/maze_environment.py:163
The MazeObservation model doesn't include a done field, but maze_environment.py:163 tries to set done=done when creating the observation:
# models.py - MazeObservation definition
@dataclass
class MazeObservation(Observation):
position: List[int] # [row, col]
total_reward: float
legal_actions: List[int] = field(default_factory=list)
# Missing: done field!# maze_environment.py:163 - tries to use done
return MazeObservation(
position=pos_list,
total_reward=self.total_reward,
legal_actions=legal_actions,
done=done, # ❌ Field doesn't exist in MazeObservation
)Fix: Add done: bool = False to MazeObservation in models.py.
🔴 BUG: Inconsistent Coordinate Systems
Files:
src/envs/maze_env/server/maze.py:58(documents col, row)src/envs/maze_env/server/maze_environment.py:44(uses row, col)src/envs/maze_env/models.py:28(documents row, col)
The original Maze class uses (col, row) coordinates but the wrapper uses (row, col). From maze.py:58:
# A note on cell coordinates:
# The cells in the maze are stored as (col, row) or (x, y) tuples.But MazeEnvironment and MazeObservation use (row, col):
# models.py:28
position: List[int] # [row, col]This mismatch will cause navigation bugs. The environment needs to consistently translate between the coordinate systems or adopt one consistently.
🟡 BUG: State Management Issues
File: src/envs/maze_env/server/maze_environment.py:168
The state() method can return None, but the type signature doesn't indicate this:
def state(self) -> Optional[MazeState]:
"""Return the current MazeState object."""
return self.state # Can be None before reset()But the base class Environment doesn't specify Optional[MazeState] for the return type. This could cause type errors.
🟡 LINT: Import Order Issues
File: src/envs/maze_env/server/mazearray.py:1
Missing blank line after imports (though this file needs full copyright header anyway).
🟡 POTENTIAL BUG: Hardcoded Maze in Examples
Files:
examples/maze_simple.py:38-47examples/maze_human.py:38-47
Both examples hardcode a maze array that should match the server's maze, but there's no guarantee they're in sync:
maze = np.array([
[0, 1, 0, 0, 0, 0, 0, 0],
# ... hardcoded maze
])This duplicates the maze definition from mazearray.py. If the server maze changes, the examples break. The examples should either:
- Fetch the maze from the server via an API endpoint, or
- Import from a shared location
🟡 CODE SMELL: Mutable Instance Variable
File: src/envs/maze_env/server/maze_environment.py:112-113
The _visited set is created on-demand and persists across steps:
if not hasattr(self, "_visited"):
self._visited = set()This should be initialized in __init__ or reset() for clarity and to avoid subtle bugs.
🟡 CORRECTNESS: Reward Logic Inconsistency
File: src/envs/maze_env/server/maze_environment.py:105-107
The reward settings define reward_move = 0.05 (positive reward) with a comment "reward for a move that didn't find the exit but is valid", but this contradicts the original Maze implementation which uses penalty_move = -0.05 (negative).
reward_move = 0.05 # reward for a move that didn't find the exit but is validvs original maze.py:69:
penalty_move = -0.05 # penalty for a move which did not result in finding the exit cellThis changes the fundamental reward structure - positive rewards for every step incentivizes wandering, while negative rewards incentivize finding the exit quickly. This seems like an unintentional bug.
Tier 2: Alignment & Architecture
🚩 ALIGNMENT FLAG: HTTP-Only Implementation
Files: Multiple
Invariant at risk: Communication patterns (INVARIANTS.md:69-74)
From INVARIANTS.md:
Communication patterns
- WebSocket for all environment communication (Gym-like API + metadata)
- Note: We are in the process of deprecating HTTP (see PR #252) in favor of WebSocket-only
This PR implements a pure HTTP environment when the project is transitioning to WebSocket-only. The client (client.py) extends HTTPEnvClient, not a WebSocket client.
Recommendation: This environment should use WebSocket communication, or at minimum, this deviation should be acknowledged and a plan to migrate should be documented.
Suggested reviewer: @Darktex (as this relates to the HTTP→WebSocket transition)
🚩 ALIGNMENT FLAG: Unclear Reward Ownership
File: src/envs/maze_env/server/maze_environment.py:step()
The step function computes rewards inside the environment (correct per INVARIANTS.md:64-67), but the reward logic is reimplemented rather than delegating to the original Maze.step(). This risks bugs (as seen with the reward sign flip above) and loses the original environment's careful tuning.
The original Maze class already has step() which returns (state, reward, status), but the wrapper reimplements all the movement and reward logic manually. This violates the DRY principle and creates maintenance burden.
Recommendation: Either use the original Maze.step() method or document why a full reimplementation was necessary.
🟠 PATTERN VIOLATION: Inconsistent Field Types
File: src/envs/maze_env/models.py
The models use @dataclass decorators, but other environments in the codebase use Pydantic BaseModel. From PATTERNS.md:77-79:
Pydantic Models
- All wire types must be Pydantic models
The maze environment uses:
@dataclass
class MazeAction(Action):
action: intInstead of:
class MazeAction(BaseModel):
action: intThis is inconsistent with the documented pattern and may cause serialization issues.
🟠 DOCUMENTATION: Missing OpenEnv Manifest
Expected file: src/envs/maze_env/openenv.yaml
Per PATTERNS.md:13, every environment should have an openenv.yaml manifest. This file is missing from the PR.
🟠 DOCUMENTATION: Missing pyproject.toml
Expected file: src/envs/maze_env/pyproject.toml
Per PATTERNS.md:14, every environment should have a pyproject.toml for dependencies. This is missing.
🟠 TESTING: No Test Coverage
Missing: tests/envs/test_maze_environment.py
Following the pattern from other environments (e.g., tests/envs/test_echo_environment.py), there should be test coverage for the maze environment. The PR includes examples but no pytest tests.
ℹ️ MINOR: Verbose Debugging in Examples
Files: examples/maze_simple.py, examples/maze_human.py
Both examples have extensive print statements and ASCII rendering. While acceptable for examples, consider noting in the README that these are demonstration scripts with verbose output.
ℹ️ MINOR: Duplicate Code in Examples
Files: examples/maze_simple.py:38-47, examples/maze_human.py:38-47
The two example files share 95% of their code. Consider refactoring shared logic into a helper function or noting in comments why they're separate.
Summary
Tier 1 Issues (Must Fix Before Merge):
- ❌ Missing copyright header in
mazearray.py - ❌
MazeObservationmissingdonefield - ❌ Coordinate system mismatch (col,row vs row,col)
⚠️ Reward sign flipped (positive instead of negative)⚠️ Hardcoded maze duplication in examples
Tier 2 Issues (Requires Discussion):
- 🚩 HTTP implementation when project is moving to WebSocket
- 🚩 Manual reward reimplementation instead of using original Maze logic
- 🟠 Using dataclasses instead of Pydantic models
- 🟠 Missing
openenv.yamlandpyproject.toml - 🟠 No test coverage
Automated Check Results:
- ❌ Lint check failed (uv not installed, manual review performed)
⚠️ Debug code check: Found print statements in existing test files (not from this PR)
Recommendation: Request changes for Tier 1 bugs. Tier 2 alignment flags should be discussed with maintainers before merge.
Automated review by Claude Code | Learn more about OpenEnv's agentic workflow
|
@Darktex I think we should close this PR due to lack of development. |
|
Thanks for the nudge @burtenshaw and my sincere apologies since I could not complete the implementation due to bandwidth issues. I'll close this PR for now and I'll create a new one once I have the required changes ready. |
|
No worries @VivekSil . We're looking forward to it. |
This PR will add Maze environment (#105)
Specifications:
Reward setting: