Remove obsolete OVPhysX bootstrap hacks

[marcodiiga]

Description

Removes OVPhysX compatibility paths that are no longer needed for the ovphysx 0.5 runtime:

• stops hiding already-loaded pxr modules around ovphysx.bootstrap();
• removes the forced atexit process-exit shutdown workaround and releases the ovphysx.PhysX instance directly;
• constructs OVPhysX through the 0.5-style PhysXConfig / carbonite_overrides path;
• updates the optional runtime dependency to ovphysx>=0.5,<0.6;
• updates kitless interface-test setup and adds focused guardrail tests for the removed bootstrap/shutdown behavior.

Dependency: ovphysx 0.5.

Fixes N/A

Type of change

• Bug fix (non-breaking change which fixes an issue)

Screenshots

N/A

Test Plan

• ./isaaclab.sh -f
• python tools/changelog/cli.py check develop
• git diff --check origin/develop..HEAD && git diff --check
• Focused pytest for the lifecycle guardrails and shared interface suites: 2085 passed, 51 skipped
• Local ovphysx 0.5 development-wheel smoke tests with presets=ovphysx, no LD_PRELOAD:
  • Isaac-Cartpole-Direct, 64 envs, 2 RSL-RL iterations
  • Isaac-Ant-Direct, 16 envs, 2 RSL-RL iterations
  • Isaac-Humanoid-Direct, 16 envs, 2 RSL-RL iterations
• OVPhysX backend tests with the same ovphysx 0.5 development wheel:
  • non-device unit tests: 16 passed
  • GPU real-backend selection: 241 passed, 6 skipped, 16 xfailed
  • CPU real-backend selection: 242 passed, 4 skipped, 18 xfailed

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have added a changelog fragment under source/<pkg>/changelog.d/ for every touched package (do not edit CHANGELOG.rst or bump extension.toml — CI handles that)
• I have added my name to the CONTRIBUTORS.md or my name already exists there

Note: the targeted OVPhysX backend pytest runs still emit existing deprecation/runtime warnings from surrounding test coverage.

[isaaclab-review-bot]

Summary

This PR removes obsolete compatibility hacks from the OVPhysX integration that were necessary for ovphysx 0.4.x but are no longer needed with ovphysx 0.5's namespaced USD and static Carbonite lifecycle. The changes are well-structured and the test coverage for preventing regression of the removed behavior is thorough.

Positive Observations

1. Clean removal of workarounds: The pxr module hiding during bootstrap, the atexit -> os._exit(0) process-exit workaround, and the physx.reset() keeping-instance-alive pattern are all cleanly removed.

2. Proper lifecycle management: The new physx.release() call in _release_physx() is cleaner than the previous reset-and-hold pattern.

3. Excellent guardrail tests: The new test_ovphysx_manager_lifecycle.py explicitly tests that the old behaviors stay removed, which is great defensive programming.

4. Consistent test file updates: All test files consistently update the kitless detection logic and documentation references from ./scripts/run_ovphysx.sh to ./isaaclab.sh -p -m pytest.

5. Clear changelog entries: Both the .skip and .minor.rst changelog fragments clearly communicate what changed.

Questions and Suggestions

ovphysx_manager.py

1. Line ~609: The _construct_physx method now unconditionally passes num_threads=8 in PhysXConfig. Was this value validated against ovphysx 0.5's recommended defaults? The old code set this via set_config_int32 or set_setting dynamically.

2. Lines ~595-607: The carbonite_overrides dictionary now includes /physics/physxDispatcher, /physics/updateToUsd, etc. unconditionally. Previously these were set conditionally via set_setting(). Is there a reason these are now always set even for CPU mode, or is this intentional?

