-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Registers direct environments to Gymnasium as string-style import #3803
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
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.
Greptile Overview
Greptile Summary
This PR standardizes Gymnasium environment registration across the IsaacLab codebase by converting all direct imports to string-based lazy imports. The changes affect environment registration files in direct/ workflows (factory, forge) and manager_based/ manipulation tasks (lift, pick_place, stack). Instead of importing environment classes and configuration objects directly at module load time, the code now uses f-string interpolations with __name__ (e.g., f"{__name__}.factory_env:FactoryEnv") as entry points. This pattern defers actual module loading until gym.make() is called, preventing unnecessary dependency loading and improving import performance. The change follows Gymnasium's standard registration pattern and aligns with the existing architecture where environment configurations are resolved at instantiation time rather than registration time.
PR Description Notes:
- Checkbox formatting inconsistency: Line with "I have made corresponding changes to the documentation" has
[]instead of[ ]
Potential Issues
-
Critical: Incorrect CHANGELOG date - The changelog entry for version 0.11.2 is dated
2025-10-22, which is in the future. This should be corrected to the actual release date (likely2024-10-22based on surrounding entries). -
Robomimic configuration path format change - In
stack/config/franka/__init__.pyand other files, therobomimic_bc_cfg_entry_pointchanged from filesystem paths usingos.path.join(agents.__path__[0], "robomimic/bc.json")to module-style pathsf"{agents.__name__}:robomimic/bc.json". This assumes the framework can resolve JSON file paths using the module:resource syntax with colon separators. If the robomimic integration expects actual filesystem paths rather than module references, this could cause runtime failures when trying to load BC policies. This should be verified with tests. -
Missing verification for string-based resource loading - The PR changes resource references (especially for JSON configuration files) from filesystem paths to string-based module references. While this works for Python classes via import mechanisms, JSON files require special handling (likely through
importlib.resourcesor similar). The codebase should have mechanisms to resolve these string references to actual file paths at runtime.
Confidence Score
3/5 - The code changes are mechanically correct and follow a clear pattern, but there are concerns about the robomimic configuration path format change and the incorrect changelog date. The lack of tests mentioned in the checklist is also concerning for validating that string-based resource loading works correctly for non-Python files (JSON configs). The pattern is sound for Python class imports, but the JSON file path changes need verification.
7 files reviewed, 1 comment
source/isaaclab_tasks/isaaclab_tasks/manager_based/manipulation/stack/config/franka/__init__.py
Show resolved
Hide resolved
|
@ooctipus sorry for the delayed review on this. Now that Isaac Lab 2.3 has been merged to main, it would be good to rebase and make the modifications in the new environments that were introduced. Unfortunately, I realized that we won't be able to get rid of --enable-pinocchio it is needed in the mimic scripts to load pinocchio before Isaac Sim is loaded so the pinocchio in Isaac Lab is used instead of Isaac Sim. |
Description
This PR ensures all imports follows the string import style.
String import style avoid pulling of unnecessary packages that is related to other environments, this pr makes sure all environments are using this import.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there