Make ovphysx an optional install of isaaclab_ovphysx#5428
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR makes the ovphysx PyPI runtime dependency optional by moving it from install_requires to extras_require in isaaclab_ovphysx/setup.py, and adds "ovphysx" to VALID_ISAACLAB_SUBMODULES so the CLI accepts ./isaaclab.sh -i ovphysx[ovphysx]. The change is mechanically correct and solves a real installation friction point.
Architecture Impact
Low-to-moderate impact. The isaaclab_ovphysx package will now install without its runtime, meaning:
- Users can import
OvPhysxCfgand other configuration classes without theovphysxwheel present. - Any code path that actually uses the
ovphysxruntime will fail with anImportErrorat that point rather than at install time.
This is a reasonable trade-off for installation flexibility, but the package should ideally provide clear error messages when the optional runtime is missing and user code attempts to use it. I did not find evidence of such guards in the provided context, but this is outside the scope of this diff.
Implementation Verdict
Ship it — the changes are correct and minimal.
Test Coverage
No tests are added, and none are strictly required. This is a packaging/installation change that would be difficult to unit test meaningfully. The checklist correctly leaves the test box unchecked. Manual verification that ./isaaclab.sh -i succeeds without ovphysx and that ./isaaclab.sh -i ovphysx[ovphysx] installs the runtime is sufficient validation.
CI Status
No CI checks available yet — cannot assess automated verification.
Findings
🔵 Improvement: source/isaaclab_ovphysx/setup.py:18-20 — Consider documenting the opt-in pattern
The extra name "ovphysx" matching the package name creates a slightly awkward invocation (ovphysx[ovphysx]). This is technically fine but could be clearer with a more descriptive extra name like "runtime" (e.g., ovphysx[runtime]). However, this is a stylistic preference and the current approach is consistent with the PR description.
EXTRAS_REQUIRE = {
"ovphysx": ["ovphysx"], # Could be "runtime": ["ovphysx"] for clarity
}🔵 Improvement: source/isaaclab/isaaclab/cli/commands/install.py:302 — Alphabetical ordering maintained correctly
The addition of "ovphysx" between "ov" and "physx" maintains alphabetical ordering in VALID_ISAACLAB_SUBMODULES. Good attention to detail.
🟡 Warning: Missing runtime guard documentation
The PR description mentions "cfg classes such as OvPhysxCfg that many users only need at import time" — this implies the package modules should import cleanly without the runtime. However, if any module in isaaclab_ovphysx does a top-level import ovphysx, this will fail even for config-only usage. This is outside the diff but worth verifying manually before merge.
Overall: This is a clean, minimal change that correctly implements the stated goal. The packaging mechanics are sound, and the CLI integration follows established patterns. The only consideration is ensuring that the isaaclab_ovphysx modules themselves don't unconditionally import ovphysx at the top level — but that's an existing code concern, not something introduced by this PR.
Greptile SummaryThis PR makes the Confidence Score: 5/5Safe to merge — minimal, correctly scoped changes with no logic errors. Both changed files have clean, targeted modifications. The setup.py change correctly separates the optional runtime into extras_require, and the CLI change adds 'ovphysx' to the validation set so the opt-in install path is properly gated. All three install scenarios (all, bare ovphysx, and ovphysx[ovphysx]) resolve correctly with the existing _install_isaaclab_submodules logic. No P0/P1 issues found. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["./isaaclab.sh -i <install_type>"] --> B{install_type?}
B -->|all| C["isaaclab_submodules = None\nsubmodule_extras = {visualizers: [all]}"]
B -->|ovphysx| D["isaaclab_submodules = [isaaclab, isaaclab_ovphysx]\nsubmodule_extras = {}"]
B -->|"ovphysx[ovphysx]"| E["isaaclab_submodules = [isaaclab, isaaclab_ovphysx]\nsubmodule_extras = {isaaclab_ovphysx: [ovphysx]}"]
C --> F["pip install -e source/isaaclab_ovphysx\n(wrapper only, no runtime)"]
D --> G["pip install -e source/isaaclab_ovphysx\n(wrapper only, no runtime)"]
E --> H["pip install -e source/isaaclab_ovphysx[ovphysx]\n(wrapper + ovphysx runtime)"]
F --> I["OvPhysxCfg importable ✓\novphysx runtime absent"]
G --> I
H --> J["OvPhysxCfg importable ✓\novphysx runtime present ✓"]
Reviews (1): Last reviewed commit: "Allow installing ovphysx via ./isaaclab...." | Re-trigger Greptile |
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
The new commit adds a SUBMODULE_DEFAULT_EXTRAS mechanism that automatically applies [ovphysx] when users run ./isaaclab.sh -i ovphysx, eliminating the awkward ovphysx[ovphysx] syntax I noted as a minor improvement opportunity in the previous review.
Implementation Verdict
Ship it — the follow-up commit directly addresses the UX concern and is implemented cleanly.
Findings
Previous concern about the awkward ovphysx[ovphysx] invocation has been addressed with SUBMODULE_DEFAULT_EXTRAS. The implementation correctly applies default extras only when the user hasn't explicitly specified an extras selector (if not editable:), preserving the ability to override with explicit syntax like ovphysx[] if needed.
No new issues identified.
ooctipus
force-pushed
the
zhengyuz/ovphysx-optional
branch
from
58358dc to
3035a16
Compare
ooctipus
changed the title
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
The latest commit (3035a16) removes the SUBMODULE_DEFAULT_EXTRAS mechanism from the previous commit, leaving only the minimal setup.py change that makes ovphysx an optional extra. This is a simpler approach that avoids touching CLI machinery.
Implementation Verdict
Ship it — The change is correct and minimal. Moving ovphysx from install_requires to extras_require achieves the stated goal of preventing install failures when the ovphysx wheel is unavailable.
Findings
🟡 Warning: Potential import-time failure — While the setup.py change is correct, if any module in isaaclab_ovphysx (e.g., the cloner or physics modules) has a top-level import ovphysx, users will hit ModuleNotFoundError at import time even though installation succeeded. This should be guarded with lazy imports or try/except blocks in the affected modules. However, this is outside the scope of this PR's diff and may already be handled — worth verifying but not blocking.
The PR description is clear about the opt-in syntax (pip install -e source/isaaclab_ovphysx[ovphysx]), which is the standard pip pattern. Clean and minimal change.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
No new changes since the previous review — commit 3c27b75 is identical to the previously reviewed 3035a16 (just a different SHA, likely from a rebase or force-push). The change correctly makes ovphysx an optional extra.
Implementation Verdict
Ship it — The previous review stands. The setup.py change is minimal and correct.
Previous Finding Status
🟡 The previous warning about potential import-time ModuleNotFoundError in isaaclab_ovphysx modules remains relevant but is outside this PR's scope. The package maintainers should verify that runtime-dependent modules use lazy imports or try/except guards.
CI Status
Pre-commit and wheel build passed. Installation tests still pending but expected to pass since this only relaxes dependencies.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
No substantive changes since the previous review — commit dd72053 is functionally identical to the previously reviewed commits. The setup.py change correctly makes ovphysx an optional extra.
Implementation Verdict
Ship it — The change is minimal and correct.
CI Status
🔴 Installation Tests failed — This needs investigation before merging. The failure could be unrelated to this PR (flaky test, infrastructure issue), but it could also indicate that removing ovphysx from hard dependencies breaks an import path somewhere in the installation test suite. The author should verify the failure cause.
Findings
🔴 Critical: CI failure requires triage — The Installation Tests job failed. Before shipping, confirm this failure is unrelated to the dependency change. If isaaclab_ovphysx modules have top-level import ovphysx statements (without try/except guards), the install tests may now fail when importing the package.
2 participants
Description
source/isaaclab_ovphysx/setup.pylists"ovphysx"ininstall_requires.The default
./isaaclab.sh -iflow auto-discovers every package undersource/and pip-installs each one, so the wrapper's hard dep on theovphysxPyPI wheel is fetched unconditionally. When that wheel isunavailable (the common case for users who don't need the runtime), the
whole install aborts and unrelated cfg classes such as
OvPhysxCfgbecome unimportable.
This change makes the runtime an opt-in extra:
After the change:
./isaaclab.sh -i(default) — installsisaaclab_ovphysxwrapper-only,cfg modules import without needing the
ovphysxwheel.pip install -e source/isaaclab_ovphysx[ovphysx]— installs the wrapperwith the
ovphysxruntime alongside it for users who want it.No CLI machinery is touched. Users who want the runtime use pip's
existing
[extra]syntax directly; we don't need to surface it through./isaaclab.shsinceisaaclab_ovphysxis already installed by thedefault all-scan.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there