-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: split eval-env into separate metaworld and procthor environments #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ments - Create environments/ directory with per-environment packages - Add environments/metaworld/ with MuJoCo-based manipulation tasks - Add environments/procthor/ with AI2-THOR procedural house tasks - Add new 'kinitro build-env <family>' CLI command - Remove deprecated monolithic eval-env/ directory - Remove deprecated 'kinitro build-eval-env' command - Add scipy to procthor requirements (needed for house generation) Benefits: - Smaller Docker images (metaworld ~1GB vs ~3GB monolithic) - Better platform compatibility (metaworld works on any platform) - Faster builds (only build what you need) - Cleaner separation of dependencies
📝 WalkthroughWalkthroughAdds MetaWorld and ProcTHOR evaluation environments (Dockerfiles, requirements, env Actors), introduces family-oriented build command and registry metadata, updates CLI and docs for per-family images, and removes eval-env .gitignore entries. No behavioral changes to existing non-environment code. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@environments/metaworld/env.py`:
- Around line 93-95: list_environments currently returns
get_all_environment_ids() which includes non-MetaWorld environments; update
environments/metaworld/env.py so MetaWorld.list_environments filters that full
list to only MetaWorld IDs (e.g., by matching the MetaWorld naming/prefix or
consulting a MetaWorld registry) before returning. Locate the list_environments
method and replace the direct return of get_all_environment_ids() with a
filtered comprehension or helper that selects only environment IDs belonging to
MetaWorld, preserving the async signature and docstring.
In `@environments/metaworld/requirements.txt`:
- Around line 10-11: The metaworld git dependency is currently tracking the
repository default branch which risks non-reproducible builds; update the
dependency line "metaworld @
git+https://github.com/Farama-Foundation/Metaworld.git" to pin it to the stable
v3.0.0 release (e.g. add `@v3.0.0` to the git URL or use an exact version
specifier) so installs are reproducible, then re-lock/reinstall dependencies to
verify.
🧹 Nitpick comments (8)
environments/procthor/requirements.txt (1)
9-11: Consider pinning the procthor git dependency to a specific commit for reproducible builds.Similar to the metaworld requirements, installing procthor from the default branch can lead to non-reproducible builds. The comment mentions a Dec 2022 fix, so pinning to that commit would ensure consistency:
# Install procthor from git to get the latest house specification fix (Dec 2022) # The PyPI version 0.0.1.dev2 has incompatible material format with newer AI2-THOR builds -procthor @ git+https://github.com/allenai/procthor.git +procthor @ git+https://github.com/allenai/procthor.git@<commit-sha-from-dec-2022>environments/README.md (1)
7-18: Add a language identifier to the fenced code block.Per static analysis (markdownlint MD040), fenced code blocks should have a language specified:
-``` +```text environments/ ├── metaworld/ # MuJoCo-based manipulation tasksenvironments/procthor/env.py (1)
143-147: Consider documenting the**kwargsparameter for forward compatibility.The unused
kwargsparameter (flagged by Ruff ARG002) appears intentional for API extensibility. A brief docstring note would clarify this:timeout: int = 300, **kwargs, ) -> dict: """ Evaluate a miner's policy on a ProcTHOR task. + + Note: **kwargs is accepted for forward compatibility with future parameters.environments/metaworld/env.py (4)
97-109: Use explicit| Nonetype annotations for optional parameters.Ruff flags implicit
Optionaltypes (RUF013). PEP 484 recommends explicitT | Nonesyntax for clarity.✨ Proposed fix for type hints
async def evaluate( self, task_id: int, - seed: int = None, - model: str = None, - base_url: str = None, + seed: int | None = None, + model: str | None = None, + base_url: str | None = None, env_id: str = "metaworld/pick-place-v3", max_timesteps: int = 500, action_timeout: float = 0.5, use_images: bool = True, timeout: int = 300, **kwargs, ) -> dict:
284-291: Prefix unusedinfovariable with underscore.The
infovariable fromenv.step()is unpacked but never used. Prefix it with an underscore to indicate it's intentionally unused.✨ Proposed fix
- obs, reward, done, info = env.step(action) + obs, reward, done, _info = env.step(action)
315-323: Use explicit| Noneforextraparameter type hint.Same Ruff RUF013 issue: the
extraparameter defaults toNonebut lacks an explicit| Noneannotation.✨ Proposed fix
def _build_error_result( self, env_id: str, task_id: int, seed: int, start_time: float, error: str, - extra: dict = None, + extra: dict | None = None, ) -> dict:
341-351: Consider logging exceptions during cleanup instead of silently swallowing them.The
try-except-passpattern can hide issues. While cleanup shouldn't propagate exceptions, logging at debug/warning level aids troubleshooting.🔧 Proposed fix to log cleanup errors
async def cleanup(self): """Cleanup resources.""" # HTTP clients are now created per-request, no need to close here # Close environments for env in self._env_cache.values(): try: env.close() - except Exception: - pass + except Exception as e: + logger.debug("Error closing environment", error=str(e)) self._env_cache.clear()environments/metaworld/Dockerfile (1)
27-48: Consider whethergitis needed at runtime.The
gitpackage is installed along with rendering dependencies. If it's only needed duringpip installfor VCS-based dependencies, you could potentially remove it after pip install to reduce image size. However, if metaworld or other packages need it at runtime, this is fine.If git is only a build-time dependency:
✨ Optional: Remove git after pip install
RUN apt-get update && apt-get install -y --no-install-recommends \ git \ # OpenGL/Mesa libraries for MuJoCo rendering ... && rm -rf /var/lib/apt/lists/* # Copy requirements and install dependencies COPY requirements.txt /app/ -RUN pip install --no-cache-dir -r /app/requirements.txt +RUN pip install --no-cache-dir -r /app/requirements.txt \ + && apt-get purge -y git \ + && apt-get autoremove -y
- MetaWorld actor now only lists metaworld/* environments - ProcTHOR actor now only lists procthor/* environments - Pin metaworld dependency to v3.0.0 for reproducible builds Addresses review feedback from CodeRabbit
- Add FAMILY_METADATA to registry with display names and descriptions - Add get_family_metadata() function to registry - Refactor list-envs CLI to use registry functions instead of hardcoding - Derives family info from single source of truth in registry
90e644b to
48fe38b
Compare
- Update README.md with new build-env command and ProcTHOR environments - Update backend-guide.md with per-family image configuration - Update environments/README.md with accurate image sizes - Replace deprecated build-eval-env references with build-env
Summary
eval-env/into separate per-environment packages underenvironments/kinitro build-env <family>CLI command for building individual environment imageseval-env/directory andbuild-eval-envCLI commandChanges
New Structure
New CLI Command
Benefits
Testing
Summary by CodeRabbit
New Features
Updates
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.