fix(rlinf): apply presets before IsaacLab env creation#5765
fix(rlinf): apply presets before IsaacLab env creation#5765johnnynunez wants to merge 2 commits into
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #5765
Title: fix(rlinf): apply presets before IsaacLab env creation
Summary
This PR implements preset/override application before gym.make() for IsaacLab environments, enabling GR00T/RLinf policies to work with SO-101 tasks. It also adds dual-version support for GR00T 1.6 and 1.7 embodiment modules.
Findings
🟡 Warning: Broad Exception Handling May Mask Real Errors
Location: Lines 54-56, 287-293, 584-589, 651-654
Multiple except Exception blocks catch all exceptions and continue silently. While pragmatic for optional features, this could mask genuine bugs (e.g., syntax errors in imported modules, permission issues).
Recommendation: Consider catching more specific exceptions (ImportError, ModuleNotFoundError) where the intent is to handle missing optional dependencies.
🟡 Warning: Directory Walk Logic is Fragile
Location: Lines 607-650
The code walks up 8 parent directories looking for a specific file path pattern. This couples the extension to a specific workshop directory structure and could silently fail on differently organized installations.
Recommendation: Consider making ISAACLAB_GROUND_PLANE_USD the primary mechanism with clear documentation. The heuristic search could be a fallback with an explicit warning when it fails.
🔵 Suggestion: Add Unit Tests for Action Key Resolution
Location: Lines 408-434
The new gr00t_action_keys handling with explicit ordering and action. prefix fallback logic is complex but has no test coverage. Consider adding unit tests for:
- Explicit key ordering with all keys present
- Fallback from
action.single_armtosingle_arm - Missing key error case
🔵 Suggestion: Validate Preset Type Before String Conversion
Location: Line 660
str(init_presets) is called when presets is not a list/tuple. Add explicit type validation:
if isinstance(init_presets, (list, tuple)):
preset_value = ",".join(init_presets)
elif isinstance(init_presets, str):
preset_value = init_presets
else:
raise TypeError(f"init_params.presets must be str, list, or tuple, got {type(init_presets)}")🔵 Suggestion: Document GR00T 1.6 vs 1.7 Compatibility
The GR00T16_RLINF_COMPAT comments are helpful but scattered. Consider adding a docstring or README section explaining the compatibility matrix and which environment variables can customize behavior.
Verdict
The core logic appears sound for the intended use case. The dual-version support for GR00T 1.6/1.7 is well-structured. Main concerns are around maintainability (broad exception handling, fragile directory discovery) and test coverage rather than correctness bugs.
No blocking issues found. The suggestions above would improve long-term maintainability.
Automated review by IsaacLab Review Bot---
Update (d6180ea): Reviewed incremental commits since 1a0d62b. New changes are CI/infrastructure improvements unrelated to the rlinf preset logic:
- Added
exclude-patterninput to test actions for flexible test filtering - Folded standalone
verify-*-non-rootjobs intotest-isaaclab-ovandtest-curobo(saves runner time) - Added JUnit XML artifact uploads for test reporting
- Added
libgmp-devfor arm64 pytetwild builds - Excluded rendering tests from
isaaclab_tasksshards viaexclude-pattern - Removed
RENDERING_CORRECTNESS_TESTS/RENDERING_CORRECTNESS_KITLESS_TESTSfromtools/test_settings.py - Trimmed integration tests from
test_shadow_hand_vision_presets.py
✅ No new issues found in the incremental changes. Previous suggestions on extension.py (broad exception handling, fragile directory walk, missing tests) remain applicable but are not blocking.
Greptile SummaryThis PR updates the RLinf extension wrapper to support dual GR00T 1.6/1.7 embodiment modules, apply IsaacLab environment presets before
Confidence Score: 3/5The env worker closure now silently swallows both simulation_io import failures, leaving converters unregistered with no warning; training will start and crash later with a cryptic KeyError rather than failing early with a clear message. The preset-application logic and dual GR00T-module patching are structurally sound, but the silent-failure path in _register_gr00t_converters is a real defect: if both module imports fail the function completes without registering anything and the _registered guard blocks any retry, so the problem is permanent for the process lifetime. The hardcoded sim_to_real_so101.tasks default is also a workshop-specific value committed to a general-purpose contrib module, producing error noise for all standard users. source/isaaclab_contrib/isaaclab_contrib/rl/rlinf/extension.py — specifically the _register_gr00t_converters silent-failure path (lines 290–316) and the ISAACLAB_TASK_PACKAGES default (lines 575–578). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[make_env_isaaclab called in child process] --> B[AppLauncher starts]
B --> C[Pre-import ISAACLAB_TASK_PACKAGES]
C --> D{ISAACLAB_GROUND_PLANE_USD or local cache found?}
D -- Yes --> E[Patch GroundPlaneCfg.usd_path]
D -- No --> F[Skip patch]
E --> G[load_cfg_from_registry]
F --> G
G --> H{init_params.presets set?}
H -- Yes --> I[collect_presets]
I --> J[parse_overrides]
J --> K[apply_overrides]
K --> L[resolve_presets - unconditional]
H -- No --> L
L --> M[Set num_envs]
M --> N[gym.make with patched cfg]
N --> O[Return env, sim_app]
Reviews (1): Last reviewed commit: "fix(rlinf): apply presets before IsaacLa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _extra_pkgs = os.environ.get( | ||
| "ISAACLAB_TASK_PACKAGES", | ||
| "sim_to_real_so101.tasks", | ||
| ) |
There was a problem hiding this comment.
ISAACLAB_TASK_PACKAGES defaults to "sim_to_real_so101.tasks", which is a workshop-specific package absent in any vanilla Isaac Lab installation. Every env worker spawned by a standard RLinf user will print:
[GR00T16_RLINF_COMPAT] could not pre-import task package 'sim_to_real_so101.tasks': ...
on every launch. The default should be "" (empty string) so the loop is a no-op unless the user explicitly sets the variable.
| _simulation_io_modules = [] | ||
| try: | ||
| from rlinf.models.embodiment.gr00t import simulation_io as _gr00t17_simio # noqa: WPS433 | ||
| _simulation_io_modules.append(_gr00t17_simio) | ||
| except Exception as _e: # pragma: no cover | ||
| logger.info(f"GR00T 1.7 simulation_io not present (skipping): {_e}") | ||
| try: | ||
| from rlinf.models.embodiment.gr00t_1_6 import simulation_io as _gr00t16_simio # noqa: WPS433 | ||
| _simulation_io_modules.append(_gr00t16_simio) | ||
| except Exception as _e: # pragma: no cover | ||
| logger.info(f"GR00T 1.6 simulation_io not present (skipping): {_e}") | ||
|
|
||
| obs_converter_type = cfg.get("obs_converter_type", "dex3") | ||
|
|
||
| if obs_converter_type not in simulation_io.OBS_CONVERSION: | ||
| simulation_io.OBS_CONVERSION[obs_converter_type] = _convert_isaaclab_obs_to_gr00t | ||
| logger.info(f"Registered obs converter: {obs_converter_type}") | ||
|
|
||
| if obs_converter_type not in simulation_io.ACTION_CONVERSION: | ||
| simulation_io.ACTION_CONVERSION[obs_converter_type] = _convert_gr00t_to_isaaclab_action | ||
| logger.info(f"Registered action converter: {obs_converter_type}") | ||
| for simulation_io in _simulation_io_modules: | ||
| if obs_converter_type not in simulation_io.OBS_CONVERSION: | ||
| simulation_io.OBS_CONVERSION[obs_converter_type] = _convert_isaaclab_obs_to_gr00t | ||
| logger.info( | ||
| f"Registered obs converter '{obs_converter_type}' on " | ||
| f"{simulation_io.__name__}" | ||
| ) | ||
| if obs_converter_type not in simulation_io.ACTION_CONVERSION: | ||
| simulation_io.ACTION_CONVERSION[obs_converter_type] = _convert_gr00t_to_isaaclab_action | ||
| logger.info( | ||
| f"Registered action converter '{obs_converter_type}' on " | ||
| f"{simulation_io.__name__}" | ||
| ) |
There was a problem hiding this comment.
Both import attempts inside _register_gr00t_converters are individually swallowed with logger.info. If both fail (e.g. a fresh RLinf install where neither gr00t nor gr00t_1_6 is on the path), _simulation_io_modules stays empty, no converters are registered, and the _registered guard prevents any retry. The first training step that calls a converter will then raise a KeyError deep in RLinf with no hint about what went wrong. At a minimum a logger.warning (or logger.error) should be emitted when _simulation_io_modules is empty after the loop.
| preset_scalar, | ||
| presets, | ||
| ) | ||
| isaac_env_cfg = resolve_presets(isaac_env_cfg) |
There was a problem hiding this comment.
resolve_presets called unconditionally changes behavior for non-preset users
resolve_presets(isaac_env_cfg) is now called on every env construction, even when init_presets is absent. Before this PR no preset resolution happened at all. If resolve_presets is not a pure no-op on a config that was never passed through apply_overrides, this silently alters the environment config for every user who doesn't set presets in their YAML — a regression that will only surface at runtime. Consider guarding this call inside the if init_presets: block, or explicitly confirming that the function is safe to call unconditionally.
1 participant
Summary
gym.make().default,physx,newton_mjwarp, etc.) for RLinf-launched IsaacLab tasks.Validation
b6c7ea780255ce8d608cccbbb4ea35eb9578943b.scripts/verify_portability.shin the downstream SO-101 integration repo.