Address OVPhysX compatibility review feedback

Author: kellyguo11

source/isaaclab_ov/isaaclab_ov/physics/ovphysx_compat.py

@@ -26,8 +26,8 @@
 
 logger = logging.getLogger(__name__)
 
-# First OVPhysX version that uses the current lifecycle entry points.
-_CURRENT_LIFECYCLE_VERSION = Version("0.6")
+# First OVPhysX release line that uses the current lifecycle entry points.
+_CURRENT_LIFECYCLE_RELEASE = (0, 6)
 
 
 def detect_ovphysx_version() -> Version | None:
@@ -62,7 +62,7 @@ def uses_current_lifecycle_api(version: Version | None) -> bool:
     Returns:
         Whether ``version`` is OVPhysX 0.6 or newer.
     """
-    return version is not None and version >= _CURRENT_LIFECYCLE_VERSION
+    return version is not None and version.release[:2] >= _CURRENT_LIFECYCLE_RELEASE
 
 
 def build_lifecycle_entry_points(version: Version | None) -> Mapping[str, str]:

source/isaaclab_ov/test/physics/test_ovphysx_compat.py

@@ -29,13 +29,14 @@
         OVPHYSX_LIFECYCLE_ENTRY_POINTS,
         build_lifecycle_entry_points,
         detect_ovphysx_version,
-        uses_current_lifecycle_api,
     )
 else:
     OVPHYSX_LIFECYCLE_ENTRY_POINTS = None
     build_lifecycle_entry_points = None
     detect_ovphysx_version = None
-    uses_current_lifecycle_api = None
+
+_LEGACY_ENTRY_POINTS = {"warmup": "warmup_gpu", "destroy": "release"}
+_CURRENT_ENTRY_POINTS = {"warmup": "warmup", "destroy": "destroy"}
 
 
 def test_detect_ovphysx_version_reads_distribution_metadata(monkeypatch: pytest.MonkeyPatch):
@@ -59,31 +60,16 @@ def test_detect_ovphysx_version_returns_none_for_unparseable_version(monkeypatch
 @pytest.mark.parametrize(
     ("version", "expected"),
     [
-        (None, False),
-        (Version("0.5.11"), False),
-        (Version("0.5.99"), False),
-        (Version("0.6"), True),
-        (Version("0.6.0+trunk.e15a64a2"), True),
-        (Version("1.0"), True),
+        (None, _LEGACY_ENTRY_POINTS),
+        (Version("0.5.11"), _LEGACY_ENTRY_POINTS),
+        (Version("0.6.0.dev1+trunk.e15a64a2"), _CURRENT_ENTRY_POINTS),
+        (Version("0.6"), _CURRENT_ENTRY_POINTS),
+        (Version("1.0"), _CURRENT_ENTRY_POINTS),
     ],
 )
-def test_uses_current_lifecycle_api_switches_at_ovphysx_06(version: Version | None, expected: bool):
-    assert uses_current_lifecycle_api(version) is expected
-
-
-@pytest.mark.parametrize("version", [None, Version("0.5.11")])
-def test_ovphysx_0511_lifecycle_entry_points(version: Version | None):
+def test_lifecycle_entry_points(version: Version | None, expected: dict[str, str]):
     entry_points = build_lifecycle_entry_points(version)
-    assert dict(entry_points) == {"warmup": "warmup_gpu", "destroy": "release"}
-
-
-def test_ovphysx_06_lifecycle_entry_points():
-    entry_points = build_lifecycle_entry_points(Version("0.6"))
-    assert dict(entry_points) == {"warmup": "warmup", "destroy": "destroy"}
-
-
-def test_installed_entry_points_match_the_installed_version():
-    assert dict(OVPHYSX_LIFECYCLE_ENTRY_POINTS) == dict(build_lifecycle_entry_points(detect_ovphysx_version()))
+    assert dict(entry_points) == expected
 
 
 def test_published_entry_points_are_read_only():

source/isaaclab_ov/test/physics/test_ovphysx_scene_data_backend.py

@@ -17,6 +17,9 @@
 # CI jobs that need OVPhysX coverage install it explicitly.
 pytest.importorskip("ovphysx.types", reason="ovphysx wheel not installed")
 
+_LEGACY_LIFECYCLE_ENTRY_POINTS = {"warmup": "warmup_gpu", "destroy": "release"}
+_CURRENT_LIFECYCLE_ENTRY_POINTS = {"warmup": "warmup", "destroy": "destroy"}
+
 
 @pytest.fixture(autouse=True)
 def _close_test_views():
@@ -467,6 +470,7 @@ def test_manager_attaches_and_releases_owned_ovstage(monkeypatch):
     from isaaclab_ov.physics import OvPhysxManager
 
     events = []
+    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", _LEGACY_LIFECYCLE_ENTRY_POINTS)
 
     class FakeWriteFloorOp:
         def __init__(self, ordinal):
@@ -544,92 +548,44 @@ def release(self):
     ]
 
 
-def test_manager_uses_current_warmup_api(monkeypatch):
-    """The version-selected current warmup API wins when both methods are visible."""
-    from isaaclab_ov.physics import OvPhysxManager
-    from isaaclab_ov.physics import ovphysx_manager as om_mod
-
-    calls = []
-    physx = SimpleNamespace(
-        warmup=lambda: calls.append("warmup"),
-        warmup_gpu=lambda: calls.append("warmup_gpu"),
-    )
-    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", {"warmup": "warmup", "destroy": "destroy"})
-
-    OvPhysxManager._warmup_physx(physx)
-
-    assert calls == ["warmup"]
-
-
-def test_manager_uses_legacy_warmup_api(monkeypatch):
-    """The version-selected 0.5.11 path uses the legacy method even if both are visible."""
+@pytest.mark.parametrize(
+    ("entry_points", "expected_calls"),
+    [
+        (_LEGACY_LIFECYCLE_ENTRY_POINTS, ["warmup_gpu", "release"]),
+        (_CURRENT_LIFECYCLE_ENTRY_POINTS, ["warmup", "destroy"]),
+    ],
+)
+def test_manager_uses_version_selected_lifecycle_apis(monkeypatch, entry_points, expected_calls):
+    """The selected lifecycle generation controls both entry points."""
     from isaaclab_ov.physics import OvPhysxManager
     from isaaclab_ov.physics import ovphysx_manager as om_mod
 
     calls = []
     physx = SimpleNamespace(
         warmup=lambda: calls.append("warmup"),
         warmup_gpu=lambda: calls.append("warmup_gpu"),
-    )
-    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", {"warmup": "warmup_gpu", "destroy": "release"})
-
-    OvPhysxManager._warmup_physx(physx)
-
-    assert calls == ["warmup_gpu"]
-
-
-def test_manager_rejects_missing_warmup_api(monkeypatch):
-    """A runtime that lacks its version-selected warmup entry point reports it."""
-    from isaaclab_ov.physics import OvPhysxManager
-    from isaaclab_ov.physics import ovphysx_manager as om_mod
-
-    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", {"warmup": "warmup", "destroy": "destroy"})
-    with pytest.raises(AttributeError, match=r"selected warmup\(\) lifecycle entry point"):
-        OvPhysxManager._warmup_physx(SimpleNamespace())
-
-
-def test_manager_uses_current_destroy_api(monkeypatch):
-    """A current OVPhysX runtime tears down through destroy()."""
-    from isaaclab_ov.physics import OvPhysxManager
-    from isaaclab_ov.physics import ovphysx_manager as om_mod
-
-    calls = []
-    physx = SimpleNamespace(
         destroy=lambda: calls.append("destroy"),
         release=lambda: calls.append("release"),
     )
-    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", {"warmup": "warmup", "destroy": "destroy"})
-
-    OvPhysxManager._destroy_physx(physx)
-
-    assert calls == ["destroy"]
-
-
-def test_manager_uses_legacy_release_api(monkeypatch):
-    """The version-selected 0.5.11 path uses release() even if both methods are visible."""
-    from isaaclab_ov.physics import OvPhysxManager
-    from isaaclab_ov.physics import ovphysx_manager as om_mod
-
-    calls = []
-    physx = SimpleNamespace(
-        destroy=lambda: calls.append("destroy"),
-        release=lambda: calls.append("release"),
-    )
-    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", {"warmup": "warmup_gpu", "destroy": "release"})
+    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", entry_points)
 
+    OvPhysxManager._warmup_physx(physx)
     OvPhysxManager._destroy_physx(physx)
 
-    assert calls == ["release"]
+    assert calls == expected_calls
 
 
-def test_manager_rejects_missing_destroy_api(monkeypatch):
-    """A runtime that lacks its version-selected destroy entry point reports it."""
+@pytest.mark.parametrize("operation", ["warmup", "destroy"])
+def test_manager_rejects_missing_lifecycle_api(monkeypatch, operation):
+    """A runtime that lacks its selected lifecycle entry point reports it."""
     from isaaclab_ov.physics import OvPhysxManager
     from isaaclab_ov.physics import ovphysx_manager as om_mod
 
-    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", {"warmup": "warmup", "destroy": "destroy"})
-    with pytest.raises(AttributeError, match=r"selected destroy\(\) lifecycle entry point"):
-        OvPhysxManager._destroy_physx(SimpleNamespace())
+    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", _CURRENT_LIFECYCLE_ENTRY_POINTS)
+    entry_point = _CURRENT_LIFECYCLE_ENTRY_POINTS[operation]
+    lifecycle_method = getattr(OvPhysxManager, f"_{operation}_physx")
+    with pytest.raises(AttributeError, match=rf"selected {entry_point}\(\) lifecycle entry point"):
+        lifecycle_method(SimpleNamespace())
 
 
 def test_manager_releases_legacy_owners_after_release_error(monkeypatch):
@@ -638,7 +594,7 @@ def test_manager_releases_legacy_owners_after_release_error(monkeypatch):
     from isaaclab_ov.physics import ovphysx_manager as om_mod
 
     events = []
-    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", {"warmup": "warmup_gpu", "destroy": "release"})
+    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", _LEGACY_LIFECYCLE_ENTRY_POINTS)
 
     class FakePhysX:
         def reset_stage(self):
@@ -679,7 +635,7 @@ def test_manager_retries_current_destroy_before_releasing_owners(monkeypatch):
     from isaaclab_ov.physics import ovphysx_manager as om_mod
 
     events = []
-    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", {"warmup": "warmup", "destroy": "destroy"})
+    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", _CURRENT_LIFECYCLE_ENTRY_POINTS)
 
     class FakePhysX:
         fail_destroy = True
@@ -745,7 +701,7 @@ def test_manager_releases_owners_after_terminal_destroy_error(monkeypatch):
     from isaaclab_ov.physics import ovphysx_manager as om_mod
 
     events = []
-    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", {"warmup": "warmup", "destroy": "destroy"})
+    monkeypatch.setattr(om_mod, "OVPHYSX_LIFECYCLE_ENTRY_POINTS", _CURRENT_LIFECYCLE_ENTRY_POINTS)
 
     class FakePhysX:
         terminal = False