Refactor sim launcher into a single config scan#5892
Conversation
ooctipus
requested review from
Mayankm96,
hhansen-bdai and
kellyguo11
as code owners
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR is a significant refactoring that consolidates the simulation launcher logic into a single-pass config scan. The core idea is sound: instead of multiple traversals patching over patches, one walk collects all needed signals (physics backend, renderer, camera requirements, visualizer intent) and applies --physics overrides in place.
What I Like
- Cleaner architecture: The
_Scandataclass approach is more maintainable than scattered helper functions - Breaking change documented: The changelog entries clearly document the removal of the compatibility shim
- Import cleanup: Moving canonical exports to
isaaclab.appmakes the API surface cleaner make_physics_cfgfactory: Nice addition for scripts that want to build configs programmatically- quadrupeds.py demo: Great that it now demonstrates both PhysX and Newton backends
Concerns & Questions
1. Type annotation inconsistency (minor)
def run_simulator(sim, entities: dict[str, Articulation], origins: torch.Tensor):The sim parameter lost its type annotation (sim_utils.SimulationContext). This is likely intentional since the import is deferred, but a TYPE_CHECKING import could preserve the annotation.
2. Potential edge case in _scan recursion
The visit function skips primitive types, but what about list or tuple containers? If a config has physics_cfgs: list[PhysicsCfg], those would be skipped since vars() would raise TypeError on the list. Is this intentional?
try:
children = vars(node)
except TypeError:
returnThe previous _scan_config had the same limitation, so this is not a regression, but worth noting.
3. Hardcoded backend names (design question)
_KITLESS_PHYSICS_CFGS = ("NewtonCfg", "OvPhysxCfg")Using string matching on class names is pragmatic but fragile. If someone subclasses NewtonCfg with a different name, they'd need to know about this list. The DeformableNewtonCfg comment acknowledges this, but it's a subtle contract.
Would a class attribute like is_kitless = True on the base configs be cleaner long-term?
4. Missing test for make_physics_cfg
The new make_physics_cfg function isn't directly tested. The existing tests exercise _scan with physics_str but not the standalone factory with invalid input (the ValueError path).
5. Visualizer loop termination in quadrupeds.py
while True:
if sim.visualizers and not any(v.is_running() and not v.is_closed for v in sim.visualizers):
breakThis assumes sim.visualizers is always defined. Is this guaranteed for all SimulationContext instances, or could this raise AttributeError when running headless without visualizers?
6. Changelog missing removed functions
The isaaclab_tasks changelog mentions removing add_launcher_args, launch_simulation, but the diff shows compute_kit_requirements, validate_runtime_compatibility, and apply_launcher_physics_override were also removed from the public API. Consider enumerating all removed symbols.
Test Plan Verification
The test plan items look reasonable. I'd suggest also running:
- A kitless Newton test without visualizer (
--visualizer none) - The distributed device resolution test with mocked multi-GPU environment
Nits
-
Line 431 in
sim_launcher.pyhas a bareexcept Exception:followed bytraceback.print_exc(). This is fine for debugging but might swallow context in production. Considerlogger.exception()instead. -
The
_get_arg/_set_arghelpers are clean, but the pattern of checkingisinstance(launcher_args, ...)twice (once in_get_arg, once in the caller) could be simplified with a small wrapper class.
Overall, this is a well-structured refactor that improves code organization. The breaking changes are clearly documented. Requesting changes mainly for the edge cases and test coverage mentioned above.
Greptile SummaryThis PR replaces the multi-pass
Confidence Score: 3/5The demo script has a NameError that fires at import time, an infinite loop in headless mode, and the core package now unconditionally imports all three physics backends — meaning a minimal installation missing any one of them breaks the entire isaaclab.app import chain. The core scan-and-launch logic is clean and the RL/benchmark import migrations are correct. However, the Articulation annotation in quadrupeds.py raises NameError at import time (no from future import annotations, TYPE_CHECKING-only import), the while True loop has no exit in headless mode (directly contradicting the --headless test plan item), and moving unconditional physics-backend imports to the core package could silently break environments missing any one of the three optional backends. scripts/demos/quadrupeds.py (NameError, infinite loop, is_closed inconsistency) and source/isaaclab/isaaclab/app/sim_launcher.py (unconditional top-level backend imports) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["launch_simulation(env_cfg, launcher_args)"] --> B["_scan(env_cfg, --physics override)"]
B --> C{_Scan}
C --> D["physics_cfgs\nhas_ovrtx\nhas_kit_camera\nvisualizer_intent"]
A --> E["_validate_runtime(scan, launcher_args)"]
E --> F{needs_kit?}
F -- "has_kit_camera OR no kitless physics\nOR --visualizer kit" --> G["_ensure_isaac_sim_available()\nAppLauncher(launcher_args)"]
F -- "kitless path" --> H["sync_visualizer_cli_settings_to_carb\n(if visualizer flags present)"]
G --> I["yield physics_cfg"]
H --> I
I --> J["SimulationContext(SimulationCfg(physics=physics_cfg))"]
J --> K["run loop"]
K --> L["finally: app.close()"]
Reviews (1): Last reviewed commit: "add string guard" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
|
|
||
| def run_simulator(sim: sim_utils.SimulationContext, entities: dict[str, Articulation], origins: torch.Tensor): | ||
| def run_simulator(sim, entities: dict[str, "Articulation"], origins: torch.Tensor): |
There was a problem hiding this comment.
Articulation annotation causes NameError at runtime
Articulation is imported only under TYPE_CHECKING (line 48–49), which is False at runtime. Without from __future__ import annotations, Python evaluates function annotations eagerly when the def statement executes, so dict[str, Articulation] immediately raises NameError: name 'Articulation' is not defined the moment this module is imported. Either add from __future__ import annotations at the top of the file, or quote the annotation as "dict[str, Articulation]".
| from isaaclab_newton.physics import NewtonCfg | ||
| from isaaclab_ovphysx.physics import OvPhysxCfg | ||
| from isaaclab_physx.physics import PhysxCfg |
There was a problem hiding this comment.
NewtonCfg, OvPhysxCfg, and PhysxCfg are imported at module load time from isaaclab_newton, isaaclab_ovphysx, and isaaclab_physx. This means any from isaaclab.app import launch_simulation call fails immediately if any backend package is not installed — even in a PhysX-only environment where Newton was never needed. The rest of the module uses string-name matching (_KITLESS_PHYSICS_CFGS) precisely to avoid this hard dependency; only make_physics_cfg actually needs the class objects. Moving these three imports inside make_physics_cfg would restore the lazy-loading behaviour and eliminate the hard dependency on optional backends for the core package.
| while True: | ||
| # exit when every visualizer window has been closed (works for kit and newton) | ||
| if sim.visualizers and not any(v.is_running() and not v.is_closed for v in sim.visualizers): | ||
| break |
There was a problem hiding this comment.
The only exit point from while True: is if sim.visualizers and not any(...). When --headless is passed (as in the test plan), sim.visualizers is empty/None, so the outer and short-circuits and the loop runs forever. The old while simulation_app.is_running(): guard handled termination via Kit's lifecycle. The new code needs an equivalent: a step counter, a SIGINT handler, or checking sim.is_running() if that exists.
| while simulation_app.is_running(): | ||
| while True: | ||
| # exit when every visualizer window has been closed (works for kit and newton) | ||
| if sim.visualizers and not any(v.is_running() and not v.is_closed for v in sim.visualizers): |
There was a problem hiding this comment.
v.is_closed is accessed as an attribute while v.is_running() is called as a method. If is_closed is a callable rather than a property, the expression not v.is_closed always evaluates to True (a bound method object is truthy), so the break condition never fires even when every window is closed. Calling it consistently with () guards against this.
| if sim.visualizers and not any(v.is_running() and not v.is_closed for v in sim.visualizers): | |
| if sim.visualizers and not any(v.is_running() and not v.is_closed() for v in sim.visualizers): |
There was a problem hiding this comment.
🔄 Updated Review (commit 1704a47)
Latest commit moves import isaaclab.sim as sim_utils from local function scope to module level in quadrupeds.py and removes the unused --visualizer CLI argument.
What changed since c29c2da
| Change | Assessment |
|---|---|
import isaaclab.sim as sim_utils moved to module level |
✅ Good practice — eliminates repeated import overhead per function call. Import was previously duplicated inside design_scene() and main(). |
Removed --visualizer argument from argparse |
✅ Correct — this argument was defined but never consumed by the script. Likely dead code from an earlier iteration. |
Previous findings — status update
| # | Finding | Status |
|---|---|---|
| 🟡 1 | needs_kit=True when no physics config exists |
Addressed (earlier commit) |
| 🟡 2 | scan does not traverse list/tuple/dict containers |
Still applicable |
| 🟡 3 | Test plan references non-existent test file | Still applicable |
| 🔵 4 | _is_kit_camera imports from isaaclab_tasks |
Still applicable |
| ✅ 5 | Headless infinite loop in quadrupeds.py |
Fixed (commit 767747d) |
| 🔵 6 | launch_simulation yield type change |
Still applicable |
Summary of all reviewed commits (since 767e53a)
68ad5cf— Renameenv_cfg→cfgparameter across scan/launch APIs57333b1— Migrate imports fromisaaclab_tasks.utilstoisaaclab.app767747d— Fix headless infinite loop inquadrupeds.pyc29c2da— Convert section comments to triple-quoted banners1704a47— Remove local imports, delete unused--visualizerarg
Verdict: Clean incremental cleanup. Module-level import is appropriate given the unconditional use of sim_utils throughout the script, and removing dead CLI arguments improves maintainability. No new issues introduced.
Update (bb9ae16): Large restructuring commit — tasks refactored into flat core/contrib layout, sim_launcher.py moved from isaaclab_tasks.utils to isaaclab.app, deferred pxr imports, PhysX tensor import path fixes (omni.physics.tensors.api → omni.physics.tensors), new resample_interval_on_reset feature, and removal of deprecated environments.
Inline comment status:
- ✅
ArticulationNameError (P1,quadrupeds.pyL135): Fixed — annotation is now quoted as"Articulation" - ⏸️ Unconditional top-level backend imports in
sim_launcher.py(P1): Not addressed in this commit - ⏸️ Headless infinite loop in
quadrupeds.py(P1): Not addressed — loop condition still has no fallback for emptysim.visualizers - ⏸️
v.is_closedattribute vs method call (P2): Not addressed
No new issues found in this commit. The refactoring is mechanical and correct — all renamed import paths are consistent, and the deferred pxr imports properly use TYPE_CHECKING guards with runtime-local imports where needed.
Update (bb9ae16): Large restructuring commit — tasks refactored into flat core/contrib layout, sim_launcher.py moved from isaaclab_tasks.utils to isaaclab.app, deferred pxr imports, PhysX tensor import path fixes (omni.physics.tensors.api → omni.physics.tensors), new resample_interval_on_reset feature, and removal of deprecated environments.
Inline comment status:
- ✅
ArticulationNameError (P1,quadrupeds.pyL135): Fixed — annotation is now quoted as"Articulation" - ⏸️ Unconditional top-level backend imports in
sim_launcher.py(P1): Not addressed in this commit - ⏸️ Headless infinite loop in
quadrupeds.py(P1): Not addressed — loop condition still has no fallback for emptysim.visualizers - ⏸️
v.is_closedattribute vs method call (P2): Not addressed
No new issues found in this commit. The refactoring is mechanical and correct — all renamed import paths are consistent, and the deferred pxr imports properly use TYPE_CHECKING guards with runtime-local imports where needed.
| count = 0 | ||
| # Simulate physics | ||
| while simulation_app.is_running(): | ||
| while True: |
There was a problem hiding this comment.
Can we put sim.visualizers and not any(v.is_running() and not v.is_closed for v in sim.visualizers) here?
AntoineRichard
left a comment
AntoineRichard
left a comment
There was a problem hiding this comment.
It looks ok, should this also cover the leapp export script?
| raise ValueError(f"Invalid physics config: {physics_cfg_str!r} (expected 'physx', 'newton_mjwarp', or 'ovphysx').") | ||
|
|
||
|
|
||
| # -- node predicates --------------------------------------------------------- |
There was a problem hiding this comment.
In general this type of comment is not homogeneous across our code base. Sometimes we use:
# -- node predicates ---------------------------------------------------------Sometimes:
"""
Node Predicates
"""| return self.has_kit_camera or not self.has_kitless_physics | ||
|
|
||
|
|
||
| def _scan(env_cfg, physics_str: str | None = None) -> _Scan: |
There was a problem hiding this comment.
Should this be public?
|
|
||
|
|
||
| @dataclass | ||
| class _Scan: |
There was a problem hiding this comment.
Should this be public?
| @dataclass | ||
| class _Scan: | ||
| """Signals gathered from one walk of the config tree (see :func:`_scan`).""" | ||
|
|
||
| physics_cfgs: list[PhysicsCfg] # physics configs in walk order (post --physics override) | ||
| has_ovrtx: bool | ||
| has_kit_camera: bool | ||
| visualizer_intent: dict[str, bool] | ||
| effective_cfg: Any # env_cfg, or the new physics config when env_cfg was an overridden root | ||
| physics_cfg: PhysicsCfg | None # config produced by the --physics override, else None | ||
|
|
||
| @property | ||
| def resolved_physics_cfg(self) -> PhysicsCfg | None: | ||
| return self.physics_cfgs[0] if self.physics_cfgs else None | ||
|
|
||
| @property | ||
| def has_kit_physics(self) -> bool: | ||
| return any(type(cfg).__name__ == "PhysxCfg" for cfg in self.physics_cfgs) | ||
|
|
||
| @property | ||
| def has_kitless_physics(self) -> bool: | ||
| return any(type(cfg).__name__ in _KITLESS_PHYSICS_CFGS for cfg in self.physics_cfgs) | ||
|
|
||
| @property | ||
| def has_ovphysx_physics(self) -> bool: | ||
| return any(type(cfg).__name__ == "OvPhysxCfg" for cfg in self.physics_cfgs) | ||
|
|
||
| @property | ||
| def needs_kit(self) -> bool: | ||
| """Whether the config requires Kit: a Kit-renderer camera or non-kitless physics. | ||
|
|
||
| The launcher additionally forces Kit when ``--visualizer kit`` is requested. | ||
| """ | ||
| return self.has_kit_camera or not self.has_kitless_physics |
There was a problem hiding this comment.
We're doing a dataclass already, does it make sense to add properties? Are those dynamic? Everything looks static to me I'm not sure we need to define these as properties vs variables.
| # Origin 1 with Anymal B | ||
| sim_utils.create_prim("/World/Origin1", "Xform", translation=origins[0]) | ||
| # -- Robot | ||
| anymal_b = Articulation(ANYMAL_B_CFG.replace(prim_path="/World/Origin1/Robot")) |
There was a problem hiding this comment.
Can we move back to Articulation by importing the class in this function?
There was a problem hiding this comment.
We can, tho generally the recommended practice is to not import anything within function. That rule was strict prior IsaacLab3.0, and we relaxed it a bit due to all the dependency issues. But I think it will be good to slowly resolve all the dependency issue instead of accepting local import is ok.
|
|
||
|
|
||
| def run_simulator(sim: sim_utils.SimulationContext, entities: dict[str, Articulation], origins: torch.Tensor): | ||
| def run_simulator(sim, entities: dict[str, "Articulation"], origins: torch.Tensor): |
There was a problem hiding this comment.
Can we keep the hints by TYPE_CHECKING?
There was a problem hiding this comment.
I think its already type checked
github-actions
Bot
added
the
documentation
Address review feedback: the derived _Scan values are static once the single walk completes, so compute them as plain fields in _scan instead of properties. Also drop the unused physics_cfg field, which was set but never read. Update the two tests that forced the kitless path by patching the old needs_kit property to instead wrap _scan and set the now-plain field.
Promote the config-tree walk (_scan) and its result (_Scan) to public symbols scan and Scan, exported from isaaclab.app, so callers can inspect launch signals without launching. Rename the launch_simulation local to config_scan to avoid shadowing the new scan function, repoint tests to the public names, and document the public sim_launcher API.
The scan and launch_simulation entry points accept any config object (a bare PhysicsCfg, an env cfg, etc.), so the env_cfg parameter name was misleadingly narrow. Rename it to the generic cfg throughout sim_launcher and update the one keyword caller in the quadrupeds demo.
The play_<lib>, play, train, and random/zero agent entry points still imported add_launcher_args and launch_simulation from isaaclab_tasks.utils, which broke after the sim_launcher shim was removed (ImportError at startup, caught by the typed-preset CLI test). Import them from isaaclab.app instead and fix the stale cross-reference in preset_cli.
Replace the while True loop and if/break with a loop condition that steps while a visualizer window is open (or none exist, e.g. headless), per review feedback. Behavior is unchanged.
Switch the in-file section dividers from the ad-hoc # -- ... --- comments to the triple-quoted banner style used across the rest of the codebase, per review feedback.
ooctipus
force-pushed
the
octi/sim-launcher-single-scan
branch
from
1704a47 to
f0417fb
Compare
3 participants
Summary
isaaclab.app.sim_launcherso launch decisions come from a single pass over the env config. One_scantraversal collects the physics backend, OVRTX renderer presence, Kit-camera requirements, and visualizer intent, and applies the--physicsCLI override in place, replacing the previous multi-walk "patches over patches" flow.launch_simulationnow accepts a any cfg asenv_cfg, resolves it against the launcher args, and yields the resolved config forSimulationContext. This makes partial cfg override and simulation launch convinient.isaaclab.app, and madescripts/demos/quadrupeds.pyusable with both the Kit (PhysX) and kit-less Newton (MJWarp) backends.Test plan
./isaaclab.sh -p -m pytest source/isaaclab/test/app/test_sim_launcher_physics_override.py./isaaclab.sh -p -m pytest source/isaaclab_tasks/test/test_sim_launcher_visualizer_intent.py source/isaaclab_tasks/test/test_distributed_device_resolution.py source/isaaclab_tasks/test/test_preset_kit_decision.py source/isaaclab_tasks/test/test_runtime_compatibility.py./isaaclab.sh -p scripts/demos/quadrupeds.py --physics newton_mjwarp --headless./isaaclab.sh -f(pre-commit) passes