fix(isaaclab_physx): only unsubscribe _on_stop to preserve warm_start callback

[preeves-nv]

Summary

Companion fix for #5358. Calling enable_all_default_callbacks(False) disables the warm_start (PLAY) callback in addition to _on_stop, breaking the rendering pipeline on sim.reset() — tiled-camera RGB output stays black and visualization markers fail.

warm_start is the PLAY-event callback in Isaac Sim's SimulationManager that initialises the rendering pipeline when simulation starts. Without it firing on sim.reset(), tiled cameras and visualization markers have no renderer to draw into.

Replace the blanket disable with a targeted unsubscribe of _default_callback_on_stop only — the sole callback that calls invalidate_physics() and wrecks the shared omni.physics.tensors view.

Expected CI impact

The following failures seen in #5358 are consistent with the warm_start callback being disabled:

• isaaclab (core) [1/3] — test_first_frame_textured_rendering (0/1), test_tiled_camera (6/8), test_multi_tiled_camera (6/11), test_visualization_markers (5/6)
• isaaclab_visualizers
• isaaclab_physx

The following may also be resolved but were not directly inspected:

• isaaclab_tasks [1/3], [2/3], [3/3]
• environments_training
• isaaclab (core) [3/3]

Test plan

• CI passes isaaclab_visualizers
• CI passes isaaclab (core) [1/3] camera tests
• CI passes isaaclab_physx

🤖 Generated with Claude Code

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR fixes a critical runtime error (Simulation view object is invalidated) that occurs after sim.reset() with recent Isaac Sim builds. The root cause is that Isaac Sim's SimulationManager._on_stop callback now correctly fires (after being fixed upstream with @staticmethod) and calls invalidate_physics(), which wrecks the shared omni.physics.tensors view that PhysxManager relies on. The fix selectively unsubscribes only the _on_stop callback while preserving the warm_start callback needed for rendering initialization.

Architecture Impact

High cross-module impact. This change touches the simulation lifecycle at the deepest level:

• isaaclab_physx/__init__.py: Patches Isaac Sim's SimulationManager at import time
• isaaclab/__init__.py: New _deprioritize_prebundle_paths() affects all imports system-wide
• isaaclab/app/app_launcher.py: Re-runs path sanitization after Kit startup
• isaaclab/cli/commands/install.py: Major additions for prebundle torch detection, pinocchio installation, and nvidia namespace handling
• All .kit files: Excludes additional conflicting extensions
• All PhysX asset/sensor files: Import path migration from omni.physics.tensors.impl.api to omni.physics.tensors.api

The changes are defensive and should be backward-compatible, but the monkey-patching in isaaclab_physx/__init__.py is inherently fragile if Isaac Sim's internal API changes.

Implementation Verdict

Minor fixes needed — The core fix is correct and well-reasoned, but there are a few concerns.

Test Coverage

Excellent. The PR adds 867 lines of new tests in test_install_commands.py and test_install_prebundle.py covering:

• Torch prebundle detection across environment types (uv, pip, conda, kit)
• Architecture-specific CUDA torch installation (x86 cu128 vs ARM cu130)
• Prebundle repointing with nvidia namespace special handling
• Edge cases: stale symlinks, missing backups, OSError recovery, Windows copy fallback

The tests are well-structured with parametrization and proper mocking. However, there's no direct integration test for the core SimulationManager patching fix itself — that relies on CI passing the previously-failing tests.

CI Status

Only labeler has completed (success). The actual test jobs haven't run yet, which are critical for validating this fix.

Findings

🔴 Critical: source/isaaclab_physx/isaaclab_physx/init.py:61-70 — Race condition in callback teardown

if hasattr(original_class, "_default_callback_on_stop"):
    original_class._default_callback_on_stop = None
else:
    try:
        original_class.enable_all_default_callbacks(False)
    except Exception:
        pass

The fallback to enable_all_default_callbacks(False) contradicts the PR's stated goal of preserving warm_start. If _default_callback_on_stop doesn't exist (API change), this silently disables all callbacks including the rendering pipeline. The comment acknowledges this but a silent pass on exception means broken rendering with no diagnostic. Should at minimum log a warning.

🟡 Warning: source/isaaclab_physx/isaaclab_physx/init.py:48-52 — Force-import may have side effects

try:
    import isaacsim.core.simulation_manager  # noqa: F401
except ImportError:
    return

Force-importing the module triggers its extension startup code which registers callbacks. This is intentional but creates an ordering dependency — if this import happens before Kit is fully initialized, it could fail or behave unexpectedly. The except ImportError is appropriate but doesn't catch RuntimeError or other Kit initialization errors.

🟡 Warning: source/isaaclab/isaaclab/init.py:52-54 — PYTHONPATH rewriting affects subprocesses

os.environ["PYTHONPATH"] = os.pathsep.join(env_clean + env_demoted)

This modifies the environment variable permanently for the process and all subprocesses. While necessary for consistency, this could cause unexpected behavior if a subprocess legitimately needs the original ordering. Consider documenting this side effect.

🟡 Warning: source/isaaclab/isaaclab/cli/commands/install.py:513-515 — nvidia cudnn check is brittle

if pkg_name == "nvidia" and not (venv_pkg / "cudnn").exists():
    print_debug(f"Skipping repoint of {prebundled}: {venv_pkg} lacks CUDA subpackages (cudnn missing).")
    continue

The cudnn subdirectory check assumes the torch CUDA wheel always creates this structure. If NVIDIA changes the package layout (e.g., nvidia_cudnn_cu12 vs nvidia/cudnn), this heuristic will fail silently. The test coverage is good but testing against the real package structure would be more robust.

🔵 Improvement: source/isaaclab_physx/isaaclab_physx/renderers/isaac_rtx_renderer.py:296-307 — Early return loses frame data silently

if tiled_data_buffer.size == 0:
    continue

When the annotator returns an empty buffer, the code now silently skips that data type. This is correct for preventing the CUDA crash, but downstream code may expect valid data. Consider logging at debug level when this happens so users can understand why the first frame(s) may have zero values.

🔵 Improvement: source/isaaclab/isaaclab/cli/commands/install.py:172-176 — Warning message could be actionable

print_warning(
    "Force-installing the cmeel pinocchio stack failed (returncode "
    f"{install_result.returncode}). The pink IK controller and its tests will not be"
    " usable until ``pin pin-pink==3.1.0 daqp==0.7.2`` is installed manually."
)

Good that it doesn't abort, but the warning should include the actual error output (install_result.stderr) to help users diagnose the failure.

🔵 Improvement: apps/*.kit files — Duplicated exclusion list
All .kit files now have identical app.extensions.excluded lists:

app.extensions.excluded = ["omni.kit.pip_archive", "omni.isaac.ml_archive", "isaacsim.pip.newton", "omni.warp.core"]

Consider extracting this to a shared include file to avoid drift. The isaaclab.python.headless.kit line 109 and others repeat this pattern.