Install ovphysx wheel by default in --install

[AntoineRichard]

Description

Promote the publicly available ovphysx PyPI wheel out of the manual ov[ovphysx] extra and into the default ./isaaclab.sh --install flow, mirroring how the newton wheel is treated. The OVRTX renderer wheel stays opt-in via --install 'ov[ovrtx]' or --install 'ov[all]'.

Concretely:

• MANUAL_EXTRA_FEATURES drops "ov" (it now only contains "contrib"), so the ov feature is part of the automatic -i / -i all set alongside newton, rl, and visualizer.
• _install_ov_extra_dependencies("") no longer prints a help message and returns; it now installs isaaclab_ovphysx[ovphysx]. Explicit ov[ovrtx], ov[ovphysx], and ov[all] behavior is unchanged.
• CLI --install help text and the command_install docstring are updated to reflect the new default.
• With the wheel guaranteed in every standard install, the pytest.importorskip("ovphysx.types", reason="ovphysx wheel not installed") guards (9 sites) and one inner importorskip("isaaclab_ovphysx.tensor_types") call are removed from the isaaclab_ovphysx test suite. The isaaclab_ov CI job (whose filter-pattern: "isaaclab_ov" already collects these tests) now exercises them for real instead of silently skipping.

No new required dependency is added; source/isaaclab_ovphysx/setup.py already declared EXTRAS_REQUIRE = {"ovphysx": ["ovphysx"]}. The extra remains unpinned for now — pinning a minimum version is a sensible follow-up once we lock in a release.

Fixes # (n/a)

Type of change

• New feature (non-breaking change which adds functionality)

Strictly, the default install footprint grows by one PyPI wheel and previously-skipped tests now run, so downstream consumers of --install see new behavior — but no public API is removed or renamed and all existing ov[...] selectors continue to work as before.

Screenshots

N/A — install-CLI and test-gating change.

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 (CLI --help text and command_install docstring updated)
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works (new test_all_installs_ov_with_default_ovphysx_selector plus updates to test_manual_extra_features and test_all_does_not_install_manual_extra_dependencies; 51/51 pass in test_install_command_parsing.py)
• I have added a changelog fragment under source/<pkg>/changelog.d/ for every touched package — source/isaaclab/changelog.d/antoiner-ovphysx-default-install.rst and source/isaaclab_ovphysx/changelog.d/antoiner-ovphysx-default-install.rst
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[isaaclab-review-bot]

Code Review Summary

This is a well-structured PR that promotes the ovphysx wheel from opt-in to default installation, aligning with how the Newton wheel is already treated. The implementation is clean and the test updates are thorough.

✅ Strengths

1. Consistent design pattern: Following the existing Newton wheel approach makes the codebase more predictable and the install experience more uniform.

2. Improved test reliability: Removing the importorskip guards means missing the ovphysx wheel in CI will now fail loudly rather than silently skipping tests — this catches real regressions rather than masking them.

3. Good documentation: Both changelog fragments clearly explain the change, and the help text in __init__.py is updated to reflect the new behavior.

4. Comprehensive test updates: All 8 test files were updated to remove the skip guards, and test_install_command_parsing.py includes a new test (test_all_installs_ov_with_default_ovphysx_selector) to verify the auto-install path.

💭 Minor Observations

1. No version pin on ovphysx (noted by author): The PR description mentions setup.py has "ovphysx": ["ovphysx"] with no version constraint. This is fine for now, but consider adding a minimum version pin once a stable release is identified to prevent potential compatibility issues.

2. CI job monitoring (noted by author): Since isaaclab_ov will now actually exercise the ovphysx tests, worth watching the first few CI runs to catch any previously-hidden issues. This is already acknowledged in the test plan.

🔍 Implementation Notes

The logic change in _install_ov_extra_dependencies() is clean:

• Empty selector → defaults to {"ovphysx"}
• Non-empty selector → validates and processes as before
• OVRTX remains opt-in via explicit ov[ovrtx] or ov[all]

The behavior for explicit selectors (ov[ovrtx], ov[all], ov[ovphysx]) is unchanged, which minimizes risk for users who already use those.

---

Overall this is a straightforward, well-tested change that improves the default user experience while maintaining backward compatibility for explicit install commands.

[greptile-apps]

Greptile Summary

This PR promotes the ovphysx wheel from an opt-in extra (ov[ovphysx]) into the default ./isaaclab.sh --install flow, matching Newton's treatment, while keeping the OVRTX renderer wheel explicitly opt-in. The companion change removes the pytest.importorskip("ovphysx.types") skip-guards across the isaaclab_ovphysx test suite so tests now fail loudly if the wheel is missing.

