Let isaacsim and ovphysx resolve into one environment - #6938
Let isaacsim and ovphysx resolve into one environment#6938AntoineRichard wants to merge 5 commits into
Conversation
ovphysx caps packaging at <24 while isaacsim-core pins it to ==26.0, so
the two extras were declared conflicting and uv forked the resolution.
That makes `uv run --extra isaacsim --extra ovphysx` impossible, and any
image wanting both PhysX backends had to drop one.
Both caps are stricter than the code needs. Widen the packaging override
to >=20,<27 so the two resolve together at 26.0, and drop the isaacsim /
ovphysx conflict pair.
The lock diff is large because ovphysx stops being a fork dimension and
uv rewrites resolution markers throughout, but exactly one package
changes version: packaging collapses from {23.2, 26.0} to {26.0}. All
426 packages are otherwise identical. Note that `uv lock` on unmodified
develop already produces most of this churn.
`ov` stays forked: it also pulls ovrtx, which has not been validated
alongside Isaac Sim.
![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
The packaging override and removal of the direct isaacsim/ovphysx conflict consistently enable those extras to co-resolve. One retained conflict and its comment need clarification: teleop/ovphysx can no longer be described as inherited from isaacsim.
- Design and architecture: Keeping
ovforked because itsovrtxdependency remains unvalidated is appropriately scoped. However, the retainedteleop/ovphysxconflict now requires an independent justification or removal becauseisaacsimitself no longer conflicts withovphysx. - API: The direct
--extra isaacsim --extra ovphysxcompatibility surface is updated consistently through the override, conflict declaration, lockfile, and regression assertions. The behavior of--extra teleop --extra ovphysxremains restricted without a stated reason after the underlyingisaacsimconflict was removed. - Implementation: The override and direct conflict removal agree with the updated tests. Fix the contradictory comment around the conflict list and either remove the retained
teleop/ovphysxpair or document the concrete independent dependency conflict that requires it.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| # bundles isaacsim, so it inherits that pair. Every other historical conflict was an | ||
| # over-strict pin that the overrides above now reconcile. | ||
| # ``ov`` stays forked: it pulls ovrtx as well, which has not been validated | ||
| # alongside Isaac Sim. ``teleop`` bundles isaacsim, so it inherits both pairs. |
There was a problem hiding this comment.
🔵 Suggestion · Design Architecture — Comment contradicts retained teleop/ovphysx conflict
The comment says teleop "inherits both pairs" from isaacsim, but after this diff isaacsim only conflicts with ov, so nothing is inherited for ovphysx. The retained [{ extra = "teleop" }, { extra = "ovphysx" }] entry now stands on its own and keeps --extra teleop --extra ovphysx forked, even though teleop bundles isaacsim, which co-resolves with ovphysx. Either drop that pair or document its actual independent reason.
Greptile SummaryThe PR allows the
Confidence Score: 5/5The PR appears safe to merge because the manifest, lockfile, tests, and documented intent are consistent and no actionable failure was identified. The changed override resolves both extras on packaging 26.0, the intended conflict alone is removed, remaining unvalidated conflicts are preserved, and the regenerated lock graph reflects those declarations without an identified broken resolution path. Important Files Changed
Reviews (1): Last reviewed commit: "Let isaacsim and ovphysx resolve into on..." | Re-trigger Greptile |
usd-core and usd-exchange each vendor a complete pxr runtime built against a different USD version (25.11 and 25.05), overlapping on 51 files including the compiled extensions. newton[importers] requires usd-exchange without a marker, defeating the platform split declared in project.dependencies, so both land on x86_64. uv installs concurrently, so the per-file winner varies between identical builds. A mixed-ABI pxr fails at import with Tf_PyEnumWrapper has not been created yet, which made image builds intermittently unusable.
StafaH
left a comment
StafaH
left a comment
There was a problem hiding this comment.
Let's trim the comments and verbose explanation
The teleop/ovphysx pair existed only because teleop bundles isaacsim, whose packaging cap the override now reconciles. Verified that `uv sync --extra teleop --extra ovphysx` resolves, so the pair is removed rather than left with a stale justification. Also condense the override comments and the changelog fragment.
3 participants
Description
ovphysxcapspackagingat<24whileisaacsim-corepins it to==26.0, so the two extras are declared conflicting and uv forks the resolution. That makesuv run --extra isaacsim --extra ovphysximpossible: an environment can have Isaac Sim or OvPhysX, never both. Any image that wants to exercise both PhysX backends has to build twice or drop one.Both caps are stricter than the code needs. This widens the
packagingoverride to>=20,<27so the two resolve together at26.0, and drops theisaacsim/ovphysxconflict pair.Validated empirically: 119 of 120 OvPhysX training rows completed on a benchmark image built with both extras in a single environment at
packaging==26.0(the one failure was unrelated). OvPhysX does not exercise anything that its<24cap protects.ovstays forked — it also pullsovrtx, which has not been validated alongside Isaac Sim. Scope is deliberately limited to the pair with evidence behind it.On the lock diff
It is large, and almost all of it is mechanical. Once
ovphysxstops being a fork dimension, uv rewrites resolution markers across every package, dropping theextra == '...-ovphysx'conjunctions.Exactly one package changes version:
Nothing is upgraded, downgraded, added, or removed. Note also that running
uv lockon unmodifieddevelopalready produces ~2570 lines of churn on its own, so a portion of this diff is pre-existing staleness rather than a consequence of this change.Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/isaaclab/changelog.d/test_uv_run_pyproject.py)uv sync --extra isaacsim --extra ovphysxresolves