-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[Newton] Drop explicit mujoco/mujoco-warp pins, defer to newton[sim] #5566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6dd489f
48018cb
4a50282
02b1456
de295cc
1c394ff
e35394a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| Removed | ||
| ^^^^^^^ | ||
|
|
||
| * Removed explicit ``mujoco`` and ``mujoco-warp`` dependencies from | ||
| :mod:`isaaclab`. These packages are not used by ``isaaclab`` core and are | ||
| now resolved transitively through Newton's ``[sim]`` extra in | ||
| :mod:`isaaclab_newton`. Users installing only the PhysX or Kit backends no | ||
| longer pull in MuJoCo. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| Changed | ||
| ^^^^^^^ | ||
|
|
||
| * Switched the Newton install to ``newton[sim]`` so that ``mujoco`` and | ||
| ``mujoco-warp`` are pulled in transitively via Newton's ``[sim]`` extra. | ||
| The explicit ``mujoco==3.8.0`` and ``mujoco-warp==3.8.0.1`` pins were | ||
| removed from :mod:`isaaclab_newton` — Newton is now the single source of | ||
| truth for those versions. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| Changed | ||
| ^^^^^^^ | ||
|
|
||
| * Switched the Newton install spec to ``newton[sim]`` in the ``newton`` | ||
| extra so the MuJoCo solver dependencies are pulled in transitively. | ||
| Required because pip resolves a git-URL requirement once for the URL; | ||
| a bare ``newton @ git+...`` here would shadow the ``[sim]`` extra | ||
| requested elsewhere. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| Changed | ||
| ^^^^^^^ | ||
|
|
||
| * Switched the Newton install spec to ``newton[sim]`` in the ``newton``, | ||
| ``rerun``, and ``viser`` extras so the MuJoCo solver dependencies are | ||
| pulled in transitively. Required because pip resolves a git-URL | ||
| requirement once for the URL; a bare ``newton @ git+...`` here would | ||
| shadow the ``[sim]`` extra requested elsewhere. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,20 +13,27 @@ | |
| "numpy", | ||
| ] | ||
|
|
||
| # Every Newton declaration in the repo must use the SAME extra spec (`newton[sim]`). | ||
| # Pip resolves a git-URL requirement once per URL: if any package declares bare | ||
| # `newton @ git+...` while another declares `newton[sim] @ git+...`, the first | ||
| # resolution wins and silently drops the `[sim]` extra. That breaks `isaaclab_newton` | ||
| # at import time because `mujoco` / `mujoco-warp` go missing. So even the rerun/viser | ||
| # extras — which don't use the MuJoCo solver directly — must pin `newton[sim]` to | ||
| # stay consistent with `isaaclab_newton`. | ||
| EXTRAS_REQUIRE = { | ||
| "kit": [], | ||
| "newton": [ | ||
| "warp-lang", | ||
| "newton==1.2.0rc3", | ||
| "newton[sim] @ git+https://github.com/newton-physics/newton.git@v1.2.0rc2", | ||
| "PyOpenGL-accelerate", | ||
| "imgui-bundle>=1.92.5", | ||
| ], | ||
| "rerun": [ | ||
| "newton==1.2.0rc3", | ||
| "newton[sim] @ git+https://github.com/newton-physics/newton.git@v1.2.0rc2", | ||
| "rerun-sdk>=0.29.0", | ||
| ], | ||
| "viser": [ | ||
| "newton==1.2.0rc3", | ||
| "newton[sim] @ git+https://github.com/newton-physics/newton.git@v1.2.0rc2", | ||
| "viser>=1.0.16", | ||
| ], | ||
|
Comment on lines
+32
to
38
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Before this PR the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a block comment above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL about pip deduplicating git-URL requirements — that explains the necessity well. The added comment should save future readers the same question. 👍 |
||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous explicit pins (
mujoco==3.8.0,mujoco-warp==3.8.0.1) guaranteed exact reproducibility. Newton's[sim]extra specifiesmujoco~=3.8.0(compatible-release, i.e.>=3.8.0,<3.9.0) andmujoco-warp>=3.8.0.1,~=3.8.0, so a fresh install after a new3.8.xrelease could resolve a different patch version than was tested. Since Newton is pinned to an immutable tag (v1.2.0rc2) the constraint itself won't change, but pip will still pick the latest matching patch from PyPI at install time. Worth confirming the team is comfortable trading bit-for-bit reproducibility for deferred version management.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!
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By design — the goal of this change is for Newton to be the single source of truth for these versions, so the loosening from
==3.8.0to~=3.8.0is the trade-off we accept (called out in the PR description). If a specific 3.8.x patch turns out to be incompatible, the right fix is to tighten Newton's own[sim]spec rather than override it here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense — single source of truth in Newton with
~=as the compatibility range is a clean approach. Agreed that tightening upstream is the right lever if a patch breaks things.