Repository navigation
Conversation
Stop registering a Python handler for SIGABRT so fatal signals keep the default disposition, terminate cleanly, and can produce core dumps. SIGSEGV was already left unhandled; graceful teardown remains on SIGTERM. Fixes isaac-sim#7774 Signed-off-by: Dundy Pasupuleti <dundysm@gmail.com>
dundysm
requested review from
AntoineRichard,
StafaH,
fatimaanes,
kellyguo11,
ooctipus and
pbarejko
as code owners
September 12, 2026 20:58
Author
|
cc @hhansen-bdai @kellyguo11 @Mayankm96 @hujc7; this continues the fatal-signal policy from #6636 for #7774 (leave SIGSEGV/SIGABRT at SIG_DFL; keep graceful SIGTERM). complementary to #6531. |
github-actions
Bot
added
bug
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Isaac Lab Review Bot
The SIGABRT handler removal is internally consistent with the intended SIGTERM-only teardown policy, but the accompanying comment overstates both the resulting signal disposition and the rationale shared with SIGSEGV.
- Design and architecture: Narrowing graceful signal teardown to SIGTERM is coherent, and the existing re-entrancy and re-raise behavior remains intact. However, merely omitting SIGABRT/SIGSEGV registration leaves any previously installed disposition untouched rather than ensuring SIG_DFL.
- API: No public Python API changes. The user-visible signal behavior is documented in the changelog, but its claim that fatal signals remain at SIG_DFL should be revised to distinguish leaving dispositions untouched from explicitly restoring defaults.
- Implementation: The registration code and test consistently verify that AppLauncher does not install SIGSEGV or SIGABRT handlers. The nearby prose should say these signals are left untouched and should not apply the returning synchronous-SIGSEGV-handler rationale to SIGABRT, whose former handler explicitly restored and re-raised the default action.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
Signed-off-by: Dundy Pasupuleti <dundysm@gmail.com>
dundysm
changed the title
Author
|
addressed greptile spacing + reworded comments per review bot (left untouched vs SIG_DFL; SIGABRT rationale) |
6 tasks done
ooctipus
added a commit
that referenced
this pull request
Sep 28, 2026
…he config (1/2) (#8068) ## Description **Part 1 of 2: the design.** The migration of every script and test is in the stacked follow-up, #8090. This PR keeps existing scripts running unchanged, so the two halves can be reviewed separately. `launch_simulation` now starts each simulation runtime through the launcher that the runtime's own backend package provides. The resolved config says which launcher is needed, so core never imports one. ```python # isaaclab.app: the interface every backend runtime implements class SimulationLauncher: device: str | None = None # optional device the runtime picked def __init__(self, launcher_args): ... # constructing it starts the runtime def close(self, exit_code: int = 0): ... # stops it # backend configs name their launcher, resolved lazily like ``class_type`` class PhysxCfg(PhysicsCfg): launcher_type: ClassVar[str] = "isaaclab_physx.app:KitLauncher" class OVRTXRendererCfg(RendererCfg): launcher_type: ClassVar[str] = "isaaclab_ov.app:OvrtxLauncher" ``` `launch_simulation(cfg, args)` scans the resolved config as it already did and collects `launcher_type` from the physics and renderer config instances it resolved. It constructs each launcher once and closes them in reverse order when the block exits. Kit needs that no config names (`require_kit`, no physics config, a Kit visualizer requested only on the CLI) fall back to `KitLauncher`. - **`isaaclab_physx.app.KitLauncher`** is the old `AppLauncher` body, minus dead code: - removed: argument-collision bookkeeping, the Isaac Sim 5 branch, and the hidden `--cpu` flag; - moved in from `launch_simulation`: the Isaac Sim availability check and the "Kit already running" guard. - **`isaaclab_ov.app.OvrtxLauncher`** registers the OVRTX USD schemas, which `launch_simulation` used to do inline. - **Moved into `sim_launcher.py`** because kitless launches need them too: argv fusing, visualizer argument normalization, and the visualizer-settings sync (no longer wrapped in a blanket `suppress(Exception)`). ### Kit stays in `isaaclab_physx` Core code no longer imports `carb`, `omni`, or `omni.replicator`. Kit concerns go through one of two owners, and no new global hook is added. - **The `SimulationContext` backend registry** owns anything tied to a simulation's lifetime: - `isaaclab_physx.app.KitStageBackend` is registered through `get_or_create_backend` when Kit runs; - it attaches the stage to Kit's USD context and closes Kit's context stage when `clear_instance()` closes the other backends; - core stage utilities never touch Kit: `clear_stage()`/`close_stage()` are Kit-free, and Kit's own consumers (PhysX reset/step, RTX render, the Kit visualizer) run the app updates they need. - **No defensive `None` handling on invariants:** a `SimulationContext` always has a stage, so consumers read `SimulationContext.instance().stage` directly (`SceneDataProvider.usd_stage` / `get_usd_stage` are removed). - **`KitLauncher`** owns one-time process setup: - `SettingsManager.set_backend(carb.settings)`, replacing `initialize_carb_settings` and `is_omniverse_mode`; - sharing Isaac Sim's stage-helper context; - one app update in `close()`, so scripts no longer need their own. - **Replicator** is enabled by the Isaac RTX renderer and seeded by the Replicator event terms that use it. The env classes no longer import it. - **`show_stage_in_viewport`** moves to `isaaclab_physx.app`, since it only works under Kit. - **Gravity:** tasks set it through `PhysxManager.set_gravity` instead of `carb.Float3`. ### Exit handling The ~110-line `_SimulationAppLifecycle` class from #6636 is gone. I measured a real Kit process with no handlers installed, and only two exit paths needed help. Both are now handled by two lines in `KitLauncher`: - An unhandled exception exits with 1. Kit's exit path reported 0. - `SIGINT` raises `KeyboardInterrupt`. Kit's own handler exited 0. `SIGTERM`, `SIGABRT`, and `SIGSEGV` keep their default actions. Fixes #7774, supersedes #7775 and #6531. ### Constructing runtime objects after launch Importing `pxr.Usd` before Kit starts makes Kit fail to start. Scripts therefore import only config classes and construct runtime objects with `instantiate(cfg)` (#8085), which resolves the config's lazy `class_type` after the runtime is up. - `ManagerBasedEnvCfg` and `ManagerBasedRLEnvCfg` gain `class_type`. - The 21 task configs with a custom env class name it. A registry test checks that all 126 match their registered entry point. - **Fixed:** `{DIR}` in an inherited `class_type` resolved against the module of a subclass missing `@configclass`, so the call failed with `ModuleNotFoundError`. ### Compatibility in this PR `isaaclab.app.AppLauncher` is deprecated. It now starts Kit through `launch_simulation`, so develop's scripts and tests run unchanged here. It still offers `is_available`, `has_gui`, and `has_window`, which unmigrated scripts use. The migration PR moves every caller and reduces it to an init-only shim. `isaaclab.test.utils.launch_test_simulation(**launcher_args)` starts Kit for a test module through `launch_simulation`. It picks the test device by default. ### Behavior changes - **Removed** (breaking; each only restated an invariant or forwarded elsewhere): - `sim_utils.update_stage`: the simulation's reset, step, and render process stage changes; - `sim_utils.is_current_stage_in_memory`: compare against `SimulationContext.instance().stage`; - `sim_utils.show_stage_in_viewport`: use `isaaclab_physx.app.show_stage_in_viewport`; - `SettingsManager.initialize_carb_settings` (and the module-level function) and `is_omniverse_mode`: `KitLauncher` sets the settings backend, and `has_kit()` checks for Kit; - `SceneDataProvider.usd_stage` / `get_usd_stage`; - the hidden `--cpu` flag: use `--device cpu`. - **Replicator seeding:** the Replicator event terms seed it with `env.cfg.seed` when they are built; `_get_replicator()` only loads it. Calling `env.seed(x)` later no longer reseeds Replicator. - **Kit startup marker** is now `[ISAACLAB] KitLauncher initialization complete`, and `tools/conftest.py` is updated to match. - **SIGTERM** ends the process immediately, without a Kit teardown. - **Warp environments:** `render_mode="rgb_array"` now warns and returns `None`, matching the core environments. Net: +2,326 / −3,045 across 154 files. ## Type of change - Breaking change (`AppLauncher` deprecated; Kit-coupled helpers removed; Kit exit and seeding behavior changed) ## Validation All runs on the display GPU (GPU 0), with develop's scripts and tests unchanged unless listed here. - **Launcher, app, and config tests:** - `isaaclab/test/app` (26) and `isaaclab_physx/test/app` (88); - `test_runtime_compatibility` (27), visualizer intent (8), distributed device (13), `test_preset_kit_decision` (6), `test_preset_cli` (11); - `test_configclass` (37), `test_sb3_leapp_export`, and the CLI tests (234). - **Kit tests through the deprecated adapter:** - `test_simulation_context` (30), PhysX `test_rigid_object` (44), `test_camera` (21); - stage utils (15), stage in memory (3), `test_events` (4), texture randomization (2), `test_manager_based_env` (4). - **Training:** `train.py --task Isaac-Cartpole --max_iterations 2` exits 0 with `isaacsim_physx` and `newton_mjwarp`. - `physics=physx` (kitless ovphysx) fails with an observation batch mismatch. - Develop `93b6e7432` fails the same way, so it is unrelated to this PR. - **Every script under `scripts/` and `examples/`**, compared against develop with the same invocation: - **Scripts outside the standalone matrix (78):** same outcome on develop and here. 52 run, 19 are blocked by an external resource (datasets, XR, Ray, gr00t, robomimic, moviepy), and 7 fail identically on develop. - **Standalone matrix (361 cases):** no regressions. The only case that first looked new was an old-style script with `--visualizer newton_rtx`. Kit and OVRTX cannot share a process, so the adapter now skips that visualizer with a warning, as Kit did on develop. - **Newton-based visualizers next to Kit PhysX:** those cases fail on develop too, when Newton builds its model from USD. - **After rebasing onto the latest develop:** the state machines, `bin_packing`, and Cartpole training still run. - **Known pre-existing failure:** `test_visualizer_integration_newton` has a flat-frame check that fails identically on develop. - Pre-commit passes. The changelog check only flags fragments from other PRs, because it compares against the fork's stale `develop`. ## Release backport - [x] <!-- backport-active-release --> Backport this pull request to the active release branch after it merges into `develop` ## Checklist - [x] I have read and understood the contribution guidelines - [x] I have run the pre-commit checks - [x] I have made corresponding changes to the documentation (API pages for `isaaclab.app`, `isaaclab_physx.app`, `isaaclab_ov.app`) - [x] My changes generate no new warnings - [x] I have added changelog fragments for every touched package
Collaborator
isaaclab-bot Bot
pushed a commit
that referenced
this pull request
Sep 28, 2026
…he config (1/2) (#8068) ## Description **Part 1 of 2: the design.** The migration of every script and test is in the stacked follow-up, #8090. This PR keeps existing scripts running unchanged, so the two halves can be reviewed separately. `launch_simulation` now starts each simulation runtime through the launcher that the runtime's own backend package provides. The resolved config says which launcher is needed, so core never imports one. ```python # isaaclab.app: the interface every backend runtime implements class SimulationLauncher: device: str | None = None # optional device the runtime picked def __init__(self, launcher_args): ... # constructing it starts the runtime def close(self, exit_code: int = 0): ... # stops it # backend configs name their launcher, resolved lazily like ``class_type`` class PhysxCfg(PhysicsCfg): launcher_type: ClassVar[str] = "isaaclab_physx.app:KitLauncher" class OVRTXRendererCfg(RendererCfg): launcher_type: ClassVar[str] = "isaaclab_ov.app:OvrtxLauncher" ``` `launch_simulation(cfg, args)` scans the resolved config as it already did and collects `launcher_type` from the physics and renderer config instances it resolved. It constructs each launcher once and closes them in reverse order when the block exits. Kit needs that no config names (`require_kit`, no physics config, a Kit visualizer requested only on the CLI) fall back to `KitLauncher`. - **`isaaclab_physx.app.KitLauncher`** is the old `AppLauncher` body, minus dead code: - removed: argument-collision bookkeeping, the Isaac Sim 5 branch, and the hidden `--cpu` flag; - moved in from `launch_simulation`: the Isaac Sim availability check and the "Kit already running" guard. - **`isaaclab_ov.app.OvrtxLauncher`** registers the OVRTX USD schemas, which `launch_simulation` used to do inline. - **Moved into `sim_launcher.py`** because kitless launches need them too: argv fusing, visualizer argument normalization, and the visualizer-settings sync (no longer wrapped in a blanket `suppress(Exception)`). ### Kit stays in `isaaclab_physx` Core code no longer imports `carb`, `omni`, or `omni.replicator`. Kit concerns go through one of two owners, and no new global hook is added. - **The `SimulationContext` backend registry** owns anything tied to a simulation's lifetime: - `isaaclab_physx.app.KitStageBackend` is registered through `get_or_create_backend` when Kit runs; - it attaches the stage to Kit's USD context and closes Kit's context stage when `clear_instance()` closes the other backends; - core stage utilities never touch Kit: `clear_stage()`/`close_stage()` are Kit-free, and Kit's own consumers (PhysX reset/step, RTX render, the Kit visualizer) run the app updates they need. - **No defensive `None` handling on invariants:** a `SimulationContext` always has a stage, so consumers read `SimulationContext.instance().stage` directly (`SceneDataProvider.usd_stage` / `get_usd_stage` are removed). - **`KitLauncher`** owns one-time process setup: - `SettingsManager.set_backend(carb.settings)`, replacing `initialize_carb_settings` and `is_omniverse_mode`; - sharing Isaac Sim's stage-helper context; - one app update in `close()`, so scripts no longer need their own. - **Replicator** is enabled by the Isaac RTX renderer and seeded by the Replicator event terms that use it. The env classes no longer import it. - **`show_stage_in_viewport`** moves to `isaaclab_physx.app`, since it only works under Kit. - **Gravity:** tasks set it through `PhysxManager.set_gravity` instead of `carb.Float3`. ### Exit handling The ~110-line `_SimulationAppLifecycle` class from #6636 is gone. I measured a real Kit process with no handlers installed, and only two exit paths needed help. Both are now handled by two lines in `KitLauncher`: - An unhandled exception exits with 1. Kit's exit path reported 0. - `SIGINT` raises `KeyboardInterrupt`. Kit's own handler exited 0. `SIGTERM`, `SIGABRT`, and `SIGSEGV` keep their default actions. Fixes #7774, supersedes #7775 and #6531. ### Constructing runtime objects after launch Importing `pxr.Usd` before Kit starts makes Kit fail to start. Scripts therefore import only config classes and construct runtime objects with `instantiate(cfg)` (#8085), which resolves the config's lazy `class_type` after the runtime is up. - `ManagerBasedEnvCfg` and `ManagerBasedRLEnvCfg` gain `class_type`. - The 21 task configs with a custom env class name it. A registry test checks that all 126 match their registered entry point. - **Fixed:** `{DIR}` in an inherited `class_type` resolved against the module of a subclass missing `@configclass`, so the call failed with `ModuleNotFoundError`. ### Compatibility in this PR `isaaclab.app.AppLauncher` is deprecated. It now starts Kit through `launch_simulation`, so develop's scripts and tests run unchanged here. It still offers `is_available`, `has_gui`, and `has_window`, which unmigrated scripts use. The migration PR moves every caller and reduces it to an init-only shim. `isaaclab.test.utils.launch_test_simulation(**launcher_args)` starts Kit for a test module through `launch_simulation`. It picks the test device by default. ### Behavior changes - **Removed** (breaking; each only restated an invariant or forwarded elsewhere): - `sim_utils.update_stage`: the simulation's reset, step, and render process stage changes; - `sim_utils.is_current_stage_in_memory`: compare against `SimulationContext.instance().stage`; - `sim_utils.show_stage_in_viewport`: use `isaaclab_physx.app.show_stage_in_viewport`; - `SettingsManager.initialize_carb_settings` (and the module-level function) and `is_omniverse_mode`: `KitLauncher` sets the settings backend, and `has_kit()` checks for Kit; - `SceneDataProvider.usd_stage` / `get_usd_stage`; - the hidden `--cpu` flag: use `--device cpu`. - **Replicator seeding:** the Replicator event terms seed it with `env.cfg.seed` when they are built; `_get_replicator()` only loads it. Calling `env.seed(x)` later no longer reseeds Replicator. - **Kit startup marker** is now `[ISAACLAB] KitLauncher initialization complete`, and `tools/conftest.py` is updated to match. - **SIGTERM** ends the process immediately, without a Kit teardown. - **Warp environments:** `render_mode="rgb_array"` now warns and returns `None`, matching the core environments. Net: +2,326 / −3,045 across 154 files. ## Type of change - Breaking change (`AppLauncher` deprecated; Kit-coupled helpers removed; Kit exit and seeding behavior changed) ## Validation All runs on the display GPU (GPU 0), with develop's scripts and tests unchanged unless listed here. - **Launcher, app, and config tests:** - `isaaclab/test/app` (26) and `isaaclab_physx/test/app` (88); - `test_runtime_compatibility` (27), visualizer intent (8), distributed device (13), `test_preset_kit_decision` (6), `test_preset_cli` (11); - `test_configclass` (37), `test_sb3_leapp_export`, and the CLI tests (234). - **Kit tests through the deprecated adapter:** - `test_simulation_context` (30), PhysX `test_rigid_object` (44), `test_camera` (21); - stage utils (15), stage in memory (3), `test_events` (4), texture randomization (2), `test_manager_based_env` (4). - **Training:** `train.py --task Isaac-Cartpole --max_iterations 2` exits 0 with `isaacsim_physx` and `newton_mjwarp`. - `physics=physx` (kitless ovphysx) fails with an observation batch mismatch. - Develop `93b6e7432` fails the same way, so it is unrelated to this PR. - **Every script under `scripts/` and `examples/`**, compared against develop with the same invocation: - **Scripts outside the standalone matrix (78):** same outcome on develop and here. 52 run, 19 are blocked by an external resource (datasets, XR, Ray, gr00t, robomimic, moviepy), and 7 fail identically on develop. - **Standalone matrix (361 cases):** no regressions. The only case that first looked new was an old-style script with `--visualizer newton_rtx`. Kit and OVRTX cannot share a process, so the adapter now skips that visualizer with a warning, as Kit did on develop. - **Newton-based visualizers next to Kit PhysX:** those cases fail on develop too, when Newton builds its model from USD. - **After rebasing onto the latest develop:** the state machines, `bin_packing`, and Cartpole training still run. - **Known pre-existing failure:** `test_visualizer_integration_newton` has a flat-frame check that fails identically on develop. - Pre-commit passes. The changelog check only flags fragments from other PRs, because it compares against the fork's stale `develop`. ## Release backport - [x] <!-- backport-active-release --> Backport this pull request to the active release branch after it merges into `develop` ## Checklist - [x] I have read and understood the contribution guidelines - [x] I have run the pre-commit checks - [x] I have made corresponding changes to the documentation (API pages for `isaaclab.app`, `isaaclab_physx.app`, `isaaclab_ov.app`) - [x] My changes generate no new warnings - [x] I have added changelog fragments for every touched package (cherry picked from commit c9f9e22)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
AppLauncherpreviously registered a Python signal handler forSIGABRT(and historically for
SIGSEGV). returning from a PythonSIGSEGVhandlerre-executes the faulting instruction forever: the process spins, produces no
core dump, and cannot be stopped with
SIGTERM. the formerSIGABRThandlerrestored
SIG_DFLand re-raised rather than returning.on
develop,#6636already stopped interceptingSIGSEGV. this PR extendsthat fatal-signal policy to
SIGABRTas well: leave both untouched (defaultor carb's crash handler), and keep graceful teardown on
SIGTERMonly via_on_abort_signal. skippingsignal.signaldoes not installSIG_DFL.no
faulthandler.enable()call was added; the project already enablesfaulthandler for tests via
PYTHONFAULTHANDLERintools/conftest.py.related open PR
#6531only changesSIGTERMhandling and is complementary;it does not conflict with leaving fatal-signal dispositions untouched.
Fixes #7774
Type of change
Release backport
developValidation
Unit tests (kitless)
Changelog
Gaps
integration test in
test_app_launcher_exit_status.py(needs Kit/SimulationApp).ctypes.string_at(1)segfault under AppLauncher here;the unit test locks the registration policy that causes the hang.
main/release/3.0.0-beta2still use the pre-#6636signal registrationpattern; a clean backport of this diff targets
develop/release/3.0.0lifecycle code.
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/isaaclab/changelog.d/CONTRIBUTORS.mdor my name already exists there