Update MjWarp dependency #1295
Update MjWarp dependency #1295
Conversation
📝 WalkthroughWalkthroughUpdates pinned mujoco_warp commit in ASV config, tightens SolverMuJoCo logic to only override nconmax/njmax when explicit smaller values are passed, and increases nconmax values in two robot example files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/examples/robot/example_robot_anymal_d.py (1)
89-89: Increased nconmax to accommodate MjWarp requirements.The increase from 20 to 35 aligns with the updated MjWarp integration where explicit values must meet MjWarp's minimum requirements. The previous value of 20 would trigger a warning (lines 1951-1954 in solver_mujoco.py).
Consider using None for automatic sizing:
If the goal is to use MjWarp's automatic heuristic, you could omit
nconmaxentirely or set it toNone, allowing MjWarp to determine the optimal value:self.solver = newton.solvers.SolverMuJoCo( self.model, cone=mujoco.mjtCone.mjCONE_ELLIPTIC, impratio=100, iterations=100, ls_iterations=50, # nconmax=None, # Let MjWarp determine the default njmax=100, use_mujoco_contacts=args.use_mujoco_contacts if args else False, )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
asv.conf.jsonnewton/_src/solvers/mujoco/solver_mujoco.pynewton/examples/robot/example_robot_anymal_d.py
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, Example.use_mujoco is a high-level attribute that controls whether to use MuJoCo solver vs other solvers (like XPBD), while SolverMuJoCo.use_mujoco_cpu controls the backend within MuJoCo (CPU vs Warp). These are separate concepts serving different purposes - the PR rename only applies to the solver parameter, not the Example class attributes.
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, there's a distinction between solver parameters and Example class attributes. The Example class can have its own use_mujoco attribute for controlling example-level behavior (like CUDA graphs, rendering logic), while the solver uses use_mujoco_cpu for backend selection. These serve different purposes and should not be conflated during API renames.
Learnt from: Kenny-Vilella
Repo: newton-physics/newton PR: 398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, Example.use_mujoco is a high-level attribute that controls whether to use MuJoCo solver vs other solvers (like XPBD), while SolverMuJoCo.use_mujoco_cpu controls the backend within MuJoCo (CPU vs Warp). These are separate concepts serving different purposes - the PR rename only applies to the solver parameter, not the Example class attributes.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/examples/robot/example_robot_anymal_d.py
📚 Learning: 2025-07-14T03:57:29.670Z
Learnt from: Kenny-Vilella
Repo: newton-physics/newton PR: 398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-18T07:05:56.836Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/solve_rheology.py:969-986
Timestamp: 2025-09-18T07:05:56.836Z
Learning: In newton/_src/solvers/implicit_mpm/solve_rheology.py, transposed_strain_mat parameter cannot be None - the type signature was corrected to reflect this guarantee, eliminating the need for None checks when accessing transposed_strain_mat.offsets.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, there's a distinction between solver parameters and Example class attributes. The Example class can have its own use_mujoco attribute for controlling example-level behavior (like CUDA graphs, rendering logic), while the solver uses use_mujoco_cpu for backend selection. These serve different purposes and should not be conflated during API renames.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/examples/robot/example_robot_anymal_d.py
📚 Learning: 2025-12-12T08:45:43.428Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:277-287
Timestamp: 2025-12-12T08:45:43.428Z
Learning: In Newtown (Newton) example code, specifically files under newton/examples, computing contacts once per frame and reusing them across all substeps is an intentional design choice, not a bug. Reviewers should verify that contacts are computed before the substep loop and reused for every substep within the same frame. This pattern reduces redundant work and preserves frame-consistency; do not flag as a regression unless the behavior is changed for correctness or performance reasons.
Applied to files:
newton/examples/robot/example_robot_anymal_d.py
📚 Learning: 2025-08-27T23:33:32.527Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 660
File: pyproject.toml:28-28
Timestamp: 2025-08-27T23:33:32.527Z
Learning: In Python projects, it's normal and correct to have different warp-lang version specifications across files: pyproject.toml should specify minimum compatible versions using >= for flexibility, while specific environments like asv.conf.json can pin exact versions using == for reproducibility. These serve different purposes and should not be "aligned" to the same version.
Applied to files:
asv.conf.json
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (3)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
1948-1955: LGTM! Delegates nconmax default to MjWarp.The updated logic now only validates and warns when
nconmaxis explicitly provided and is smaller than MjWarp's computed value. WhennconmaxisNone, it's passed through tomujoco_warp.put_data(line 1968), allowing MjWarp to determine the default value. This aligns with the PR objective to "follow MjWarp by default."
1957-1962: LGTM! Delegates njmax default to MjWarp.Similar to
nconmax, the updated logic now only validates and warns whennjmaxis explicitly provided and is smaller than MjWarp's computed value. WhennjmaxisNone, it's passed through tomujoco_warp.put_data(line 1969), allowing MjWarp to determine the default value. This aligns with the PR objective to "follow MjWarp by default."asv.conf.json (1)
22-22: Verify the mujoco_warp commit is valid and compatible.The commit hash
24e07def301f3c073d8bc0f948831c5611737ff0is pinned in both asv.conf.json and uv.lock, suggesting the dependency was successfully resolved. However, the commit should be verified directly in the mujoco_warp repository to ensure it exists and doesn't introduce breaking changes with the current Newton codebase, particularly given the extensive mujoco_warp integration across test suites and the MuJoCo solver implementation.
|
The failing benchmark is an artifact because of a mismatch between MjWarp dep in the asv file and the njmax and nconmax heuristic. We are using the same asv file for both trunk and this branch instead of using their own asv file. |
…_mujoco_warp_dependency
|
change is looking good and I agree with using MjWarp defaults. Will approve once the benchmark failures are addressed/investigated. |
|
@adenzler-nvidia I think that the failing benchmarks are just artifacts. |
…_mujoco_warp_dependency
|
I can reproduce locally. The reason for the perf regression is that njmax changes from 0 to 64 with the new defaults. So to restore perf we could just set njmax to 0. The other question is whether that asset really does not have any constraints. Are there no joint limits? We explicitly disable contacts, but to me it seems like the proper thing would actually be to change the asset to make sure we have proper collision filtering. Either way I think it's both ok to merge like this or to go hardcode njmax to 0. I don't think we low a lot of signal in the benchmarks. The additional follow-up is whether the selection example and the standalone cartpole example should somehow be merged to avoid these 2 diverging all the time. |
|
@adenzler-nvidia For the Cartpole issue, are you sure that you purged the asv/env folder between the two runs?
I will see the regression. In those cases, I always run on two different fresh clones to be sure there is no clash at all. On proper main njmax is actually equal to 5, which makes more sense than 0. It's actually worse than that, IsaacLab also use another Cartpole, one has a single pendulum and the other a double pendulum. |
adenzler-nvidia
left a comment
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Can confirm, with a fresh env folder I'm getting the same numbers for both branches. Let's merge this.
3 participants
Description
Updated MjWarp dependency
Updated njmax and nconmax heuristic to follow MjWarp by default.
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.