Articulation Reordering Part 5/8: Extract articulation interface test utilities#6338
Articulation Reordering Part 5/8: Extract articulation interface test utilities#6338AntoineRichard wants to merge 7 commits into
Conversation
5c8e65d to
7f8fa75
Compare
… utilities (#6334) # Description Moves the rigid object and rigid object collection interface test factories into shared underscore-prefixed utility modules so later parts can reuse them. Pure code movement with no behavior change and no ordering content. Part 1/8 of the series splitting #6248 into independently reviewable units. #6248 remains open, untouched, as the reference implementation; this stack was cut from its final tree after a `develop` sync (Kamino Core, Newton bump, OVPhysX 0.5.1). ## Stack - **Part 1/8 (this PR)**: #6334 — Extract rigid object interface test utilities - Part 2/8: #6335 — Core ordering foundation - Part 3/8: #6336 — Newton backend - Part 4/8: #6337 — PhysX backend - Part 5/8: #6338 — Extract articulation interface test utilities - Part 6/8: #6339 — OVPhysX backend - Part 7/8: #6340 — Cross-backend ordering interface tests - Part 8/8: #6341 — Events, consumers, and documentation ## Type of change - Test/documentation/refactor change ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have added a changelog fragment under `source/<pkg>/changelog.d/` for every touched package - [x] My name exists in `CONTRIBUTORS.md` ## Test Plan - [x] `./isaaclab.sh -p -m pytest source/isaaclab/test/assets/test_rigid_object_iface.py` -> 1478 passed, 12 xfailed - [x] `./isaaclab.sh -p -m pytest source/isaaclab/test/assets/test_rigid_object_collection_iface.py` -> 1902 passed; the 7 failures + 636 errors are pre-existing on `develop` in the local environment (OVPhysX 0.4.13 wheel vs the 0.5.x binding API from #6314) — fail-set identical to the `develop` baseline - [x] `./isaaclab.sh -f` -> all hooks pass --------- Signed-off-by: Antoine RICHARD <antoiner@nvidia.com> Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
068aac3 to
b6da962
Compare
# Description Introduces the public joint/body ordering machinery in `isaaclab` core: the `ArticulationNameMap` type and builder, the ordering preset helper, the Warp reorder kernels with an explicit visibility contract, the symbolic convention resolvers (`physx`, `mjwarp`, `robot_schema`), the `ArticulationCfg.joint_ordering`/`body_ordering` fields, the `BaseArticulation`/`BaseArticulationData` plumbing that backends wire into, and the API reference entries. **Transitional semantics:** no backend consumes the machinery yet, so configuring an ordering in this intermediate state is a silent no-op until Parts 3/4/6 land. This is called out in the cfg field docstrings and resolves quickly as the stack merges. Part 2/8 of the series splitting #6248 into independently reviewable units. #6248 remains open, untouched, as the reference implementation; this stack was cut from its final tree after a `develop` sync (Kamino Core, Newton bump, OVPhysX 0.5.1). ## Stack - Part 1/8: #6334 — Extract rigid object interface test utilities - **Part 2/8 (this PR)**: #6335 — Core ordering foundation - Part 3/8: #6336 — Newton backend - Part 4/8: #6337 — PhysX backend - Part 5/8: #6338 — Extract articulation interface test utilities - Part 6/8: #6339 — OVPhysX backend - Part 7/8: #6340 — Cross-backend ordering interface tests - Part 8/8: #6341 — Events, consumers, and documentation > [!NOTE] > All parts target `develop`, so until the preceding parts merge this PR's diff includes them. The true delta of this part is [this compare](AntoineRichard/IsaacLab@antoiner/articulation-reordering-p1...antoiner/articulation-reordering-p2). ## Type of change - New feature (non-breaking change which adds functionality) ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have added a changelog fragment under `source/<pkg>/changelog.d/` for every touched package - [x] My name exists in `CONTRIBUTORS.md` ## Test Plan - [x] `./isaaclab.sh -p -m pytest source/isaaclab/test/assets/test_articulation_ordering.py` -> 67 passed - [x] `./isaaclab.sh -p -m pytest source/isaaclab/test/assets/test_articulation_ordering_kernels.py` -> 24 passed - [x] `./isaaclab.sh -f` -> all hooks pass
3236e7d to
0571467
Compare
# Description Wires ordering into the Newton backend read/write paths. Non-identity orderings register an unconditional post-step publish that reorders the core state buffers inside the stepped (and CUDA-graph-captured) region, keeping the documented Tier-1 capture-safety contract without Python read guards; `NewtonManager` gains the matching `register`/`unregister_post_step_callback` API. Actuator torque telemetry reads live backend state through the ordering map. Also carries the shared branching USD test fixture used by cross-backend convention tests here and in later parts. Part 3/8 of the series splitting #6248 into independently reviewable units. #6248 remains open, untouched, as the reference implementation; this stack was cut from its final tree after a `develop` sync (Kamino Core, Newton bump, OVPhysX 0.5.1). ## Stack - Part 1/8: #6334 — Extract rigid object interface test utilities - Part 2/8: #6335 — Core ordering foundation - **Part 3/8 (this PR)**: #6336 — Newton backend - Part 4/8: #6337 — PhysX backend - Part 5/8: #6338 — Extract articulation interface test utilities - Part 6/8: #6339 — OVPhysX backend - Part 7/8: #6340 — Cross-backend ordering interface tests - Part 8/8: #6341 — Events, consumers, and documentation > [!NOTE] > All parts target `develop`, so until the preceding parts merge this PR's diff includes them. The true delta of this part is [this compare](AntoineRichard/IsaacLab@antoiner/articulation-reordering-p2...antoiner/articulation-reordering-p3). ## Type of change - New feature (non-breaking change which adds functionality) ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have added a changelog fragment under `source/<pkg>/changelog.d/` for every touched package - [x] My name exists in `CONTRIBUTORS.md` ## Test Plan - [x] `./isaaclab.sh -p -m pytest source/isaaclab_newton/test/assets/test_articulation_ordering_kernels.py` -> 2 passed - [x] `./isaaclab.sh -p -m pytest source/isaaclab_newton/test/assets/test_newton_actuators_newton.py` -> 76 passed - [x] `./isaaclab.sh -p -m pytest source/isaaclab_newton/test/assets/test_articulation.py` -> 257 passed, 8 skipped, 1 xfailed - [x] `./isaaclab.sh -f` -> all hooks pass
| # | ||
| # SPDX-License-Identifier: BSD-3-Clause | ||
|
|
||
| """Shared mocked articulation backend factories for interface tests.""" |
There was a problem hiding this comment.
Agent review: the Part 4 (#6337) review pointed the duplicated actuator ordering-trace machinery (test_newton_actuators_{newton,physx}.py, ~107 identical lines) at this part's shared utilities — but this module can't host it as designed: it's directory-local (resolved only via pytest's per-directory sys.path; the Newton/PhysX test dirs can't import it), and importing it boots Kit and monkeypatches PhysxManager.get_physics_sim_view module-wide, so a real-simulation test file can't pull in just the torch-based trace helpers. The installable isaaclab.test package (already imported cross-package by sibling backend tests, e.g. isaaclab.test.utils) is a home that would work — worth settling here so the Part 4 consolidation lands somewhere it can live.
There was a problem hiding this comment.
Added a pure installable helper at isaaclab.test.utils.articulation_ordering. Both Newton and PhysX actuator tests now import the same canonicalization and trace assertion directly; the backend factories remain directory-local, and the helper is not eagerly re-exported from isaaclab.test.utils.
| # LD_PRELOAD to the ovphysx libcarb.so, which is the signature of a kitless | ||
| # ovphysx run. Also guard the case where neither LD_PRELOAD nor EXP_PATH is | ||
| # set (bare Python, no Kit at all). | ||
| _kitless = "ovphysx" in os.environ.get("LD_PRELOAD", "") or ( |
There was a problem hiding this comment.
Agent review: this kitless-detection/AppLauncher/omni-stub/gravity-mock preamble is the third verbatim copy in this directory (after Part 1's two _rigid_object*_iface_test_utils.py), ~150 duplicated lines total, and the copies have already drifted in comment wording. It's load-bearing CI plumbing — the LD_PRELOAD signature check and the stub list must stay in lockstep across all copies for the kitless runner — and the extraction needs zero parameters (e.g. from _iface_test_boot import simulation_app at the top of all three).
There was a problem hiding this comment.
Extracted the shared Kit/kitless setup into _iface_test_boot.py and switched all three interface utility modules to import its simulation_app. A subprocess regression verifies that they resolve the same bootstrap object.
| _mock_physics_sim_view = MagicMock() | ||
| _mock_physics_sim_view.get_gravity.return_value = (0.0, 0.0, -9.81) | ||
|
|
||
| from isaaclab_physx.physics import PhysxManager as SimulationManager |
There was a problem hiding this comment.
Agent review: this unconditional isaaclab_physx import defeats the find_spec("isaaclab_physx") guard ten lines below — if the package is genuinely optional the import needs to move under the guard; if it isn't, the guard can go.
There was a problem hiding this comment.
Moved the PhysxManager import and gravity mock under the availability guard in all three interface utility modules. A subprocess regression blocks isaaclab_physx imports and verifies that the mock-only utilities still load.
# Description Wires ordering into the PhysX backend read/write paths using the shared kernels: fused user-and-backend target writes, lazily refreshed timestamped `body_mass`/`body_inertia` (see the changelog `Changed` entry for the semantics change vs develop), and ordering maps resolved at initialization. Part 4/8 of the series splitting #6248 into independently reviewable units. #6248 remains open, untouched, as the reference implementation; this stack was cut from its final tree after a `develop` sync (Kamino Core, Newton bump, OVPhysX 0.5.1). ## Stack - Part 1/8: #6334 — Extract rigid object interface test utilities - Part 2/8: #6335 — Core ordering foundation - Part 3/8: #6336 — Newton backend - **Part 4/8 (this PR)**: #6337 — PhysX backend - Part 5/8: #6338 — Extract articulation interface test utilities - Part 6/8: #6339 — OVPhysX backend - Part 7/8: #6340 — Cross-backend ordering interface tests - Part 8/8: #6341 — Events, consumers, and documentation > [!NOTE] > All parts target `develop`, so until the preceding parts merge this PR's diff includes them. The true delta of this part is [this compare](AntoineRichard/IsaacLab@antoiner/articulation-reordering-p3...antoiner/articulation-reordering-p4). ## Type of change - New feature (non-breaking change which adds functionality) ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have added a changelog fragment under `source/<pkg>/changelog.d/` for every touched package - [x] My name exists in `CONTRIBUTORS.md` ## Test Plan - [x] `./isaaclab.sh -p -m pytest source/isaaclab_physx/test/assets/test_articulation.py` -> 239 passed - [x] `./isaaclab.sh -p -m pytest source/isaaclab_physx/test/assets/test_newton_actuators_physx.py` -> 44 passed - [x] `./isaaclab.sh -f` -> all hooks pass
Move the articulation interface test factories into a shared utility module mirroring the rigid object extraction, with ordering-aware construction hooks used by the later cross-backend ordering tests. Part 5 of the articulation reordering series.
The ordering core deleted the _cache_ordering_maps guard machinery; mock articulation factories now configure backend staging directly, matching the backend initialization paths.
0571467 to
389e66c
Compare
Greptile SummaryThis PR extracts articulation backend factory functions from
Confidence Score: 3/5The test file itself is safe, but the new shared utility module has three issues that can break test runs in certain environments. The utility module unconditionally imports isaaclab_physx at module level and replaces graceful try/except ImportError backend detection with bare find_spec guards, meaning any environment where a backend package is discoverable but not importable will crash the entire session. The mock backend also silently discards joint_ordering and body_ordering params that Part 7 will pass. _articulation_iface_test_utils.py — the unconditional physx import block (lines 57-59), the find_spec backend guards (lines 67-98), and the mock branch in get_articulation (lines 546-558) all need attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant T as test_articulation_iface.py
participant U as _articulation_iface_test_utils.py
participant P as isaaclab_physx (unconditional import)
participant N as isaaclab_newton (find_spec guarded)
participant O as isaaclab_ovphysx (find_spec guarded)
participant M as MockArticulation
T->>U: import BACKENDS, get_articulation
U->>P: from isaaclab_physx.physics import PhysxManager (unconditional)
U->>P: "SimulationManager.get_physics_sim_view = MagicMock(...)"
U->>N: importlib.util.find_spec(isaaclab_newton)
U->>O: importlib.util.find_spec(isaaclab_ovphysx)
T->>T: "@pytest.fixture articulation_iface(request)"
T->>U: get_articulation(backend, ...)
alt "backend == physx"
U->>U: create_physx_articulation(...)
U->>U: _resolve_and_install_ordering_maps()
U->>U: _ordering_configure_backend_staging()
else "backend == newton"
U->>U: create_newton_articulation(...)
U->>U: _resolve_and_install_ordering_maps()
U->>U: _ordering_configure_backend_staging()
else "backend == ovphysx"
U->>U: create_ovphysx_articulation(...)
U->>U: _resolve_and_install_ordering_maps()
U->>U: _create_buffers()
else "backend == mock"
U->>M: MockArticulation(...) [ordering params ignored]
end
U-->>T: (articulation, view)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant T as test_articulation_iface.py
participant U as _articulation_iface_test_utils.py
participant P as isaaclab_physx (unconditional import)
participant N as isaaclab_newton (find_spec guarded)
participant O as isaaclab_ovphysx (find_spec guarded)
participant M as MockArticulation
T->>U: import BACKENDS, get_articulation
U->>P: from isaaclab_physx.physics import PhysxManager (unconditional)
U->>P: "SimulationManager.get_physics_sim_view = MagicMock(...)"
U->>N: importlib.util.find_spec(isaaclab_newton)
U->>O: importlib.util.find_spec(isaaclab_ovphysx)
T->>T: "@pytest.fixture articulation_iface(request)"
T->>U: get_articulation(backend, ...)
alt "backend == physx"
U->>U: create_physx_articulation(...)
U->>U: _resolve_and_install_ordering_maps()
U->>U: _ordering_configure_backend_staging()
else "backend == newton"
U->>U: create_newton_articulation(...)
U->>U: _resolve_and_install_ordering_maps()
U->>U: _ordering_configure_backend_staging()
else "backend == ovphysx"
U->>U: create_ovphysx_articulation(...)
U->>U: _resolve_and_install_ordering_maps()
U->>U: _create_buffers()
else "backend == mock"
U->>M: MockArticulation(...) [ordering params ignored]
end
U-->>T: (articulation, view)
|
| from isaaclab_physx.physics import PhysxManager as SimulationManager | ||
|
|
||
| SimulationManager.get_physics_sim_view = MagicMock(return_value=_mock_physics_sim_view) |
There was a problem hiding this comment.
Unconditional
isaaclab_physx import
isaaclab_physx is imported and patched at module level, outside any availability guard. If isaaclab_physx is absent (e.g., a pure Newton or mock-only environment), importing _articulation_iface_test_utils will raise ImportError before any test can run — including the mock-backend tests that are supposed to "always be available". The old test file had the same issue, but moving to a shared utility that other files will also import makes the blast radius larger.
There was a problem hiding this comment.
Fixed across all three interface utility modules: PhysxManager and its gravity mock now live inside the isaaclab_physx availability guard. The new mock-only subprocess regression explicitly rejects any attempted isaaclab_physx import.
| if importlib.util.find_spec("isaaclab_physx") is not None: | ||
| from isaaclab_physx.assets.articulation.articulation import Articulation as PhysXArticulation | ||
| from isaaclab_physx.assets.articulation.articulation_data import ArticulationData as PhysXArticulationData | ||
| from isaaclab_physx.test.mock_interfaces.views import MockArticulationViewWarp as PhysXMockArticulationViewWarp | ||
|
|
||
| BACKENDS.append("physx") | ||
|
|
||
| if importlib.util.find_spec("isaaclab_newton") is not None: | ||
| from isaaclab_newton.assets.articulation.articulation import Articulation as NewtonArticulation | ||
| from isaaclab_newton.assets.articulation.articulation_data import ArticulationData as NewtonArticulationData | ||
| from isaaclab_newton.test.mock_interfaces.views import MockNewtonArticulationView as NewtonMockArticulationView | ||
|
|
||
| BACKENDS.append("newton") | ||
|
|
||
| if ( | ||
| importlib.util.find_spec("isaaclab_ovphysx") is not None | ||
| and importlib.util.find_spec("ovphysx") is not None | ||
| ): | ||
| from isaaclab_ovphysx.assets.articulation.articulation import Articulation as OvPhysxArticulation | ||
| from isaaclab_ovphysx.assets.articulation.articulation_data import ArticulationData as OvPhysxArticulationData | ||
| from isaaclab_ovphysx.test.mock_interfaces.views import MockOvPhysxBindingSet | ||
|
|
||
| BACKENDS.append("ovphysx") | ||
|
|
||
|
|
||
| def create_physx_articulation( | ||
| num_instances: int = 2, | ||
| num_joints: int = 6, | ||
| num_bodies: int = 7, | ||
| num_fixed_tendons: int = 0, | ||
| num_spatial_tendons: int = 0, | ||
| device: str = "cuda:0", |
There was a problem hiding this comment.
find_spec does not guard against import errors
The old code used try/except ImportError which gracefully degrades if a backend package exists on disk but fails to import (missing transitive dependency, broken extension, etc.). importlib.util.find_spec only checks whether the package is locatable; it does not attempt the import. If isaaclab_physx, isaaclab_newton, or isaaclab_ovphysx is on the PYTHONPATH but unimportable, the bare from … import … inside the if block will raise an unhandled ImportError at module load time, crashing the entire test session instead of gracefully skipping the backend. This is especially relevant for OvPhysX, which the PR description notes is not yet fully wired.
There was a problem hiding this comment.
Keeping this behavior intentionally. find_spec treats an absent backend as unavailable, while a discoverable backend whose internal dependency is broken should fail loudly rather than silently reduce test coverage. Added a subprocess regression that distinguishes those two cases and requires the internal ModuleNotFoundError to propagate.
| """ | ||
| Check which backends are available. | ||
| """ |
There was a problem hiding this comment.
The triple-quoted string here is a standalone expression, not a module/function docstring. Python evaluates it (creating a string object) but immediately discards it, so it produces no effect and will not appear in
__doc__. Convert to a regular comment to express the intent clearly.
| """ | |
| Check which backends are available. | |
| """ | |
| # --------------------------------------------------------------------------- | |
| # Check which backends are available. | |
| # --------------------------------------------------------------------------- |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Converted the discarded string expression to the regular section comment shown in the suggestion.
Extract common test bootstrapping and ordering assertions so backend tests share one implementation. Guard optional PhysX setup and make unsupported mock ordering explicit.
Apply repository formatting and import ordering after the new helper files became tracked.
|
Greptile follow-up: the mock factory now preserves |
Cover the isaaclab_newton and isaaclab_physx test changes so the changelog gate recognizes every package touched by P5.
Match the established rigid-object test helper conventions for the mock backend label and private test access. Add a regression check that keeps the three interface utilities consistent.
Exclude the Mock backend case-insensitively from cache tests that only apply to solver-backed articulation implementations.
Description
Moves the articulation interface test factories into a shared utility module mirroring Part 1's rigid object extraction, with ordering-aware construction hooks used by Part 7's cross-backend suite.
Part 5/8 of the series splitting #6248 into independently reviewable units. #6248 remains open, untouched, as the reference implementation; this stack was cut from its final tree after a
developsync (Kamino Core, Newton bump, OVPhysX 0.5.1).Stack
Note
All parts target
develop, so until the preceding parts merge this PR's diff includes them. The true delta of this part is this compare.Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched packageCONTRIBUTORS.mdTest Plan
./isaaclab.sh -p -m pytest source/isaaclab/test/assets/test_articulation_iface.py-> 4086 passed, 96 skipped, 32 xfailed (with Newton and PhysX wired, OVPhysX not yet — exercises the unwired-backend identity path)./isaaclab.sh -f-> all hooks pass