Switch MuJoCo examples and Benchmarks to Parallel Linesearch#738
Merged
adenzler-nvidia merged 8 commits intoSep 8, 2025
Merged
Conversation
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds a per-robot Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI
participant Example
participant SolverMuJoCo
participant MuJoCoModel
CLI->>Example: parse args (includes --ls-parallel)
Note right of Example: choose ls_parallel = CLI value if set else ROBOT_CONFIGS[robot]["ls_parallel"]
Example->>SolverMuJoCo: init(..., ls_parallel=chosen_value)
SolverMuJoCo->>MuJoCoModel: create_model_builder(ls_parallel=chosen_value)
Note right of MuJoCoModel: set opt.ls_parallel = chosen_value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
📜 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)
✨ Finishing Touches
🧪 Generate unit tests
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 |
eric-heiden
approved these changes
Sep 4, 2025
Member
|
The benchmarks show a noticeable runtime increase. Is this acceptable for the low env counts we are benchmarking? |
Member
Author
|
something is off looking at the benchmarks, will look at it. Thanks for reminding me! |
Member
Author
|
I think we should defer this investigation. I found out what happens but couldn't figure out how to fix it - might also be an issue in MjWarp. Given that this is working for the real robots, I suggest to go ahead and follow up in that ticket. |
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/examples/example_mujoco.py (1)
240-251: Fix undefined variable and wire CLI correctly; also avoidtype=boolpitfall.
ls_parallelis referenced before definition (Line 240). The CLI flag is parsed but never passed toExample, andtype=boolwill misparse strings. Wire the flag through and switch toBooleanOptionalAction.Apply this diff:
@@ def __init__( self, robot="humanoid", stage_path=None, num_envs=1, use_cuda_graph=True, use_mujoco_cpu=False, randomize=False, headless=False, actuation="None", solver=None, integrator=None, solver_iteration=None, ls_iteration=None, njmax=None, nconmax=None, + ls_parallel=None, builder=None, ): @@ - ls_parallel = ls_parallel if ls_parallel is not None else ROBOT_CONFIGS[robot]["ls_parallel"] + ls_parallel = ls_parallel if ls_parallel is not None else ROBOT_CONFIGS[robot]["ls_parallel"] @@ - parser.add_argument("--ls-parallel", type=bool, default=None, help="Use parallel line search.") + parser.add_argument( + "--ls-parallel", + default=None, + action=argparse.BooleanOptionalAction, + help="Use parallel line search.", + ) @@ example = Example( robot=args.robot, stage_path=args.stage_path, num_envs=args.num_envs, use_cuda_graph=args.use_cuda_graph, use_mujoco_cpu=args.use_mujoco_cpu, randomize=args.random_init, headless=args.headless, actuation=args.actuation, solver=args.solver, integrator=args.integrator, solver_iteration=args.solver_iteration, ls_iteration=args.ls_iteration, njmax=args.njmax, nconmax=args.nconmax, + ls_parallel=args.ls_parallel, )Also applies to: 383-384, 391-407
🧹 Nitpick comments (1)
newton/examples/example_mujoco.py (1)
438-446: Optional: print actual ls_parallel in the run summary.Adds quick visibility to the effective setting and avoids confusion across CPU/Warp paths.
@@ print(f"{'Line Search Iterations':<{LABEL_WIDTH}}: {example.solver.mj_model.opt.ls_iterations}") print(f"{'Max Constraints / env':<{LABEL_WIDTH}}: {actual_njmax}") print(f"{'Max Contacts / env':<{LABEL_WIDTH}}: {actual_nconmax}") + # Prefer Warp option; fall back to mj_model if present + lspar = ( + getattr(getattr(getattr(example.solver, 'mjw_model', None), 'opt', None), 'ls_parallel', None) + if hasattr(example.solver, 'mjw_model') else None + ) + if lspar is None: + lspar = getattr(getattr(example.solver.mj_model, 'opt', object()), 'ls_parallel', 'n/a') + print(f"{'Parallel Linesearch':<{LABEL_WIDTH}}: {lspar}") print(f"{'Joint DOFs':<{LABEL_WIDTH}}: {example.model.joint_dof_count}") print(f"{'Body Count':<{LABEL_WIDTH}}: {example.model.body_count}")
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/examples/example_mujoco.py(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: adenzler-nvidia
PR: newton-physics/newton#479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: In the Newton MuJoCo solver implementation, the ls_parallel option may not be available in the pure MuJoCo mjOption structure, despite being documented in newer MuJoCo versions. The user adenzler-nvidia confirmed that mj_model does not have this attribute in their implementation, so ls_parallel should only be set on mjw_model.opt.
Learnt from: adenzler-nvidia
PR: newton-physics/newton#479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: The ls_parallel option is specific to MuJoCo Warp and is not available in the pure MuJoCo mjOption structure. It should only be set on mjw_model.opt, not mj_model.opt.
📚 Learning: 2025-07-28T15:04:54.360Z
Learnt from: adenzler-nvidia
PR: newton-physics/newton#479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: In the Newton MuJoCo solver implementation, the ls_parallel option may not be available in the pure MuJoCo mjOption structure, despite being documented in newer MuJoCo versions. The user adenzler-nvidia confirmed that mj_model does not have this attribute in their implementation, so ls_parallel should only be set on mjw_model.opt.
Applied to files:
newton/examples/example_mujoco.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
PR: newton-physics/newton#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/examples/example_mujoco.py
🪛 Ruff (0.12.2)
newton/examples/example_mujoco.py
240-240: Undefined name ls_parallel
(F821)
240-240: Undefined name ls_parallel
(F821)
⏰ 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)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (2)
newton/examples/example_mujoco.py (2)
44-44: Confirm per-robotls_paralleldefaults
Defaults are False for humanoid and True for all other robots; ensure this matches IsaacLab guidelines and expected benchmark performance at low environment counts.
241-251: TheSolverMuJoCo.__init__signature does include anls_parallelparameter and it only applies it onmjw_model.opt, notmj_model.opt. No changes needed.
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
newton/examples/example_mujoco.py (2)
209-210: Expose ls_parallel on the Example instanceUseful for introspection and summaries; store it on self.
Apply:
class Example: @@ - ls_parallel=None, + ls_parallel=None,And in the body (see next comment) set self.ls_parallel once.
241-252: Store ls_parallel on self and pass that into the solverThis avoids recomputing and makes the setting accessible.
- ls_parallel = ls_parallel if ls_parallel is not None else ROBOT_CONFIGS[robot]["ls_parallel"] + self.ls_parallel = ls_parallel if ls_parallel is not None else ROBOT_CONFIGS[robot]["ls_parallel"] self.solver = newton.solvers.SolverMuJoCo( @@ - ls_parallel=ls_parallel, + ls_parallel=self.ls_parallel, )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/examples/example_mujoco.py(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: adenzler-nvidia
PR: newton-physics/newton#479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: In the Newton MuJoCo solver implementation, the ls_parallel option may not be available in the pure MuJoCo mjOption structure, despite being documented in newer MuJoCo versions. The user adenzler-nvidia confirmed that mj_model does not have this attribute in their implementation, so ls_parallel should only be set on mjw_model.opt.
Learnt from: adenzler-nvidia
PR: newton-physics/newton#479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: The ls_parallel option is specific to MuJoCo Warp and is not available in the pure MuJoCo mjOption structure. It should only be set on mjw_model.opt, not mj_model.opt.
📚 Learning: 2025-07-28T15:04:54.360Z
Learnt from: adenzler-nvidia
PR: newton-physics/newton#479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: In the Newton MuJoCo solver implementation, the ls_parallel option may not be available in the pure MuJoCo mjOption structure, despite being documented in newer MuJoCo versions. The user adenzler-nvidia confirmed that mj_model does not have this attribute in their implementation, so ls_parallel should only be set on mjw_model.opt.
Applied to files:
newton/examples/example_mujoco.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
PR: newton-physics/newton#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/examples/example_mujoco.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
PR: newton-physics/newton#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/examples/example_mujoco.py
🔇 Additional comments (3)
newton/examples/example_mujoco.py (3)
44-44: Per-robot ls_parallel defaults look goodDefaults align with the PR intent and IsaacLab practice. No issues spotted.
Also applies to: 51-51, 58-58, 65-65, 72-72, 79-79
409-409: CLI plumbing LGTMThe CLI flag is correctly forwarded into Example(...).
242-252: Resolved: SolverMuJoCo init supports ls_parallel Confirmed the constructor signature defines anls_parallelparameter (default False) and applies it tomjw_model.opt.ls_parallel.
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
4 tasks
4 tasks
eric-heiden
pushed a commit
to eric-heiden/newton
that referenced
this pull request
Jan 28, 2026
…physics#738) Signed-off-by: Alain Denzler <adenzler@nvidia.com> Co-authored-by: Eric Shi <97630937+shi-eric@users.noreply.github.com>
4 tasks
mmacklin
pushed a commit
to mmacklin/newton
that referenced
this pull request
Apr 7, 2026
…physics#738) Signed-off-by: Alain Denzler <adenzler@nvidia.com> Co-authored-by: Eric Shi <97630937+shi-eric@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First step towards aligning the benchmarks with IsaacLab configs. There, parallel linesearch is enabled everywhere.
Before your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
New Features
Documentation
Notes