3. Potential race condition consideration: With the removal of the _atexit_registered guard, if _construct_physx were somehow called twice (shouldn't happen given the design, but defensively), there's no longer a sentinel preventing duplicate work. The existing _physx is None check in _warmup_and_load guards this, but it might be worth adding a comment noting the single-construction guarantee.

test_ovphysx_manager_lifecycle.py

4. Line 82: The _FakePhysX.release() method sets self.released = True, which is good. Consider adding an assertion that release() wasn't called before (to catch double-release bugs), though this may be out of scope for this PR.

5. The fake module fixture restores _physx and _locked_device but not _warmup_done, _stage_path, or other class variables. If any future test accidentally modifies these, it could leak between tests. This is minor since the current tests don't touch them.

pyproject.toml

6. Line 26: The version constraint ovphysx>=0.5,<0.6 is good practice. However, consider whether ~=0.5 (compatible release) would be more appropriate if ovphysx follows semver strictly, though the explicit range is clearer.

Test Interface Files

7. The simplification of _kitless detection from the complex LD_PRELOAD-based check to just "EXP_PATH" not in os.environ is clean. Just confirming: are there any edge cases where EXP_PATH might be set but Kit is still not available? The original check seemed to guard against that.

Minor Nits

• In contact_sensor.py, the updated error message now reads "The IsaacLab OVPhysX adapter has not wired detailed contact/friction reads" — this is clear but slightly verbose. Consider shortening to "The OVPhysX adapter does not yet support" for consistency with similar messages.

Overall this is a clean, well-tested PR that removes technical debt. The test plan is comprehensive and the changes align well with the ovphysx 0.5 API surface.

[greptile-apps]

Greptile Summary

This PR removes all OVPhysX compatibility hacks that were required for the pre-0.5 runtime: hiding host pxr modules from sys.modules during ovphysx.bootstrap(), forcing process exit via an atexit-registered os._exit(0) to dodge dual-Carbonite static-destructor races, runtime API detection via inspect.signature, and keeping the ovphysx.PhysX reference alive indefinitely to avoid mid-process destructor races. The 0.5 runtime owns namespaced USD and static Carbonite itself, so _release_physx() now calls physx.release() and clears the reference directly.

• _construct_physx() is rewritten to use the 0.5-style PhysXConfig(num_threads=..., carbonite_overrides={...}) path; the GPU suppressReadback/suppressFabricUpdate overrides are included only when ovphysx_device == \"gpu\".
• Kitless detection in the three shared interface test files is simplified from an LD_PRELOAD-plus-EXP_PATH heuristic to a single \"EXP_PATH\" not in os.environ check, and the PhysxManager mock is moved inside the conditional try block to prevent a NameError when isaaclab_physx is not installed.
• A new test_ovphysx_manager_lifecycle.py file adds three focused guardrail tests (using a fake ovphysx module) to prevent reintroduction of the removed hacks.

Confidence Score: 4/5

Safe to merge; the changes remove well-documented temporary workarounds and the new PhysXConfig construction path is confirmed working by smoke-test runs against the target ovphysx 0.5 wheel.

The core manager rewrite is clean and well-tested by both the new lifecycle guardrail tests and the smoke runs. The one thing worth a second look is the new lifecycle test fixture, which only saves and restores _physx and _locked_device while leaving the other class-level fields (_warmup_done, _pending_clones, _usd_handle, etc.) unguarded. This is benign for the three current tests but would silently cause state leakage if future tests exercise a wider slice of the lifecycle through the same fixture.

source/isaaclab_ovphysx/test/physics/test_ovphysx_manager_lifecycle.py — the fixture teardown is incomplete relative to the full set of class-level state it could disturb.

Important Files Changed

Filename	Overview
source/isaaclab_ovphysx/isaaclab_ovphysx/physics/ovphysx_manager.py	Removes all temporary hacks (pxr hiding, atexit os._exit workaround, inspect.signature version detection); constructs PhysX via the clean 0.5-style PhysXConfig/carbonite_overrides path; _release_physx() now calls physx.release() and clears _physx instead of soft-resetting and keeping the instance alive.
source/isaaclab_ovphysx/test/physics/test_ovphysx_manager_lifecycle.py	New guardrail test file verifying that pxr hiding, atexit os._exit, and the old reset-and-keep pattern are gone; fixture saves only _physx and _locked_device, leaving other ClassVar fields unrestored — harmless for current tests but fragile for future additions.
source/isaaclab/test/assets/test_articulation_iface.py	Simplifies kitless detection from LD_PRELOAD-based heuristic to pure EXP_PATH check; moves PhysxManager mock patch inside the isaaclab_physx try block so it no longer causes a NameError when that package is absent.
source/isaaclab/test/assets/test_rigid_object_iface.py	Same kitless and mock-patch cleanup as test_articulation_iface.py — consistent and correct.
source/isaaclab/test/assets/test_rigid_object_collection_iface.py	Same kitless and mock-patch cleanup as the other two iface test files — consistent and correct.
source/isaaclab_ovphysx/pyproject.toml	Bumps optional ovphysx dependency from the pinned ==0.4.13 to the >=0.5,<0.6 range, matching the new API surface used throughout this PR.

Sequence Diagram

sequenceDiagram
    participant SC as SimulationContext
    participant OM as OvPhysxManager
    participant OVP as ovphysx.PhysX

    SC->>OM: initialize()
    Note over OM: _warmup_done=False, _physx unchanged

    SC->>OM: reset()
    OM->>OM: _warmup_and_load()
    alt _physx is None (first call or after close)
        OM->>OVP: bootstrap()
        OM->>OVP: "PhysX(device, config=PhysXConfig(...))"
        OVP-->>OM: physx instance
        Note over OM: _locked_device set
    else reuse path (_physx still alive)
        OM->>OVP: physx.reset() + wait_op()
    end
    OM->>OVP: add_usd(stage_file)
    OVP-->>OM: usd_handle
    OM->>OVP: warmup_gpu() [GPU only]

    SC->>OM: step() [repeated]
    OM->>OVP: step_sync(dt) + update_articulations_kinematic()

    SC->>OM: close()
    OM->>OM: _release_physx()
    OM->>OVP: physx.release()
    Note over OM: _physx=None, _locked_device survives

Loading

_{Reviews (1): Last reviewed commit: "Remove obsolete ovphysx bootstrap hacks" | Re-trigger Greptile}

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot — PR #6074

Verdict: Clean removal of technical debt with good guardrail coverage.

Summary

This PR removes OVPhysX 0.4.x compatibility workarounds (pxr-hiding during bootstrap, atexit → os._exit(0) shutdown hack, reset-and-hold instance pattern) and migrates to ovphysx 0.5's PhysXConfig/carbonite_overrides construction API. The changes are well-scoped, the new lifecycle test file prevents regression of the removed workarounds, and test documentation consistently updates references from the deprecated run_ovphysx.sh wrapper to ./isaaclab.sh -p -m pytest.

---

Findings

🟡 Warning: CI license-check and pre-commit are currently failing

The license-check and pre-commit jobs are failing on this SHA. These may be infrastructure issues, but worth verifying — in particular the new test_ovphysx_manager_lifecycle.py file should pass the license header check and any formatting lints.

🟡 Warning: No error handling around physx.release() in _release_physx

File: source/isaaclab_ovphysx/isaaclab_ovphysx/physics/ovphysx_manager.py

physx = cls._physx
cls._physx = None
physx.release()

If physx.release() raises (e.g., the C++ layer encounters an error during teardown), the exception will propagate up through close(). The reference-clearing before release is good defensive practice, but a try/finally or logging around release() would make the failure mode more graceful — especially since this runs during cleanup paths where swallowing or logging errors is often preferable to propagating them.

🟡 Note: _warmup_and_load reuse path narrowed to within-session only

File: ovphysx_manager.py

The reuse path (else branch when cls._physx is not None) in _warmup_and_load calls cls._physx.reset(). Previously this was always reachable because _release_physx kept the instance alive. Now that close() → _release_physx() calls physx.release() and sets _physx = None, the reuse path is only exercised within a single SimulationContext lifetime (i.e., multiple reset() calls before close()). The docstring correctly states "On subsequent calls before close" — just flagging this behavioral narrowing for visibility.

🔵 Suggestion: Document why num_threads=8 is hardcoded

File: ovphysx_manager.py → _construct_physx

num_threads=8 is now a construction-time constant in PhysXConfig. Previously it was set via set_config_int32 or set_setting. If this value is an ovphysx 0.5 API constraint, a brief comment or exposing it as an OvPhysxCfg field would help future maintainers understand the choice and know whether it can be tuned.

🔵 Suggestion: Consider a lifecycle re-construction test

File: test_ovphysx_manager_lifecycle.py

A fourth test verifying _construct_physx succeeds after _release_physx (the close → reinitialize → construct flow) would confirm the new lifecycle's re-entrancy. Optional since real-backend tests cover it, but cheap to add as a guardrail test.

---

Positive Observations

• ✅ Guardrail tests explicitly verify the old hacks stay removed — excellent defensive testing pattern
• ✅ _kitless detection simplification ("EXP_PATH" not in os.environ) is correct and backwards-compatible
• ✅ Moving PhysxManager mock patching inside the try/except ImportError block in interface tests prevents mock leaks when the backend is not installed
• ✅ Clear changelog fragments for both packages (.skip for isaaclab, .minor.rst for isaaclab_ovphysx)
• ✅ Version constraint ovphysx>=0.5,<0.6 is appropriately bounded
• ✅ The _release_physx clearing pattern (save ref → null field → release) prevents concurrent access during teardown

Reviewed at SHA: d9056d5

---

Update (48307ea): Incremental changes reviewed. ✅ Fixed: Test fixture in test_ovphysx_manager_lifecycle.py now properly saves/restores all class-level state fields via _MANAGER_STATE_FIELDS tuple (addressing the earlier inline comment about incomplete fixture state). No new issues introduced in this push — changes are docstring/comment refinements and the fixture improvement. LGTM.

[marcodiiga]

Stalling this PR for now and marking it as draft. The cleanup intentionally targets the next ovphysx line via ovphysx>=0.5,<0.6, but the public package indexes currently only resolve ovphysx<=0.4.13. That makes dependency-resolution CI fail before this can be mergeable.

We should mark this ready again only after a public Python ovphysx wheel satisfying >=0.5,<0.6 is available on the configured indexes, then rerun CI and the existing smoke matrix.