[Fix] Defer pxr loading in SimulationContext and AssetBase#5826
Conversation
Importing AssetBase / BaseArticulation forced pxr (USD) and downstream omni.* extensions to load at module-import time. That ordering broke conditional AppLauncher() patterns: any user script doing `from isaaclab.assets import Articulation` at module top loaded pxr before Kit, and Kit's later USD extension registration then failed with a cascade of "extension class wrapper for base class pxr...::TfNotice has not been created yet" errors and a SEGV. Three changes: * asset_base.py: move `from pxr import Usd` under TYPE_CHECKING; drop module-top `from isaaclab.sim.simulation_context import SimulationContext` and `from isaaclab.sim.utils.stage import get_current_stage`. Call sites use `sim_utils.SimulationContext.instance()` and `sim_utils.get_current_stage()` via the already-lazy isaaclab.sim package, so pxr is loaded on first runtime access instead of at import. * base_articulation.py: defer `from ...sim import SimulationContext` to the body of __init__ (its only use site). `from isaaclab.assets import Articulation` no longer adds pxr / omni / carb / isaacsim to sys.modules. A full smoke test that imports asset classes at module top, then launches AppLauncher, then constructs an Articulation now succeeds end-to-end.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR defers pxr loading in asset base classes to runtime, unblocking workflows that import isaaclab.assets before launching Kit. The implementation correctly leverages the existing lazy-loading infrastructure in isaaclab.sim to achieve deferred loading without introducing new complexity.
Design Assessment
Design is sound. The approach is minimal and surgical:
- Moving
from pxr import UsdunderTYPE_CHECKINGpreserves type hints while deferring the actual import - Using
sim_utils.SimulationContext.instance()instead of direct imports leverages the existinglazy_loaderinfrastructure - Deferring the
SimulationContextimport to__init__inbase_articulation.pyis appropriate since that's the only call site
The PR description is exemplary — it clearly explains the mechanism, provides verification steps, and acknowledges follow-up work for sensors.
Findings
🔵 Suggestion: Consider documenting the lazy-loading pattern — source/isaaclab/isaaclab/assets/asset_base.py
The TYPE_CHECKING pattern for deferring pxr imports could benefit from a brief inline comment explaining why it's structured this way. Future contributors unfamiliar with the import ordering constraints might inadvertently revert to direct imports.
if TYPE_CHECKING:
# pxr is imported under TYPE_CHECKING to defer loading until runtime.
# This allows `from isaaclab.assets import Articulation` before AppLauncher.
from pxr import UsdTest Coverage
- Bug fix: This is a fix for import-time side effects
- Regression test: The PR description mentions manual verification steps, and references
test_env_cfg_no_forbidden_imports.pyas a potential extension. The existing articulation tests should catch any runtime regressions. - Gaps: Consider adding a test that explicitly validates
from isaaclab.assets import Articulationdoesn't triggerpxrloading (as mentioned in "Out of scope / follow-ups" section 4)
CI Status
Several checks still in progress (Build Base Docker Image, Installation Tests). Core checks (pre-commit, changelog, Build Wheel) have passed.
Verdict
No issues found
Clean implementation with proper lazy-loading patterns. The changes are minimal, correct, and well-documented. The only suggestion is adding a clarifying comment for maintainability.
Pushes the pxr deferral down to the producer module. simulation_context.py no longer does `from pxr import Gf, Usd, UsdGeom, UsdPhysics, UsdUtils` at module top; instead `__init__` does a local `from pxr import UsdUtils` and `_init_usd_physics_scene` does a local `from pxr import Gf, UsdGeom, UsdPhysics`. The `Usd` annotation on `_predicate(prim: Usd.Prim)` stays typed via the existing `from __future__ import annotations` (already at line 6) + a TYPE_CHECKING import. stage_utils, create_new_stage, and close_stage are reached via sim_utils.X / a function-local `from isaaclab.sim.utils import stage` so simulation_context.py's module-top imports no longer transitively load pxr through stage.py either. With this in place, `from isaaclab.sim import SimulationContext` is pxr-free at module top, which means the earlier `base_articulation.py:__init__`-local `from ...sim import SimulationContext` workaround is no longer needed -- reverted to the original module-top form.
1. Summary
from isaaclab.sim import SimulationContextandfrom isaaclab.assets import Articulationno longer loadpxrat module-import time2. Mechanism
source/isaaclab/isaaclab/sim/simulation_context.py: removed module-topfrom pxr import Gf, Usd, UsdGeom, UsdPhysics, UsdUtils,import isaaclab.sim.utils.stage as stage_utils, andfrom isaaclab.sim.utils import create_new_stage.Usdis now underTYPE_CHECKING.__init__and_init_usd_physics_sceneeach do a function-localfrom pxr import …at the top of the method.create_new_stage/close_stageare reached via the already-lazysim_utils.X, andstage_utils._contextis reached via a function-localfrom isaaclab.sim.utils import stage as stage_utils.source/isaaclab/isaaclab/assets/asset_base.py:from pxr import Usdmoved underTYPE_CHECKING; module-topfrom isaaclab.sim.simulation_context import SimulationContextandfrom isaaclab.sim.utils.stage import get_current_stageremoved. Call sites usesim_utils.SimulationContext.instance()andsim_utils.get_current_stage()via the already-lazyisaaclab.simpackage, so pxr loads on first runtime access rather than at import.3. Verification
Module-top import hook (forbidden =
pxr,omni,carb,isaacsim, matchingsource/isaaclab_tasks/test/test_env_cfg_no_forbidden_imports.py):from isaaclab.sim import SimulationContextandfrom isaaclab.assets import Articulationloadpxr,pxr.Tf,pxr.Tf._tf, ... intosys.modules.sim_utils.SimulationContext.instance()later triggers the load at runtime.Kit smoke test (module-top imports of
SimulationContextandArticulation, thenAppLauncher(args), then construct):AppLauncherstartup raises a cascade ofRuntimeError: extension class wrapper for base class pxrInternal_v0_25_11__pxrReserved__::TfNotice has not been created yetacrossomni.UsdMdl,pxr.PhysxSchema,omni.usd,pxr.UsdSkel,omni.physx, ending in a SEGV whenSimulationContext.__init__callsomni.usd.get_context().STAGE1_TOPLEVEL_CLEAN→STAGE2_APPLAUNCHED→STAGE3_SIMCTX→STAGE4_GROUND→STAGE5_ARTICULATIONall pass.4. Out of scope / follow-ups
from pxr import …at module top:source/isaaclab/isaaclab/sensors/camera/camera.py,source/isaaclab/isaaclab/sensors/ray_caster/base_ray_caster.py,source/isaaclab/isaaclab/sensors/ray_caster/base_ray_caster_camera.py,source/isaaclab/isaaclab/sensors/ray_caster/base_multi_mesh_ray_caster.py.source/isaaclab/isaaclab/scene/interactive_scene.pyandsource/isaaclab/isaaclab/sim/utils/stage.pystill loadpxrat module top. Every spawner undersource/isaaclab/isaaclab/sim/spawners/**does too.source/isaaclab_tasks/test/test_env_cfg_no_forbidden_imports.py(or a new sibling) to cover barefrom isaaclab.assets import *,from isaaclab.sim import *, andfrom isaaclab.sensors import *at module top would lock the discipline in.5. Test plan
./isaaclab.sh -fclean