[cherry pick] use lazy runtime imports to import leapp in export.py#5870
Conversation
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
PR Type: Cherry-pick from #5869 (bug fix)
Target Branch: release/3.0.0-beta2
Summary
This PR cherry-picks lazy runtime import changes for the LEAPP export script from #5869 to the release branch.
Review Findings
✅ Cherry-pick is clean — The code changes are identical to the original PR. Line number differences are expected due to the different base branch state.
✅ Lazy import pattern is correct:
- Top-level
torch,leapp, andannotateimports moved to_load_runtime_dependencies() torch.jit._state.disable()now called aftertorchis imported at runtimeTYPE_CHECKINGguard used appropriately for type hints (torch_type)- Module-level global declarations (
torch = None, etc.) enable proper lazy loading
✅ Import order fix:
setup_preset_cliimport moved insideparse_export_args()fold_preset_tokensimport moved insiderun_export_with_hydra()- New
parse_prelaunch_args()function allows CLI parsing before Isaac Sim launch
✅ Runtime setup:
prepare_cli_export_runtime()properly initializes stage and settingsmain_cli()now correctly sequences: prelaunch args → AppLauncher → full arg parsing → export
Verdict
LGTM 👍 — Clean cherry-pick that correctly applies the lazy import pattern to fix the CLI export flow on the release branch.
Update (d9e24fb): New commit further refactors the simulation lifecycle management:
✅ Removed parse_prelaunch_args() — No longer needed; simulation launch is now deferred to the Hydra-decorated function.
✅ Introduced launch_simulation context manager — run_export_with_hydra() now uses isaaclab_tasks.utils.sim_launcher.launch_simulation to handle AppLauncher lifecycle within the Hydra config context.
✅ Optional simulation_app parameter — export_rsl_rl_agent() now accepts simulation_app=None with a null-check before the is_running() wait loop, supporting both context-managed and standalone invocation.
✅ Local import aliasing — Added Any type import and used local aliases (hydra_task_config_fn, main_fn) to improve type checker compatibility.
✅ Simplified main_cli() — Now just parses args and calls run_export_with_hydra(); no manual AppLauncher creation needed.
Verdict: Changes look good — cleaner separation of concerns with the context manager pattern. LGTM 👍
Update (0dde313): Further simplification removing type-hint workarounds:
✅ Removed TYPE_CHECKING guard — The torch_type import is no longer needed since type annotations now use plain device and dtype without explicit type hints.
✅ Top-level isaaclab_tasks.utils imports — fold_preset_tokens and setup_preset_cli are now imported at module top-level instead of inside functions; safe since these don't trigger heavy dependencies.
✅ Removed prepare_cli_export_runtime() — This function is no longer needed with the launch_simulation context manager handling stage setup.
✅ Simplified ensure_actor_hidden_state_initialized signature — Now uses untyped device and dtype parameters instead of torch_type.device/torch_type.dtype.
✅ Removed Any type workarounds — hydra_task_config_fn and main_fn local aliases removed; functions are now called directly without type-checker evasion.
Verdict: Clean simplification that removes unnecessary indirection. LGTM 👍
Greptile SummaryThis cherry-pick fixes a CLI import-order regression introduced by PR #5633 by deferring all heavy imports (
Confidence Score: 3/5The refactored import ordering is correct overall, but the getattr(leapp_module, 'annotate') call in _load_runtime_dependencies could fail at runtime with an AttributeError instead of cleanly importing the annotate submodule, which would make every export attempt crash after App launch. The two-phase launch sequence and lazy-import pattern are sound, but the substitution of getattr(leapp_module, 'annotate') for the conventional 'from leapp import annotate' deviates from how every other file in this repo imports that submodule. If leapp/init.py does not re-export annotate, the global will remain unset and any policy exercising the recurrent-state annotation path will raise AttributeError deep inside export_rsl_rl_agent. scripts/reinforcement_learning/leapp/rsl_rl/export.py — specifically the leapp annotate import inside _load_runtime_dependencies Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as main_cli(argv)
participant PLA as parse_prelaunch_args
participant AL as AppLauncher / Kit
participant PEA as parse_export_args
participant REWH as run_export_with_hydra
participant LRD as _load_runtime_dependencies
participant PCER as prepare_cli_export_runtime
participant ERA as export_rsl_rl_agent
CLI->>PLA: parse minimal args (no task utils)
PLA-->>CLI: "prelaunch_args (headless=True)"
CLI->>AL: AppLauncher(prelaunch_args)
AL-->>CLI: simulation_app
CLI->>PEA: parse_export_args(argv)
Note over PEA: imports isaaclab_tasks.utils.setup_preset_cli
PEA-->>CLI: args_cli, hydra_args
CLI->>REWH: run_export_with_hydra(args_cli, hydra_args, simulation_app)
REWH->>LRD: _load_runtime_dependencies()
Note over LRD: imports torch, leapp, annotate,<br/>rsl_rl, isaaclab modules
LRD-->>REWH: globals populated
REWH->>REWH: "@hydra_task_config → _main(env_cfg, agent_cfg)"
REWH->>PCER: prepare_cli_export_runtime()
Note over PCER: create_new_stage(), set RTX/physics flags
PCER-->>REWH: done
REWH->>ERA: export_rsl_rl_agent(args_cli, env_cfg, agent_cfg, simulation_app)
ERA-->>REWH: exported (bool)
REWH-->>CLI: exported
CLI->>AL: simulation_app.close()
Reviews (1): Last reviewed commit: "moved imports to use the lazy import par..." | Re-trigger Greptile |
| try: | ||
| import leapp as leapp_module | ||
| except ImportError as e: | ||
| raise ImportError("LEAPP package is required for policy export. Install with: pip install leapp") from e | ||
| annotate_module = getattr(leapp_module, "annotate") |
There was a problem hiding this comment.
getattr(leapp_module, "annotate") does not reliably import a submodule — Python only exposes submodules as attributes after they have been explicitly imported. Every other file in this codebase (utils.py, export_annotator.py, tutorial scripts) uses from leapp import annotate, which triggers a proper importlib lookup of leapp.annotate as a submodule when it isn't yet in leapp.__dict__. Using getattr will raise AttributeError at runtime if leapp/__init__.py doesn't re-export annotate itself.
| try: | |
| import leapp as leapp_module | |
| except ImportError as e: | |
| raise ImportError("LEAPP package is required for policy export. Install with: pip install leapp") from e | |
| annotate_module = getattr(leapp_module, "annotate") | |
| try: | |
| import leapp as leapp_module | |
| from leapp import annotate as annotate_module | |
| except ImportError as e: | |
| raise ImportError("LEAPP package is required for policy export. Install with: pip install leapp") from e |
cherry pick of #5869
Fixes # (issue)
Following #5633, the CLI path broke due to an error in import order. That resulted in a failure inside the simulation setup which would cause a silent failure when running the CLI. This fix conforms to the new
_load_runtime_dependencies()pattern introduced by the previous PR and fixes the runtime for the export. New export script follows these steps:Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there