[WIP] Onboard new assets from Mujoco Menagerie#5330
[WIP] Onboard new assets from Mujoco Menagerie#5330matthewtrepte wants to merge 3 commits intoisaac-sim:developfrom
Conversation
matthewtrepte
requested review from
Mayankm96,
hhansen-bdai,
jtigue-bdai,
kellyguo11,
ooctipus and
pascal-roth
as code owners
Greptile SummaryThis PR onboards several robot assets (ANYmal-B/C, Spot, Allegro, Shadow Hand, Unitree H1/G1/Go2) from the MuJoCo Menagerie USD collection, replacing legacy Nucleus paths. It introduces a
Confidence Score: 4/5Safe to merge after fixing the distributed training flag typo; all other findings are style/robustness suggestions. One P1 bug (--nnprod_per_node typo) would silently break any multi-GPU benchmark run; remaining issues are P2 quality suggestions. Core asset-path switching and variant-injection logic are well-structured. source/isaaclab_tasks/test/benchmarking/test_environments_training.py (distributed flag typo), source/isaaclab/isaaclab/utils/assets.py (case-sensitive path match, unhandled ValueError) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[train.py / sim_launcher] -->|--menagerie-physics-variant| B[apply_menagerie_physics_variant_from_launcher_args]
B -->|writes| C[ISAACLAB_MUJOCO_MENAGERIE_PHYSICS_VARIANT env var]
D[spawn_from_usd] --> E[_spawn_from_usd_file]
E --> F[merge_mujoco_menagerie_spawn_variants]
F --> G{is_mujoco_menagerie_usd_path?}
G -- No --> H[return cfg.variants unchanged]
G -- Yes --> I[resolve_mujoco_menagerie_physics_variant_selection]
I --> J{env var value}
J -- auto --> K[infer_menagerie_physics_variant_from_argv]
K -->|presets=newton in argv?| L{newton?}
L -- yes --> M[variant = mujoco]
L -- no --> N[variant = physx]
J -- physx/mujoco --> O[use explicit value]
J -- none --> P[skip variant injection]
J -- invalid --> Q[raise ValueError]
M & N & O --> R[select_usd_variants on prim]
C -.->|read by| I
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| cmd.append(f"--nnprod_per_node={num_gpus}") | ||
| cmd.append("--distributed") |
There was a problem hiding this comment.
--nnprod_per_node is not a recognized flag in any of the RL training scripts; they all use parse_known_args(), so this argument is silently swallowed without ever setting the number of workers. Distributed training benchmarks will launch as single-process runs rather than multi-GPU, producing wrong timings and potentially wrong KPI numbers.
| cmd.append(f"--nnprod_per_node={num_gpus}") | |
| cmd.append("--distributed") | |
| cmd.append(f"--nproc_per_node={num_gpus}") |
| def resolve_mujoco_menagerie_physics_variant_selection() -> str | None: | ||
| """Resolve the ``Physics`` variant selection string, or None to skip variant application.""" | ||
| raw = os.environ.get(ISAACLAB_MUJOCO_MENAGERIE_PHYSICS_VARIANT_ENV, "auto") | ||
| mode = raw.strip().lower() | ||
| if mode in ("", "auto"): | ||
| return infer_menagerie_physics_variant_from_argv() | ||
| if mode == "none": | ||
| return None | ||
| if mode in (MUJOCO_MENAGERIE_PHYSICS_VARIANT_PHYSX, MUJOCO_MENAGERIE_PHYSICS_VARIANT_MUJOCO): | ||
| return mode | ||
| raise ValueError( | ||
| f"Invalid {ISAACLAB_MUJOCO_MENAGERIE_PHYSICS_VARIANT_ENV}={raw!r}. " | ||
| f"Expected 'auto', 'none', '{MUJOCO_MENAGERIE_PHYSICS_VARIANT_PHYSX}', or " | ||
| f"'{MUJOCO_MENAGERIE_PHYSICS_VARIANT_MUJOCO}'." | ||
| ) |
There was a problem hiding this comment.
ValueError from invalid env var propagates through USD spawn
resolve_mujoco_menagerie_physics_variant_selection raises ValueError when ISAACLAB_MUJOCO_MENAGERIE_PHYSICS_VARIANT is set to an invalid value. This exception is called from merge_mujoco_menagerie_spawn_variants, which is called in the middle of _spawn_from_usd_file — after the prim has already been created in the stage. The resulting stack trace won't clearly indicate the root cause (an env var) to the user.
Consider catching ValueError in merge_mujoco_menagerie_spawn_variants (or in _spawn_from_usd_file) and re-raising with a more actionable message, or validating the env var eagerly at process startup (e.g. in apply_menagerie_physics_variant_from_launcher_args).
| def is_mujoco_menagerie_usd_path(usd_path: str) -> bool: | ||
| """True if *usd_path* points at assets under the MuJoCo Menagerie tree (Nucleus or downloaded).""" | ||
| norm = usd_path.replace("\\", "/") | ||
| return "Mujoco_Menagerie" in norm |
There was a problem hiding this comment.
"Mujoco_Menagerie" in norm is case-sensitive after only normalizing backslashes. A locally downloaded path, a symlink, or a Nucleus server that uses a different casing (mujoco_menagerie, MuJoCo_Menagerie) would return False and never receive the Physics variant selection. Using a case-insensitive check would be more robust.
| def is_mujoco_menagerie_usd_path(usd_path: str) -> bool: | |
| """True if *usd_path* points at assets under the MuJoCo Menagerie tree (Nucleus or downloaded).""" | |
| norm = usd_path.replace("\\", "/") | |
| return "Mujoco_Menagerie" in norm | |
| return "mujoco_menagerie" in norm.lower() |
1 participant
Description
Working change to benchmark and onboard new robot assets from Mujoco Menagerie
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there