Warp-first experimental envs implements lazy export + cfg implementation split#5916
Warp-first experimental envs implements lazy export + cfg implementation split#5916ooctipus wants to merge 4 commits into
Conversation
ooctipus
requested review from
Mayankm96,
Toni-SM,
jtigue-bdai and
kellyguo11
as code owners
github-actions
Bot
added
bug
![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: Warp-first experimental envs + physics-event backend decoupling
Reviewed at: 4b5b475b
Summary
This PR refactors physics-event randomization terms into per-backend implementations (isaaclab_physx, isaaclab_newton, isaaclab_ovphysx), separates env configs from env implementations to enable lazy imports via lazy_export() + .pyi stubs, and moves heavyweight imports (pxr, carb, Articulation, etc.) behind TYPE_CHECKING guards to keep the env-cfg parse path simulator-free.
The architecture is clean and well-motivated. Below are specific findings.
Findings
1. 🔴 [Critical] Missing isaaclab.envs.mdp.physics_events module — broken imports in backend files and tests
Files:
source/isaaclab_newton/isaaclab_newton/envs/mdp/physics_events.py(line 19)source/isaaclab_physx/isaaclab_physx/envs/mdp/physics_events.py(line 20)source/isaaclab_newton/test/assets/test_newton_actuators_newton.py(line 531)source/isaaclab_physx/test/assets/test_newton_actuators_physx.py(line 477)
Both backend physics_events.py files import:
from isaaclab.envs.mdp.physics_events import randomize_prop_by_op, validate_scale_rangeAnd both test files import:
from isaaclab.envs.mdp.physics_events import randomize_actuator_gainsHowever, source/isaaclab/isaaclab/envs/mdp/physics_events.py does not exist in the repository tree at this commit (nor on develop). The base events.py has _randomize_prop_by_op and _validate_scale_range (private names), but no physics_events submodule.
This appears to depend on a prerequisite PR that extracts these utilities into physics_events.py. Without that dependency merged first, all new backend files will fail to import at runtime.
Action needed: Either include the physics_events.py extraction in this PR, or document/enforce the merge dependency.
2. 🔴 [High] PhysX RandomizePhysicsSceneGravity ignores the distribution parameter at runtime
File: source/isaaclab_physx/isaaclab_physx/envs/mdp/physics_events.py (line ~235)
gravity = randomize_prop_by_op(
gravity,
(self._dist_param_0.cpu(), self._dist_param_1.cpu()),
None,
slice(None),
operation=operation,
distribution="uniform", # ← hard-coded, ignores `distribution` argument
)The Newton implementation correctly resolves the distribution function at init time via self._dist_fn. The PhysX implementation hard-codes "uniform", so users configuring distribution="gaussian" or "log_uniform" will silently get uniform sampling.
Suggested fix:
distribution=distribution,3. 🟡 [Medium] Newton RandomizeRigidBodyMaterial ignores runtime call parameters
File: source/isaaclab_newton/isaaclab_newton/envs/mdp/physics_events.py (lines 75–90 in __call__)
The __call__ signature accepts static_friction_range, dynamic_friction_range, and restitution_range as arguments, but the implementation samples from self._static_friction_range and self._restitution_range (cached at __init__ time):
friction_range = torch.tensor(self._static_friction_range, device=device)
restitution_range_t = torch.tensor(self._restitution_range, device=device)Unlike PhysX (which pre-samples into fixed buckets due to the 64K material limit), Newton has no such constraint. If the event manager ever passes updated ranges at call time (e.g., curriculum-driven), Newton will silently ignore them.
Suggested fix: Use the call-time static_friction_range / restitution_range parameters for sampling. Reserve self._*_range only as a fallback when call-time values are not provided.
4. 🟡 [Medium] Top-level pxr / sim_utils imports in stable modules may break pre-SimulationApp workflows
Files:
source/isaaclab/isaaclab/envs/utils/camera_view.py(line 17:from pxr import UsdGeom)source/isaaclab/isaaclab/scene_data/scene_data_provider.py(line 16:from pxr import UsdGeom)source/isaaclab/isaaclab/sim/schemas/schemas_actuators.py(line 27:from pxr import Sdf, Usd)source/isaaclab/isaaclab/terrains/utils.py(line 14:from pxr import UsdGeom)
These files previously deferred pxr and sim_utils imports to prevent Isaac Sim from launching at module import time. The original code had explicit comments:
# need to import these here to prevent isaacsim launching when importing this moduleThe stated goal of this PR is to keep the env-cfg parse path clean, but these utility modules are not part of the env-cfg parse path. If any downstream code imports isaaclab.terrains.utils or isaaclab.sim.schemas.schemas_actuators before SimulationApp is launched, it will now crash.
Suggested fix: Verify that these modules are never imported prior to SimulationApp initialization. If there's any risk, keep the deferred import pattern — it was explicitly added to prevent this class of bug.
5. 🟡 [Medium] {DIR} string-based class_type convention undocumented
File: source/isaaclab_experimental/isaaclab_experimental/envs/mdp/actions/actions_cfg.py (lines 47, 65)
class_type: type[JointPositionAction] | str = "{DIR}.joint_actions:JointPositionAction"This is a novel pattern for lazy class resolution. Contributors unfamiliar with the mechanism will not understand:
- What
{DIR}resolves to at runtime - When and how the string is converted to an actual class reference
- Whether this is an officially supported pattern or experimental
Suggested fix: Add a brief docstring or comment explaining the {DIR} convention (e.g., "Resolved at runtime by the experimental manager; {DIR} is replaced with the directory of this config file").
6. 🟢 [Low] CI "Check changelog fragments" failing — missing fragments for backend packages
The PR adds new envs/mdp/physics_events.py modules to isaaclab_newton, isaaclab_physx, and isaaclab_ovphysx, but only provides changelog fragments for isaaclab_experimental and a .skip file for isaaclab_tasks. The failing CI check likely expects fragments for the backend packages as well.
7. 🟢 [Low] Newton RandomizeRigidBodyCom stability warning could be a runtime log
File: source/isaaclab_newton/isaaclab_newton/envs/mdp/physics_events.py (class docstring, ~line 165)
The docstring notes that CoM randomization may cause simulation instability on Newton because notify_model_changed(BODY_INERTIAL_PROPERTIES) doesn't fully recompute the mass matrix. This is valuable information, but users who don't read source docstrings will miss it.
Suggestion: Emit a logger.warning() on first call to surface this caveat at runtime.
Overall Assessment
The core architecture — backend-dispatched physics events, lazy exports, and import deferral for env configs — is sound and addresses a real pain point (importing env configs before SimulationApp).
Key blockers:
- The missing
isaaclab.envs.mdp.physics_eventsmodule makes the new backend implementations non-functional. This is either a missing file in this PR or an undocumented dependency on another PR. - The PhysX gravity distribution bug introduces a silent behavioral difference from configuration intent.
Non-blocking concerns:
3. Newton material randomizer ignoring runtime params (medium risk for curriculum workflows).
4. Elevated pxr imports in utility modules (risk depends on import graphs).
CI changelog failure should be resolved before merge.
Update (12ede03): Reviewed incremental changes from 4b5b475 → 12ede035.
New commits complete the lazy-import refactoring for the experimental packages:
- ✅ Env configs extracted to dedicated
*_cfg.pymodules (ant, cartpole, humanoid) — clean separation of config from runtime env class - ✅ All experimental
__init__.pyfiles converted tolazy_export()pattern - ✅ MDP term leaves (
rewards,observations,terminations,events,curriculums) correctly guardArticulation,ContactSensor,TerrainImporterunderTYPE_CHECKING - ✅
env_cfg_entry_pointstrings updated to reference the new cfg modules - ✅ Changelog fragment added
No new issues found in this increment. Previous findings (1–7) remain as-is — they apply to code not touched in this push.
ooctipus
changed the title
ooctipus
changed the title
Greptile SummaryThis PR decouples physics-engine-specific event terms from the shared
Confidence Score: 4/5The backend dispatch mechanism and The architecture of the backend dispatch layer is sound — constructor signatures in Newton, PhysX, and OVPhysX all match what source/isaaclab/isaaclab/envs/mdp/physics_events.py (the Important Files Changed
Sequence DiagramsequenceDiagram
participant EC as EventManager
participant PE as isaaclab.envs.mdp.physics_events
participant RBE as _resolve_backend_event()
participant SC as SimulationContext
participant BACK as isaaclab_{backend}.envs.mdp
EC->>PE: randomize_rigid_body_material.__init__(cfg, env)
PE->>RBE: _resolve_backend_event("RandomizeRigidBodyMaterial", cfg, env, asset, asset_cfg)
RBE->>SC: SimulationContext.instance().physics_manager.__name__
SC-->>RBE: "PhysxManager" / "NewtonManager" / "OVPhysxManager"
RBE->>BACK: "importlib.import_module("isaaclab_{backend}.envs.mdp")"
BACK-->>RBE: lazy module proxy
RBE->>BACK: "getattr(module, "RandomizeRigidBodyMaterial")(*args)"
BACK-->>RBE: impl instance
RBE-->>PE: self._impl
EC->>PE: randomize_rigid_body_material.__call__(env, env_ids, ...)
PE->>BACK: self._impl(env, env_ids, ...)
BACK-->>EC: (done)
|
There was a problem hiding this comment.
Code Review: Lazy export + cfg implementation split for Warp-first experimental envs
Summary
This PR implements lazy imports via lazy_export() + .pyi stubs and separates env configs from env implementations to allow importing task configurations before SimulationApp is launched. It also moves heavyweight runtime type imports (Articulation, InteractiveScene, ContactSensor, etc.) behind TYPE_CHECKING guards.
Findings
1. 🟠 [Medium] Top-level pxr and sim_utils imports may break pre-SimulationApp workflows
Files:
source/isaaclab/isaaclab/envs/utils/camera_view.py(lines 17, 21)source/isaaclab/isaaclab/scene_data/scene_data_provider.py(lines 16, 18)source/isaaclab/isaaclab/sim/schemas/schemas_actuators.py(lines 27-29)source/isaaclab/isaaclab/terrains/utils.py(lines 14, 17)
These files previously used deferred imports inside functions to prevent pxr and sim_utils from being loaded at module import time. The original code had explicit comments:
# need to import these here to prevent isaacsim launching when importing this moduleThe refactor hoists these imports to module level. While this simplifies the code, it may break downstream workflows that import these modules before SimulationApp is initialized.
Risk assessment: If any of these modules are reachable from env-cfg parse paths, the PR's stated goal of keeping env-cfg imports simulator-free would be violated.
Suggested action: Verify that isaaclab.terrains.utils, isaaclab.envs.utils.camera_view, isaaclab.scene_data.scene_data_provider, and isaaclab.sim.schemas.schemas_actuators are not transitively imported during env-cfg loading. If they are, the deferred import pattern should be preserved.
2. 🟡 [Low] Undocumented {DIR} string-based class_type convention
File: source/isaaclab_experimental/isaaclab_experimental/envs/mdp/actions/actions_cfg.py (lines 47, 65)
class_type: type[JointPositionAction] | str = "{DIR}.joint_actions:JointPositionAction"This introduces a lazy class resolution pattern where {DIR} is presumably replaced at runtime with the directory of the config file. However:
- There's no documentation explaining what
{DIR}resolves to - Contributors unfamiliar with this mechanism may not understand how to use it
- The
|type union syntax (type[X] | str) is valid in Python 3.10+ but the runtime resolution mechanism is not standard
Suggested fix: Add a brief docstring or comment explaining the convention, e.g.:
# {DIR} is resolved at runtime to the directory containing this config module.
# This enables lazy class loading without eagerly importing the implementation.3. 🟡 [Low] Missing .pyi stub files referenced by lazy_export()
Files:
source/isaaclab_experimental/isaaclab_experimental/envs/__init__.pysource/isaaclab_experimental/isaaclab_experimental/envs/mdp/__init__.pysource/isaaclab_experimental/isaaclab_experimental/envs/mdp/actions/__init__.pysource/isaaclab_experimental/isaaclab_experimental/managers/__init__.py
The module docstrings reference "lazy __init__.py + .pyi stubs (with explicit __all__)", but the diff doesn't include the corresponding .pyi stub files. The lazy_export() function relies on these stubs to determine which symbols to export.
Impact: Without the stub files, the lazy export mechanism may not expose the expected symbols, causing ImportError or AttributeError at runtime when accessing module members.
Question: Are the .pyi files included in the PR but not shown in this diff, or do they need to be added?
4. 🟢 [Nit] Inconsistent import aliasing in scene_data_provider.py
File: source/isaaclab/isaaclab/scene_data/scene_data_provider.py
The diff shows:
+import isaaclab.sim as sim_utilsBut the original code used:
import isaaclab.sim as isaaclab_simAnd the usage was changed from isaaclab_sim.resolve_prim_pose(prim) to sim_utils.resolve_prim_pose(prim).
While sim_utils is the more common alias across the codebase, this creates a minor inconsistency within the file's history. Not a functional issue.
5. 🟢 [Good] Proper use of TYPE_CHECKING guards
The PR correctly moves runtime type imports behind TYPE_CHECKING blocks in:
events.pyobservations.pyrewards.pyterminations.pyscene_entity_cfg.py- All task-specific MDP modules
This is the correct pattern for avoiding circular imports and keeping config modules lightweight.
6. 🟢 [Good] Config/implementation split is clean
The separation of *Cfg classes into dedicated *_cfg.py files (e.g., ant_env_warp_cfg.py, cartpole_warp_env_cfg.py, humanoid_warp_env_cfg.py) is well-executed:
- Entry points correctly updated to point to
*_cfg:*Cfg - Environment classes import their configs locally with
TYPE_CHECKINGguards where needed - No code duplication
Overall Assessment
The PR's core approach—lazy exports, TYPE_CHECKING guards, and config/implementation separation—is architecturally sound and addresses a real pain point (importing env configs before SimulationApp).
Items to verify before merge:
- Ensure the hoisted
pxrimports in stableisaaclabmodules don't break any pre-SimulationApp import paths - Confirm the
.pyistub files exist and are properly configured
Minor improvements:
- Document the
{DIR}string-based class resolution convention
Update (c117f97): New commit adds resample_interval_on_reset feature to EventTermCfg (both stable and experimental managers). Implementation is clean—proper docstring, correct logic, good test coverage with deterministic intervals. No issues found with the new code.
Previous concerns (1–3) from the original review remain unaddressed—they concern different files not touched in this commit. No action needed on those unless the lazy-export paths are affected.
Update (5251d1c): New commit adds a changelog entry (fix-warp-env-cfg-forbidden-imports.skip) documenting the intentional import hoisting from concern #1. The entry confirms this was deliberate and states no user-facing change, implying the affected modules are not on pre-SimulationApp import paths. ✅ Concern #1 acknowledged by author. No new issues found.---
Update (b103091): ✅ Concern #3 resolved — all .pyi stub files are now included (envs/__init__.pyi, envs/mdp/__init__.pyi, envs/mdp/actions/__init__.pyi, managers/__init__.pyi, plus task-specific stubs). The stubs have proper __all__ lists and explicit imports matching the lazy export pattern.
Additional change: scene_entity_cfg.py replaces isinstance(entity, BaseArticulation) with hasattr(entity, "num_joints") duck-typing to avoid importing BaseArticulation at module level — reasonable trade-off for the lazy-import architecture.
No new issues found.
Update (c67e2b0): Major cartpole task consolidation commit — merges direct_cartpole and manager_cartpole into unified isaaclab_tasks.core.cartpole package, drops -v0 suffixes from task IDs, removes all deprecated per-variant aliases (35+ retired gym.register entries), and updates docs/tests/scripts throughout. The lazy_export() + .pyi pattern is also extended to more experimental task MDP packages (locomotion, humanoid, reach) with proper stubs. The experimental env env_cfg_entry_point registrations are corrected to point at the new *_cfg.py files. Clean, consistent refactoring — no new issues found.
Previous concerns status:
The experimental Warp packages use lazy_export(), which requires a sibling .pyi stub to resolve names. Those stubs are gitignored (**/__*) and were never force-added, so a clean checkout (CI) had no managers/envs/mdp stubs. Loading any Warp env config then crashed (e.g. cannot import ObservationTermCfg / UniformVelocityCommandCfg), failing test_config_load_does_not_import_backend_modules for all 23 Warp tasks. Add and track the missing stubs for isaaclab_experimental (managers, envs, envs/mdp, envs/mdp/actions) and the four task mdp packages (velocity, cartpole, humanoid, reach). Also drop the top-level BaseArticulation import in the experimental SceneEntityCfg, which transitively pulled scene_data_provider -> pxr during config parsing. The resolve() articulation branch now duck-types on num_joints, mirroring the body branch.
|
|
||
| # -- Warp joint mask / ids for articulations | ||
| if isinstance(entity, BaseArticulation): | ||
| if hasattr(entity, "num_joints"): |
There was a problem hiding this comment.
Is this also because of importing baseArticulation pulls extra dependencies? The only concern is that it's not very obvious when things can be done and cannot. Wondering if there's a way to generalize or referred in agents.md
| keeping stable task configs and the stable `isaaclab.managers` package intact. | ||
| """ | ||
|
|
||
| from isaaclab.managers import * # noqa: F401,F403 |
There was a problem hiding this comment.
This fallback is needed I think like that in mdp in .pyi?
| "ManagerBasedEnvWarp", | ||
| "ManagerBasedRLEnvWarp", | ||
| ] | ||
| from . import mdp |
There was a problem hiding this comment.
Why this is different from others? agent suggests it should be in all.
| Uses ``FrameView`` first so PhysX/Fabric-backed transforms are current; falls | ||
| back to USD only if the backend view cannot be constructed. | ||
| """ | ||
| from pxr import UsdGeom |
There was a problem hiding this comment.
Would it be an extra safeguard to keep those? Or with lazy import properly setup, it won't be a problem any more?
2 participants
Summary
isaaclab_experimentalandisaaclab_tasks_experimental(direct + manager-based cartpole/humanoid/ant/reach/locomotion), with Warp-kernel implementations of actions, observations, rewards, terminations, and events.__init__.py+.pyistubs (with explicit__all__) across the new/updated packages.Test plan
./isaaclab.sh -p -m pytest source/isaaclab_tasks_experimental/test./isaaclab.sh -p -m pytest source/isaaclab_physx/test/assets/test_newton_actuators_physx.py source/isaaclab_newton/test/assets/test_newton_actuators_newton.pyrandomize_rigid_body_scaleraises on Newton and warns (deprecated) on PhysX.