[Task Clean-up][OVPhysX] Dexterous Part 2/8: Fix articulation and manager runtime - #6412
Conversation
Actuator joint indices now follow the common actuator indexing contract; initialization alongside Kit reuses Kit's registered PhysX schema provider instead of double-registering; the manager accepts both the declared public runtime API and the current runtime API. Validated by full dexterous training runs on the OVPhysX backend as part of the Task Clean-up campaign.
Greptile SummaryThis PR fixes three OVPhysX correctness issues: actuator joint indices now follow the common isaaclab contract (
Confidence Score: 3/5The current-API path used in training is well-exercised; the declared-legacy path has a real gap where four carbonite overrides that suppress USD write-back are silently skipped when set_setting is absent. The declared-legacy path in _create_physx_instance calls only set_config_int32(NUM_THREADS, 8) when set_setting is unavailable, silently omitting physxDispatcher, updateToUsd, updateVelocitiesToUsd, and updateParticlesToUsd. If those settings default to enabled in that API variant, physics data would be written to USD every step. The inspect.signature call on line 743 also has no guard against C extension types that do not expose a Python signature, which would crash initialization entirely. ovphysx_manager.py lines 742-763 (_create_physx_instance legacy branch) and articulation.py line 3930 (torch.int32 index tensor). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[_create_physx_instance] --> B{hasattr PhysX set_cpu_mode?}
B -- Yes/current --> C[set_cpu_mode + PhysXConfig full overrides]
B -- No/legacy --> D[inspect.signature PhysX.parameters]
D --> E{active_cuda_gpus in params AND gpu?}
E -- Yes --> F[PhysXConfig suppressReadback only]
E -- No --> G{gpu_index in params?}
G -- Yes --> H[add gpu_index to kwargs]
G -- No --> I[device only]
F --> J[physx = PhysX kwargs]
H --> J
I --> J
J --> K{hasattr physx set_setting?}
K -- Yes --> L[set_setting all overrides]
K -- No --> M[set_config_int32 NUM_THREADS only - missing physxDispatcher updateToUsd etc]
%%{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"}}}%%
flowchart TD
A[_create_physx_instance] --> B{hasattr PhysX set_cpu_mode?}
B -- Yes/current --> C[set_cpu_mode + PhysXConfig full overrides]
B -- No/legacy --> D[inspect.signature PhysX.parameters]
D --> E{active_cuda_gpus in params AND gpu?}
E -- Yes --> F[PhysXConfig suppressReadback only]
E -- No --> G{gpu_index in params?}
G -- Yes --> H[add gpu_index to kwargs]
G -- No --> I[device only]
F --> J[physx = PhysX kwargs]
H --> J
I --> J
J --> K{hasattr physx set_setting?}
K -- Yes --> L[set_setting all overrides]
K -- No --> M[set_config_int32 NUM_THREADS only - missing physxDispatcher updateToUsd etc]
Reviews (1): Last reviewed commit: "Fix OVPhysX articulation indexing and ma..." | Re-trigger Greptile |
| if hasattr(physx, "set_setting"): | ||
| physx.set_setting("/persistent/physics/numThreads", "8") | ||
| physx.set_setting("/physics/physxDispatcher", "true") | ||
| physx.set_setting("/physics/updateToUsd", "false") | ||
| physx.set_setting("/physics/updateVelocitiesToUsd", "false") | ||
| physx.set_setting("/physics/updateParticlesToUsd", "false") | ||
| else: | ||
| physx.set_config_int32(ovphysx.ConfigInt32.NUM_THREADS, 8) |
There was a problem hiding this comment.
Missing carbonite overrides in legacy path when
set_setting is unavailable
The else branch only applies NUM_THREADS via set_config_int32, silently skipping physxDispatcher, updateToUsd, updateVelocitiesToUsd, and updateParticlesToUsd. The PhysXConfig passed to the constructor (lines 746–751) only contains suppressReadback and suppressFabricUpdate, so these four settings are never applied.
If the declared legacy wheel defaults updateToUsd to True, physics state would be written back to the USD stage on every step — causing severe performance degradation and potentially inconsistent state. The original code (before this PR) applied all four overrides unconditionally via carbonite_overrides. The new test for the legacy API (test_manager_supports_declared_legacy_runtime_api) only asserts that set_config_int32(NUM_THREADS, 8) is called, leaving this gap unverified.
| return ovphysx.PhysX(**physx_kwargs) | ||
|
|
||
| physx_kwargs = {"device": ovphysx_device} | ||
| physx_parameters = inspect.signature(ovphysx.PhysX).parameters |
There was a problem hiding this comment.
inspect.signature may fail on C extension types without recovery
inspect.signature(ovphysx.PhysX) is called without a try/except guard. For C extension types (e.g., pybind11 or Cython bindings) that do not expose a Python-visible __init__ signature via __text_signature__, this raises ValueError: callable ... is not supported by signature. The caller (_warmup_and_load) does not catch this, so PhysX initialization would abort with an unhandled exception. A try/except (ValueError, TypeError) fallback that treats missing parameters as empty (and proceeds to the set_setting/set_config_int32 post-construction path) would make this branch safe.
| if len(joint_names) == self.num_joints: | ||
| actuator_joint_ids = slice(None) | ||
| else: | ||
| actuator_joint_ids = torch.tensor(joint_ids, device=self.device, dtype=torch.int32) |
There was a problem hiding this comment.
torch.int32 index tensor may not be accepted by all PyTorch advanced-indexing paths
actuator_joint_ids is created with dtype=torch.int32 and is immediately used to index several .torch tensors at lines 3943–3950. Standard PyTorch advanced indexing expects int64 (LongTensor); older bundled PyTorch versions raise RuntimeError: expected scalar type Long but found Int for int32 index tensors. Using dtype=torch.int64 (or inserting .long() casts at the indexing sites) would eliminate this version dependency.
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!
C-extension constructors may not expose a Python-visible signature; fall back to an empty parameter set instead of raising. Also documents the legacy runtime's settings limitation.
|
AI-generated review comment: |
The module also carries OvPhysxManager tests added in this PR; the docstring now names both coverage areas.
…' into jichuanh/task-cleanup-dex-part02
…isualizer teardown (#6411) ## Review Map - **Exact changes**: a stacked PR's page shows the cumulative diff of its dependency chain; the link pins the commit range that is the PR's own contribution. - Links pin specific SHAs and can go stale after a branch update — the table on #6324 is refreshed first. | PR | Status | Depends on | Exact changes | |---|---|---|---| | #6410 [Docs] Environment overview regen |  | — | — | | 📌 #6411 Part 1/8: Newton cloner/cubric/visualizer fixes (this PR) |  | — | — | | #6412 Part 2/8: OVPhysX articulation + manager runtime |  | — | — | | #6413 Part 3/8: Reorient Direct, torch |  | — | — | | #6414 Part 4/8: MARL-to-single-agent fix + handover/camera Direct |  | #6413 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6414/changes/6e8a63e4e028b2d43676ea30c446b9dc9068c7b5..5cb00e7cb5cc813b202521272e043007cd255194) | | #6418 Part 5/8: Reorient manager counterparts |  | #6413 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6418/changes/79f87501ac4c81de93a71dab00dc443da62113aa..e7c9a9a3fae3a7972b0c5165ae683abffb7d0e0f) | | #6421 Part 6/8: Handover + camera manager counterparts |  | #6413, #6414, #6418 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6421/changes/01c9f4d8c5c35a5688b2a5bb90209e16b8f81b99..835a5815ec49b11aada1d20a76c177054505e6e7) | | #6415 Part 7/8: Benchmark success-rate utilities + docs |  | #6413, #6414, #6418, #6421 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6415/changes/e1abb6b1790ccc57af42551eebccf743633f1f13..d6348539aa8032d9668c20a8fea462c5d88d3af9) | | #6582 Part 8/8: Warp variants → experimental (draft; merges last) |  | #6413 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6582/changes/21dbb1769c4e30c8e9e5b0f563c2dae24c230349..83e1587cadd9712a60615ed2a3cb2d177c2ac24d) | | #6324 [DO-NOT-MERGE] Lumped validation reference |  | ALL | — | ## Summary - Fixes Newton cloner label rows, the cubric IAdapter version audit (exact-match fallback to the CPU hierarchy path), and visualizer teardown. - Retains an in-tree `ignore_paths` workaround for custom-frequency USD traversal; it becomes redundant once the Newton pin advance (#6584) merges — this PR then only needs a rebase. ## Stacking - Independent; based on `develop`. ## Review history - Approved. The Newton pin + MuJoCo overrides were split out to #6584 via revert commits (2026-07-17) so this PR's CI runs against develop's pins.
…eorientation Direct tasks (#6413) ## Review Map - **Exact changes**: a stacked PR's page shows the cumulative diff of its dependency chain; the link pins the commit range that is the PR's own contribution. - Links pin specific SHAs and can go stale after a branch update — the table on #6324 is refreshed first. | PR | Status | Depends on | Exact changes | |---|---|---|---| | #6411 Part 1/9: Newton cloner/cubric/visualizer fixes |  | — | merged | | #6412 Part 2/9: OVPhysX articulation + manager runtime |  | — | merged | | 📌 #6413 Part 3/9: Reorient Direct, torch (this PR) |  | — | [changes](https://github.com/isaac-sim/IsaacLab/pull/6413/changes/f4895f0f9ee..d29afc75e71) | | #6414 Part 4/9: MARL-to-single-agent fix + handover/camera Direct |  | #6413 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6414/changes/d29afc75e71..b10a84948f8) | | #6418 Part 5/9: Reorient manager counterparts |  | #6413 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6418/changes/d29afc75e71..707d37f8f99) | | #6421 Part 6/9: Handover + camera manager counterparts |  | #6413, #6414, #6418 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6421/changes/707d37f8f99..b50ac8906fc) | | #6410 Part 7/9: Environment overview docs |  | #6421 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6410/changes/b50ac8906fc..af259c0778d) | | #6415 Part 8/9: Benchmark success-rate utilities |  | #6421 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6415/changes/b50ac8906fc..c7f2f019d8b) | | #6582 Part 9/9: Warp variants → experimental (draft; merges last) |  | #6413 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6582/changes/d29afc75e71..167c28578b3) | | #6324 [DO-NOT-MERGE] Lumped validation reference |  | ALL | — | ## Summary - Adds a behavioral `Metrics/success_rate` signal (goal-reach streaks per episode) and threshold-independent episode orientation-error diagnostics to the Direct reorientation environments; success gates task health, reward stays diagnostic. - Task logic is **torch-first** per the mainline convention (plain torch buffers; `.torch` accessors only at the core-lib boundary). This supersedes the earlier warp-first revision of this PR; the warp implementation moved to `isaaclab_tasks_experimental` (#6582). - Lands shared helpers in `isaaclab_tasks.core.utils` (`EpisodeErrorRecorder`, `sample_joint_positions_within_limits`) with torch math tests; fixes hand resets below lower joint limits. ## Stacking - First stack PR; based on `develop`. ## Validation - Kit-free torch math + utils tests green; Direct state training on Newton: success rate 0.85–1.00, reward ≈3600–3900 at 2000 iterations, frame-verified policy video. Full integrated evidence: #6324. ## Review history - The earlier warp-era review rounds on this PR are superseded by the torch restack (2026-07-16); the warp implementation and its review-polished kernels live on in #6582.
… and enable handover Direct RSL-RL (#6414) ## Review Map - **Exact changes**: a stacked PR's page shows the cumulative diff of its dependency chain; the link pins the commit range that is the PR's own contribution. This PR (#6414) is now rebased directly onto `develop`, so its own **Files changed** tab is its contribution. - Links pin specific SHAs and can go stale after a branch update — the table on #6324 is refreshed first. | PR | Status | Depends on | Exact changes | |---|---|---|---| | #6411 Part 1/9: Newton cloner/cubric/visualizer fixes |  | — | merged | | #6412 Part 2/9: OVPhysX articulation + manager runtime |  | — | merged | | #6413 Part 3/9: Reorient Direct, torch |  | — | merged | | 📌 #6414 Part 4/9: MARL-to-single-agent fix + handover/camera Direct (this PR) |  | — (on develop) | Files changed tab | | #6418 Part 5/9: Reorient manager counterparts |  | #6413 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6418/changes/d29afc75e71..707d37f8f99) | | #6421 Part 6/9: Handover + camera manager counterparts |  | #6414, #6418 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6421/changes/707d37f8f99..b50ac8906fc) | | #6410 Part 7/9: Environment overview docs |  | #6421 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6410/changes/b50ac8906fc..af259c0778d) | | #6415 Part 8/9: Benchmark success-rate utilities |  | #6421 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6415/changes/b50ac8906fc..c7f2f019d8b) | | #6582 Part 9/9: Warp variants → experimental (draft; merges last) |  | — | [changes](https://github.com/isaac-sim/IsaacLab/pull/6582/changes/d29afc75e71..167c28578b3) | | #6324 [DO-NOT-MERGE] Lumped validation reference |  | ALL | — | ## Summary Converts the two-hand Shadow Hand **handover** task from multi-agent (MARL) to **single-agent** (enabling RSL-RL training), fixes the underlying MARL→single-agent observation bridge, and consolidates the shared Shadow Hand definitions into the asset. - **MARL → single-agent bridge (general, broader than this task).** Single-agent RL libraries train `DirectMARLEnv` tasks via `multi_agent_to_single_agent`; the bridge dropped the latest observations from the public buffer. Fixed generally — RSL-RL observations read from the env-owned `obs_buf`, stored by `reset` like `step` on all env bases (`DirectRLEnv`, `DirectMARLEnv`, the experimental warp base). Every MARL task + single-agent runner benefits; handover is the first consumer. - **Handover Direct → single-agent + RSL-RL.** Single-agent conversion, RSL-RL runner config, shared identity in `handover_common`, success-rate reward metrics, and a fix for its Newton construction failure (see below). - **Shadow Hand Newton robot → the asset.** Moved the Newton (MJWarp) robot cfg into `isaaclab_assets` as `SHADOW_HAND_NEWTON_CFG` (beside `SHADOW_HAND_CFG`); reorient and handover consume it as **siblings** (neither imports the other). Reorient uses the default gains; handover overrides only the finger stiffness/damping to `20/2` for its catch. - **Shadow camera cleanup.** Renamed the camera Direct modules to `shadow_hand_direct_camera_env[_cfg]`; moved cube-keypoint math to the shared, unit-tested `reorient/mdp/observations.py` (replacing the per-env `compute_keypoints` shim); and added OVPhysX physics presets to the handover and camera Direct environments. The camera env config is otherwise unchanged from develop. ### Behavior changes **Reorient-on-Newton is unchanged.** The Newton hand definition moves **verbatim** into `SHADOW_HAND_NEWTON_CFG` — same joint expressions, effort limits, gains, friction and armature — so its config diff vs `develop` is purely the move, and the Direct env config has no diff at all. Handover changes in three ways: - Its **default physics backend moves from PhysX to Newton** (MJWarp), matching the reorientation tasks. Pass `physics=physx` for the previous backend. - Its `distal_passive` actuator group is **removed**. It targeted `robot0_(FF|MF|RF|LF)J0` — joints that **do not exist** on the Newton asset (its fingers are numbered `J1`–`J4`/`J5`), so constructing the environment raised `No joints found for actuator group`. The `fingers` group already drives those joints, and the comment justifying the group (USD-baked `stiffness=286/damping=57`) does not apply to this asset, which authors no drive stiffness at all. - Two orientation fixes: the goal orientation was initialized to a 180-degree rotation instead of identity, and the Newton root rotation replaced the asset's baked rotation instead of composing with it, leaving both palms rotated 90 degrees. Two pre-existing discrepancies were found while verifying against the asset and are left for a follow-up, since each is a behavior change needing its own validation: the config sets `armature=2e-3` where the asset authors `0.0002`, and the per-finger `J1`/`J2` pair is coupled by a fixed tendon that the MJWarp solver currently skips — the actuator gains are what hold that pair together today, which is why the `J4` knuckle-abduction joints cannot simply be added to the driven set. ## Notes - Rebased onto **latest develop**; clean 3-commit history: `Add single-agent flattening for MARL Direct environments` / `Convert Shadow Hand handover to single-agent Direct with RSL-RL` / `Clean up Shadow reorient/camera tasks and consolidate the Newton hand`. ## Validation - Kit-free suite green: handover, keypoint math, the MARL adapter, and new checks pinning the RSL-RL wrapper observation contract (verified to fail against the previous implementation). - Camera Direct reaches training takeoff under `rsl_rl` (frame-verified videos); its env config now matches develop's established vision env. - **Reorient-on-Newton vs `develop`** (`rsl_rl`, 2048 envs, 1500 iterations, 4 runs per config): this branch spans 524–664 reward, `develop` spans 654–822 — overlapping distributions whose per-config spreads (140–168) exceed the gap between them. The move is additionally verified at the config level: the Newton robot configuration is character-for-character identical to `develop`'s. - **Handover-on-Newton** trains end-to-end on this branch (reward 1314), which the `develop` config cannot do at all — it fails during construction. Full evidence: #6324.
Review Map
Summary
Stacking
develop.Review history