[Exp] Warp-first experimental envs implements lazy export + cfg implementation split (#5916)#5943
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Lazy Export + Cfg/Implementation Split (Cherry-pick of #5916)
This is a well-structured cherry-pick that addresses a real pain point: importing experimental Warp env configs would eagerly pull in pxr, omni, and other backend modules, crashing if SimulationApp had not yet launched. The fix is architecturally sound and follows patterns already established in the stable isaaclab package.
✅ What Works Well
-
Lazy export pattern is consistent — Every
__init__.py→lazy_export()+.pyistub pair follows the same structure. The.pyistubs serve dual duty: IDE type resolution and runtime lazy-load manifests. -
Cfg/implementation split — Moving
*Cfgdataclasses into dedicated*_cfg.pyfiles while keeping the runtime env class in the original module is the correct approach. Config loading (e.g., gymnasium registryenv_cfg_entry_point) no longer triggers heavy imports. -
TYPE_CHECKINGguards — MovingArticulation,ContactSensor,InteractiveScene, etc. underif TYPE_CHECKING:in leaf MDP modules prevents runtime import chains that would pull in the simulator. This is clean and consistent across all affected files. -
isinstance(entity.cfg, ArticulationCfg)change inscene_entity_cfg.py— Avoids importingBaseArticulation(which itself imports backend code). Checking the config type is a lightweight proxy that achieves the same dispatch logic without the heavy import. -
Conflict resolution — The PR description clearly documents the three conflicts and their resolutions. The choices (keeping release-branch-side deferred imports for
camera_view.py) are correct.
⚠️ Observations & Minor Concerns
| # | File | Note |
|---|---|---|
| 1 | actions_cfg.py |
The class_type: ... | str = "{DIR}.joint_actions:JointPositionAction" pattern assumes the runtime config resolver handles {DIR} string interpolation. This is fine if lazy_export or the manager infrastructure already resolves these—just confirming this pattern is tested. |
| 2 | scene_entity_cfg.py |
The isinstance(entity.cfg, ArticulationCfg) check is semantically slightly different from isinstance(entity, BaseArticulation). If any non-articulation entity happened to use an ArticulationCfg (unlikely but possible with custom wrappers), it would incorrectly enter this branch. Low risk given the current codebase. |
| 3 | envs/mdp/__init__.pyi |
Uses from isaaclab.envs.mdp import * as a fallback for stable MDP terms. If the stable package adds a name that conflicts with an experimental override, the override wins (since it comes after). This is intentional per the comments, but worth a note for future maintainers. |
| 4 | schemas_actuators.py / terrains/utils.py / scene_data_provider.py |
Hoisting pxr / sim_utils imports to module top is the reverse of the typical deferred-import pattern. This works here because these modules are only imported after SimulationApp is already running (they are implementation modules, not config modules). Just ensure nothing transitively imports these at config-load time. |
🧪 Test Coverage
- The
test_env_cfg_no_forbidden_imports.pyextension to scanisaaclab_tasks_experimentalis the right gate — it will catch any regression where a config import pulls in forbidden backend modules. - The test correctly imports
isaaclab_tasks_experimentalto trigger gymnasium registration before scanning.
Summary
No blocking issues. The lazy-export + cfg split pattern is well-executed and solves the eager-import crash reliably. The PR is ready to merge once CI completes.
🔄 Update (1b35411)
Reviewed incremental changes from 1496594f → 1b354112:
- ✅ P1 Fixed: Agent config file names for
Isaac-Cartpole-Direct-Warp-v0now correctly referencestable_agents(rl_games_ppo_cfg.yaml, etc.) instead of non-existent renamed variants. - ✅ P1 Fixed: Manager-based cartpole (
Isaac-Cartpole-Warp-v0) now importsagentsfromisaaclab_tasks.manager_based.classic.cartpoleand usesagents.__name__for correct path resolution. - P2 (
scene_entity_cfg.pyhasattr guard): Not addressed — acceptable as low-risk observation.
No new issues found in the incremental diff. LGTM. 👍
hujc7
force-pushed
the
jichuanh/cherrypick-5916-release-3.0.0-beta2
branch
from
1496594 to
56aa42d
Compare
Greptile SummaryThis cherry-pick onto
Confidence Score: 3/5The lazy-export infrastructure and cfg/env split are correct, but both cartpole gym registrations point to agent config files that do not exist on disk; any RL training run against those environments will immediately fail. The core architectural changes are sound and consistent with established patterns in the codebase. The pxr hoisting in implementation files is safe because their parent packages use lazy_export. The config/env split for ant and humanoid looks correct. The two cartpole init.py files, however, rename agent config references to file names that do not exist in isaaclab_tasks; every RL framework that tries to load an agent config for these tasks will fail at runtime. source/isaaclab_tasks_experimental/isaaclab_tasks_experimental/direct/cartpole/init.py and source/isaaclab_tasks_experimental/isaaclab_tasks_experimental/manager_based/classic/cartpole/init.py both reference agent config files that do not exist; the rest of the changed files look safe. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["import isaaclab_tasks_experimental"] --> B["gym.register(...)"]
B --> C["env_cfg_entry_point\n*_cfg.py - pure data, no pxr"]
B --> D["entry_point\n*_env.py - runtime, pxr OK"]
B --> E["rl_games/rsl_rl/skrl/sb3 cfg\nrenamed files do not exist"]
C --> F["*_cfg.py imports\nDirectRLEnvCfg, ArticulationCfg\nlazy, no pxr at load time"]
G["from isaaclab_experimental.managers import SceneEntityCfg"]
G --> H["managers/__init__.py\nlazy_export()"]
H --> I[".pyi stub\nresolves imports lazily"]
I --> J["scene_entity_cfg.py\nimport ArticulationCfg\nnot BaseArticulation"]
K["from isaaclab_experimental.envs import mdp"]
K --> L["envs/__init__.py\nlazy_export()"]
L --> M["envs/mdp/__init__.py\nlazy_export()"]
M --> N["actions/__init__.py\nlazy_export()"]
N --> O["actions_cfg.py\nclass_type = {DIR}.joint_actions:..."]
Reviews (1): Last reviewed commit: "Warp-first experimental envs implements ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "rl_games_cfg_entry_point": f"{stable_agents}:rl_games_direct_ppo_cfg.yaml", | ||
| "rsl_rl_cfg_entry_point": f"{stable_agents}.rsl_rl_direct_ppo_cfg:CartpolePPORunnerCfg", | ||
| "skrl_cfg_entry_point": f"{stable_agents}:skrl_direct_ppo_cfg.yaml", | ||
| "sb3_cfg_entry_point": f"{stable_agents}:sb3_direct_ppo_cfg.yaml", | ||
| }, |
There was a problem hiding this comment.
The renamed references (rl_games_direct_ppo_cfg.yaml, rsl_rl_direct_ppo_cfg, skrl_direct_ppo_cfg.yaml, sb3_direct_ppo_cfg.yaml) do not exist in isaaclab_tasks.direct.cartpole.agents — the actual files are still rl_games_ppo_cfg.yaml, rsl_rl_ppo_cfg.py, skrl_ppo_cfg.yaml, and sb3_ppo_cfg.yaml. Any attempt to load RL training configs for Isaac-Cartpole-Direct-Warp-v0 (via load_cfg_from_registry or a training script) will fail with a FileNotFoundError or ImportError. The same pattern is broken in manager_based/classic/cartpole/__init__.py (see rl_games_manager_ppo_cfg.yaml etc.).
There was a problem hiding this comment.
Fixed. This release branch predates the core/contrib task refactor, so the agent configs in isaaclab_tasks.direct.cartpole.agents are still named rl_games_ppo_cfg.yaml / rsl_rl_ppo_cfg / skrl_ppo_cfg.yaml / sb3_ppo_cfg.yaml (the _direct_ infix only exists on develop). The cherry-pick now references the release-branch names. Verified by training Isaac-Cartpole-Direct-Warp-v0 with rl_games: the log loads .../direct/cartpole/agents/rl_games_ppo_cfg.yaml and completes.
There was a problem hiding this comment.
Great context — thanks for the training verification too. The release-branch naming makes sense given the refactor timeline. ✅
| "rl_games_cfg_entry_point": f"{agents.__name__}:rl_games_manager_ppo_cfg.yaml", | ||
| "rsl_rl_cfg_entry_point": f"{agents.__name__}.rsl_rl_manager_ppo_cfg:CartpolePPORunnerCfg", | ||
| "skrl_cfg_entry_point": f"{agents.__name__}:skrl_manager_ppo_cfg.yaml", | ||
| "sb3_cfg_entry_point": f"{agents.__name__}:sb3_manager_ppo_cfg.yaml", |
There was a problem hiding this comment.
The references to rl_games_manager_ppo_cfg.yaml, rsl_rl_manager_ppo_cfg:CartpolePPORunnerCfg, skrl_manager_ppo_cfg.yaml, and sb3_manager_ppo_cfg.yaml don't match any files in isaaclab_tasks.manager_based.classic.cartpole.agents — only rl_games_ppo_cfg.yaml, rsl_rl_ppo_cfg.py, skrl_ppo_cfg.yaml, and sb3_ppo_cfg.yaml exist there. Attempting to train on Isaac-Cartpole-Warp-v0 with any RL framework will immediately fail to resolve the agent config.
There was a problem hiding this comment.
Fixed, same root cause as the direct variant — repointed rl_games_manager_ppo_cfg.yaml / rsl_rl_manager_ppo_cfg / skrl_manager_ppo_cfg.yaml / sb3_manager_ppo_cfg.yaml back to the release-branch names (rl_games_ppo_cfg.yaml, rsl_rl_ppo_cfg:CartpolePPORunnerCfg, etc.) in isaaclab_tasks.manager_based.classic.cartpole.agents. All referenced files and the CartpolePPORunnerCfg class exist on this branch.
There was a problem hiding this comment.
👍 Consistent fix across both variants.
There was a problem hiding this comment.
Verified — the config entry points now correctly reference the existing files in isaaclab_tasks.manager_based.classic.cartpole.agents. Thanks for the quick fix! ✅
|
|
||
| # -- Warp joint mask / ids for articulations | ||
| if isinstance(entity, BaseArticulation): | ||
| if isinstance(entity.cfg, ArticulationCfg): |
There was a problem hiding this comment.
The guard changed from isinstance(entity, BaseArticulation) to isinstance(entity.cfg, ArticulationCfg). For Warp-first backends where the articulation object may not inherit from BaseArticulation, checking the config class is likely more correct. However, if any future scene entity uses an ArticulationCfg but does not expose entity.num_joints / entity.joint_ids (e.g., a thin wrapper or proxy type), this block would hit an AttributeError. Consider adding a hasattr(entity, "num_joints") guard or a comment explaining the assumption that ArticulationCfg always implies the full articulation interface.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
This is intentional and carried over unchanged from the merged PR #5916. The guard was switched to isinstance(entity.cfg, ArticulationCfg) specifically so the experimental SceneEntityCfg no longer imports BaseArticulation at module top, which transitively pulled scene_data_provider -> pxr during config parsing — the leak this PR exists to fix. For Warp-first backends the articulation object may not subclass BaseArticulation, so the cfg-type check is the deliberate replacement. Validated by test_env_cfg_no_forbidden_imports.py (224 passed, incl. 27 Warp tasks).
There was a problem hiding this comment.
Makes sense — cfg-type check avoids the transitive pxr import while remaining correct for Warp backends. Good that the import test validates this. ✅
…ion split (isaac-sim#5916) - Adds Warp-first experimental environments and MDP terms under `isaaclab_experimental` and `isaaclab_tasks_experimental` (direct + manager-based cartpole/humanoid/ant/reach/locomotion), with Warp-kernel implementations of actions, observations, rewards, terminations, and events. - Adds lazy `__init__.py` + `.pyi` stubs (with explicit `__all__`) across the new/updated packages. - [ ] `./isaaclab.sh -p -m pytest source/isaaclab_tasks_experimental/test` - [ ] Train the experimental Warp cartpole/humanoid/ant configs on the Newton backend and confirm parity with the stable envs. - [ ] `./isaaclab.sh -p -m pytest source/isaaclab_physx/test/assets/test_newton_actuators_physx.py source/isaaclab_newton/test/assets/test_newton_actuators_newton.py` - [ ] Verify `randomize_rigid_body_scale` raises on Newton and warns (deprecated) on PhysX.
hujc7
force-pushed
the
jichuanh/cherrypick-5916-release-3.0.0-beta2
branch
from
56aa42d to
1b35411
Compare
3 participants
Summary
Cherry-pick of #5916 (squash commit
429aff2) ontorelease/3.0.0-beta2. Fixes NVBug 6121889 (Warp env startup crash with the kit visualizer).isaaclab_experimentalandisaaclab_tasks_experimental(direct + manager-based cartpole/humanoid/ant/reach/locomotion).__init__.py+.pyistubs so a clean checkout can resolve the experimental Warp env configs without crashing.pxrleak in the experimentalSceneEntityCfg.Conflict resolution
Three files conflicted, all due to divergence between
developand the release branch:isaaclab_tasks_experimental/.../direct/cartpole/__init__.pyand.../manager_based/classic/cartpole/__init__.py— the develop versions reference theisaaclab_tasks.corepackage and the renamed agent configs (rl_games_direct_ppo_cfg.yaml,rl_games_manager_ppo_cfg.yaml, etc.) introduced by the core/contrib refactor, which is not on this release branch. Repointed to the release-branch agent package (isaaclab_tasks.direct.cartpole.agents/isaaclab_tasks.manager_based.classic.cartpole) and the release-branch agent filenames (rl_games_ppo_cfg.yaml,rsl_rl_ppo_cfg:CartpolePPORunnerCfg,skrl_ppo_cfg.yaml,sb3_ppo_cfg.yaml).isaaclab/envs/utils/camera_view.py— followed Warp-first experimental envs implements lazy export + cfg implementation split #5916: hoistedUsdGeomandFrameViewfrom the local function scope to the module top. Usedfrom pxr import UsdGeom(notSdf, UsdGeomas on develop), since the release file does not contain develop'sscenePartitionblock that usesSdf. Result is identical to develop'scamera_view.pyapart from that release-absent block.The forbidden-imports test modification landed at its release-branch path (
test/test_env_cfg_no_forbidden_imports.py) via rename detection.Validation (release branch + cherry-pick)
pytest test_env_cfg_no_forbidden_imports.py: 224 passed, 0 failed, including all 27 experimental Warp tasks (this is the exact regression guard Warp-first experimental envs implements lazy export + cfg implementation split #5916 restores).train.py --task Isaac-Cartpole-Direct-Warp-v0 --headless: completes, loadsrl_games_ppo_cfg.yaml, parsesCartpoleWarpEnvCfg, no traceback.Original PR: #5916