use lazy runtime imports to import leapp in export.py#5869
Conversation
|
@AntoineRichard, do you mind taking a look a this? This fixes an issue with the CLI call path seen after the recent refactors to unblock CI. |
Greptile SummaryThis PR fixes a silent CLI failure introduced in #5633 by deferring all
Confidence Score: 3/5The import-ordering fix is conceptually correct, but the getattr change for leapp.annotate may silently break the export path if leapp.annotate is a submodule not auto-imported by leapp.init. The getattr approach for annotate does not invoke Python's submodule import machinery, so annotate.state_tensors and annotate.update_state could raise AttributeError at runtime if leapp.init does not explicitly re-export annotate. Every other call site in the codebase uses from leapp import annotate, making this the sole divergence. scripts/reinforcement_learning/leapp/rsl_rl/export.py — specifically the annotate import inside _load_runtime_dependencies and the two-phase argv parsing in main_cli. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as main_cli(argv)
participant Pre as parse_prelaunch_args
participant App as AppLauncher / Kit
participant Full as parse_export_args
participant Hydra as run_export_with_hydra
participant Deps as _load_runtime_dependencies
participant Prep as prepare_cli_export_runtime
participant Export as export_rsl_rl_agent
CLI->>Pre: parse minimal args (no task utils)
Pre-->>CLI: "prelaunch_args (headless=True)"
CLI->>App: AppLauncher(prelaunch_args)
App-->>CLI: simulation_app
CLI->>Full: parse_export_args(argv) [imports setup_preset_cli lazily]
Full-->>CLI: args_cli, hydra_args
CLI->>Hydra: run_export_with_hydra(args_cli, hydra_args, simulation_app)
Hydra->>Deps: _load_runtime_dependencies() [imports torch, leapp, rsl_rl, isaaclab_tasks]
Deps-->>Hydra: globals populated
Hydra->>Hydra: "@hydra_task_config resolves env_cfg, agent_cfg"
Hydra->>Prep: prepare_cli_export_runtime() (new stage, disable RTX/physics)
Prep-->>Hydra: done
Hydra->>Export: export_rsl_rl_agent(args_cli, env_cfg, agent_cfg, simulation_app)
Export-->>Hydra: True/False
Hydra-->>CLI: exported bool
CLI->>App: simulation_app.close()
Reviews (1): Last reviewed commit: "moved imports to use the lazy import par..." | Re-trigger Greptile |
| 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 does not trigger Python's submodule import machinery, so if leapp.annotate is a submodule not explicitly re-exported in leapp/__init__.py, this call will raise AttributeError instead of the expected ImportError. Every other use of annotate across the codebase (utils.py, export_annotator.py, anymal_c_env.py) consistently uses from leapp import annotate, which does trigger the submodule import. This needs to match that pattern to be safe.
| annotate_module = getattr(leapp_module, "annotate") | |
| from leapp import annotate as annotate_module |
There was a problem hiding this comment.
not needed. leapp will always import annotate in the init. This pattern works when testing locally.
| def main_cli(argv: list[str] | None = None) -> bool: | ||
| """Run the command-line export flow.""" | ||
| args_cli, hydra_args = parse_export_args(argv) | ||
|
|
||
| app_launcher = AppLauncher(args_cli) | ||
| prelaunch_args = parse_prelaunch_args(argv) | ||
| app_launcher = AppLauncher(prelaunch_args) | ||
| simulation_app = app_launcher.app | ||
|
|
||
| try: | ||
| args_cli, hydra_args = parse_export_args(argv) | ||
| return run_export_with_hydra(args_cli, hydra_args, simulation_app) | ||
| finally: | ||
| simulation_app.close() |
There was a problem hiding this comment.
argv is parsed twice with different parsers. parse_prelaunch_args uses bare parse_known_args, while parse_export_args calls setup_preset_cli, which may expand preset tokens into additional arguments — including ones relevant to AppLauncher (e.g., --device, --num_envs). If a preset produces AppLauncher flags that are absent in prelaunch_args, the AppLauncher is initialized without those values, while the rest of the export logic uses the expanded args_cli. The split is intentional to break the import cycle, but the behaviour could silently diverge when presets affect launcher configuration.
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!
| from collections.abc import Mapping | ||
| from pathlib import Path | ||
|
|
||
| import torch |
There was a problem hiding this comment.
I think as long as torch is imported after AppLauncher it should work. is it possible to follow something similar to the launch_simulation context?
There was a problem hiding this comment.
updated. also removed a bunch of uneeded changes by the AI. right now it should only include the inport order change and using the simulation context managed environment.
There was a problem hiding this comment.
🤖 IsaacLab Review Bot
PR #5869: use lazy runtime imports to import leapp in export.py
Summary
This PR correctly addresses the import order issue introduced by #5633 by applying the lazy runtime imports pattern to export.py.
✅ What Looks Good
- Lazy import pattern applied correctly -
torch,leapp, andannotateare declared as globals and imported inside_load_runtime_dependencies() torch.jit._state.disable()repositioned - Now called after torch is imported within the runtime dependency loader- TYPE_CHECKING guard - Proper use of
if TYPE_CHECKINGfor type annotations (torch_type.device,torch_type.dtype) - Clean refactor to
launch_simulation- Simplifies CLI entry point by using the context manager pattern - Imports moved to point-of-use -
fold_preset_tokens,setup_preset_cli, andhydra_task_confignow imported where needed
📝 Minor Notes (Non-blocking)
- The
Anytype hint onhydra_task_config_fnis a reasonable workaround for the type checker - The
simulation_app=Nonedefault with the guard is a clean way to handle both CLI and test contexts
Verdict
LGTM ✅ - The lazy import pattern is correctly implemented and aligns with the codebase conventions established in #5633.
Update (a4579db): Code cleanup iteration - removes TYPE_CHECKING guard and torch type hints, moves fold_preset_tokens/setup_preset_cli to module-level imports, removes intermediate variable assignments (hydra_task_config_fn, main_fn), deletes unused prepare_cli_export_runtime() function. These simplifications improve readability while maintaining the core lazy import pattern for runtime dependencies. Still LGTM ✅
Automated review by IsaacLab Review Bot
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: 1. Parse minimal launcher args. 2. Start AppLauncher / Kit. 3. Import torch, leapp, RSL-RL, task utilities, and Hydra helpers. 4. Resolve task config. 5. Create the env with gym.make(). 6. Patch for LEAPP and export. ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there
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