• install.py / __init__.py: ov removed from MANUAL_EXTRA_FEATURES; _install_ov_extra_dependencies now defaults an empty selector to {\"ovphysx\"} instead of printing an info message and returning early. All four selector paths (\"\", ovphysx, ovrtx, all) behave correctly.
• Test suite guard removal: pytest.importorskip(\"ovphysx.types\") and the per-test importorskip(\"isaaclab_ovphysx.tensor_types\") guards are deleted from 7 test files; import pytest is dropped from files where it was only used for those guards; # noqa: E402 annotations on now-correctly-ordered top-level imports are cleaned up.
• Install CI tests: test_manual_extra_features updated, new test_all_installs_ov_with_default_ovphysx_selector added, two test methods renamed to drop the manual_ prefix from their names.

Confidence Score: 4/5

Safe to merge; install logic, dispatch tests, and guard removal are all internally consistent. The only thing worth a follow-up is a redundant module-level import left in one helper test file.

The install-CLI changes are correct and well-covered by the updated test suite. The guard-removal sweep across the seven test files is clean — import pytest is dropped only in files that had no remaining pytest.* API calls, and pytest fixtures like monkeypatch/caplog are injected by the framework without a direct import. The one minor leftover is a duplicated MockOvPhysxBindingSet import inside test_mock_binding_set_rigid_object_shapes in test_articulation_helpers.py, which was pre-existing but is more visible now that the module-level import sits right above it. No functional defects found.

source/isaaclab_ovphysx/test/assets/test_articulation_helpers.py — has a redundant inner import of MockOvPhysxBindingSet that mirrors the module-level import added by this PR.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/cli/commands/install.py	Removes ov from MANUAL_EXTRA_FEATURES and rewires _install_ov_extra_dependencies so an empty selector defaults to ovphysx; logic is correct and all four selector paths behave as intended.
source/isaaclab/test/install_ci/test_install_command_parsing.py	Tests correctly updated: MANUAL_EXTRA_FEATURES assertion tightened to just contrib, new test verifies ov is auto-installed with empty selector for -i all, two test renames clarified.
source/isaaclab_ovphysx/test/physics/test_ovphysx_scene_data_backend.py	Removed importorskip guard and import pytest; file never calls pytest.xxx directly (uses monkeypatch/caplog as injected fixtures only), so removal is correct.
source/isaaclab_ovphysx/test/assets/test_articulation_helpers.py	importorskip guard removed, import pytest dropped, but MockOvPhysxBindingSet is now imported redundantly at module level and again inside test_mock_binding_set_rigid_object_shapes.
source/isaaclab_ovphysx/test/assets/test_articulation.py	importorskip guard and trailing noqa comments removed cleanly; imports now appear at the top in natural order.
source/isaaclab_ovphysx/test/sensors/test_contact_sensor.py	importorskip guard and noqa annotations removed cleanly; pytest import retained as it is still used for test decorators.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["./isaaclab.sh --install [token]"] --> B{token?}
    B -- "all / empty" --> C[Auto-install VALID_EXTRA_FEATURES minus MANUAL_EXTRA_FEATURES]
    C --> D["newton, rl, visualizer, ov (empty selector)"]
    D --> E["_install_ov_extra_dependencies('')"]
    E --> F["selectors = {'ovphysx'}"]
    F --> G["pip install isaaclab_ovphysx[ovphysx]"]
    B -- "ov" --> E
    B -- "ov[ovrtx]" --> H["_install_ov_extra_dependencies('ovrtx')"]
    H --> I["selectors = {'ovrtx'}"]
    I --> J["pip install isaaclab_ov[ovrtx]"]
    B -- "ov[all]" --> K["_install_ov_extra_dependencies('all')"]
    K --> L["selectors expanded to {'ovrtx','ovphysx'}"]
    L --> G
    L --> J
    B -- "none" --> O["Core submodules only — no extras"]
    B -- "contrib" --> P["MANUAL: requires explicit flag, never auto-installed"]

Loading

Comments Outside Diff (1)

1. source/isaaclab_ovphysx/test/assets/test_articulation_helpers.py, line 115-119 (link)

   The MockOvPhysxBindingSet import inside the function body duplicates the module-level import directly above it. With the importorskip guard gone, the inner import no longer serves as a conditional guard, so it can simply be removed.

_{Reviews (1): Last reviewed commit: "Install ovphysx wheel by default in `--i..." | Re-trigger Greptile}

[kellyguo11]

is making ovphysx installable by default mostly for the CI tests? I feel like it could be kept as an optional dependency from user perspective, but we can force an install of it for our CI jobs?

[AntoineRichard]

is making ovphysx installable by default mostly for the CI tests? I feel like it could be kept as an optional dependency from user perspective, but we can force an install of it for our CI jobs?

Yes it's mostly for CI tests. Sure we could leave that optional and handle it similarly to OvRTX in the CI.