[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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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.
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
hujc7
changed the title
hujc7
changed the title
hujc7
changed the title
2 participants
Review Map
Summary
Stacking
develop.Review history