Fix Newton preset validation#5939
Conversation
Require selected Newton solver presets to match an active physics preset during Hydra resolution. This prevents tasks that only expose non-physics presets with the same name from silently creating mixed PhysX/Newton configs.
github-actions
Bot
added
bug
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR adds post-resolution validation to ensure that when a user selects a Newton physics preset (e.g., presets=newton_mjwarp), it actually matched an active PhysicsCfg field in the resolved task. Without this fix, tasks that expose non-physics presets with the same name (e.g., scalar actuator or sensor variants) could silently create mixed PhysX/Newton configurations — a subtle and dangerous silent failure.
Overall: Clean, well-scoped fix that addresses an important correctness issue. The approach of tracking consumed targets alongside consumed names is sound.
✅ Strengths
- Minimal invasive change. The fix threads
consumed_selected_targetsthrough the existing resolution machinery without restructuring the call graph. This is the right approach for a targeted bug fix. - Clear error messaging. The
ValueErrorraised by_validate_selected_newton_physics_presetsgives actionable guidance — it tells users what failed, why, and what to do about it. - Defensive design. Uses
setdefaultand set operations, so multiple preset resolution passes accumulate targets correctly without duplication issues.
💡 Suggestions
-
_preset_targetsiterates allPresetTargetmembers (line ~79) — this is fine for 3 members, but if the enum grows, consider caching per-type results or short-circuiting after the first match if targets are expected to be mutually exclusive. Low priority given current enum size. -
Validation only covers
PresetTarget.PHYSICS— ifRENDERERgains similar silent-failure potential in the future, the same pattern could apply there. Worth a# TODOcomment in_validate_selected_newton_physics_presetsto signal this is extensible, but not blocking. -
No regression test added. The PR notes "No tests added by request; verified with direct config-resolution probes" and the checklist mentions existing
test_hydra.pypasses (78 tests). However, given this bug's silent nature, a targeted test that asserts theValueErroris raised when a Newton preset is selected for a task without physics support would be valuable regression protection. Even a single parametrized case (e.g., mock task with only domain presets +presets=newton_mjwarp→ValueError) would prevent future regressions. -
Minor: redundant
valassignment movement (lines 319/325). The refactor movesval = fields[name]up and removes the laterfields[name]reference — this is correct and slightly more efficient, but the diff could be confusing to reviewers since it looks like a behavior change. The net effect is identical (just avoids a second dict lookup). No action needed, just noting for context.
⚠️ Potential Edge Case
The validation in _selected_newton_physics_presets uses PresetTarget.PHYSICS.legacy_aliases to determine the canonical Newton preset names:
newton_names = set(aliases.values())
return {aliases.get(name, name) for name in selected if aliases.get(name, name) in newton_names}This means only presets whose canonical name appears in the alias values are considered Newton physics presets. If a new Newton solver is added in the future (e.g., newton_foo) without adding it to legacy_aliases, it would bypass this validation. Consider whether the validation should instead check membership against a dedicated registry or naming convention (e.g., name.startswith("newton_")), though this depends on how solver presets are expected to evolve.
CI Status
All checks are still pending at time of review. No pre-commit or changelog issues detected from the passed checks.
Verdict: Approve-worthy once CI passes. The missing regression test is the only notable gap — would recommend adding one before merge if feasible.
🔄 Update (re-review of 6971203)
All previous concerns addressed:
- ✅ Dedicated registry introduced —
_NEWTON_PHYSICS_PRESETSmodule-level constant cleanly separates preset identity from legacy alias resolution. New Newton solvers just need to be added to this set. - ✅ Validation order fixed —
consumed_selectedgate ensures Newton validation only fires for presets that were actually consumed (not truly unknown ones), eliminating the misleading error ordering issue. - ✅ Cleaner comprehension —
_selected_newton_physics_presetsrefactored for clarity.
No new issues found. LGTM. 👍
Greptile SummaryThis PR adds validation to prevent silent mixed PhysX/Newton configurations when a user selects
Confidence Score: 4/5Safe to merge; the core fix correctly prevents mixed PhysX/Newton configs and the 78-test suite passes, but two design rough edges in the validation helper are worth addressing. The targeted fix works correctly for the described scenarios: tasks with only scalar Newton presets now raise, while locomotion tasks with a real PhysicsCfg Newton entry continue to resolve. Two non-blocking concerns remain: the Newton validator runs before the unknown-preset check, so a completely-unrecognised Newton name produces a misleading error message; and the set of Newton physics preset names is derived from legacy_aliases.values() rather than a dedicated registry, meaning future Newton variants added without a legacy alias entry would silently bypass the check. source/isaaclab_tasks/isaaclab_tasks/utils/hydra.py — specifically _selected_newton_physics_presets and the ordering of _validate_selected_newton_physics_presets relative to the unknown_presets block. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["register_task()\nglobal_presets, consumed_preset_targets = {}"] --> B["_resolve_active_presets(env_cfg)\nthreads consumed_selected_targets"]
B --> C["_pick_alternative(preset_obj, selected)"]
C --> D{{"preset name\nmatches field?"}}
D -- yes --> E["_preset_targets(val)\nisinstance check vs PhysicsCfg / RendererCfg"]
E --> F{{"targets\nnon-empty?"}}
F -- yes --> G["consumed_selected_targets[name].update(targets)"]
F -- no --> H["consumed_selected only\n(scalar preset, no typed target)"]
G --> I["resolve continues"]
H --> I
D -- no --> I
I --> J["_resolve_active_presets(agent_cfg)"]
J --> K["_validate_selected_newton_physics_presets"]
K --> L{{"newton preset\nhas PHYSICS target?"}}
L -- yes --> M["✓ valid"]
L -- no --> N["✗ ValueError"]
K --> O["unknown_presets check"]
Reviews (1): Last reviewed commit: "Fix Newton preset validation" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| missing = sorted( | ||
| name | ||
| for name in _selected_newton_physics_presets(selected) | ||
| if PresetTarget.PHYSICS not in consumed_selected_targets.get(name, set()) | ||
| ) | ||
| if missing: | ||
| raise ValueError( | ||
| f"Selected Newton physics preset(s) {', '.join(missing)} did not match any active physics preset. " | ||
| "This task does not declare Newton physics support for the selected solver. " | ||
| "Remove the Newton preset or choose a task that declares it on a PhysicsCfg." | ||
| ) |
There was a problem hiding this comment.
_validate_selected_newton_physics_presets runs before the unknown_presets block (line 634). When a task has no newton_mjwarp entry whatsoever — not even a scalar actuator preset — consumed_preset_targets contains no entry for that name, so the Newton validator raises "did not match any active physics preset." The unknown_presets check, which would emit the clearer "Unknown preset(s): newton_mjwarp" message, never runs.
Adding a guard if name in consumed_presets inside the missing comprehension restricts the Newton error to the intended case (preset was applied somewhere but never to a PhysicsCfg), leaving truly-unknown names for the existing unknown-preset error path.
There was a problem hiding this comment.
Fixed in 6971203. The validator now only raises for Newton physics preset names that were consumed somewhere but did not resolve through a PHYSICS target, so truly unknown names continue to use the existing Unknown preset(s) error path.
There was a problem hiding this comment.
Nice — that distinction (consumed-but-not-resolved via PHYSICS vs truly-unknown) cleanly addresses the concern. 👍
| def _selected_newton_physics_presets(selected: list[str]) -> set[str]: | ||
| """Return canonical Newton physics preset names requested by the user.""" | ||
| aliases = PresetTarget.PHYSICS.legacy_aliases | ||
| newton_names = set(aliases.values()) | ||
| return {aliases.get(name, name) for name in selected if aliases.get(name, name) in newton_names} |
There was a problem hiding this comment.
legacy_aliases values, not on a dedicated registry
newton_names = set(aliases.values()) — i.e. {"newton_mjwarp", "newton_kamino"} — is derived from the deprecation-rename table in PresetTarget.PHYSICS.legacy_aliases, whose purpose is tracking old-to-new renames, not listing every valid Newton preset. If a future Newton variant (e.g. newton_tgs) is added directly to task configs without a corresponding legacy alias entry, _selected_newton_physics_presets will silently omit it from the check and the mixed-config bug will resurface without warning. A brief inline comment explaining why aliases.values() is the right source of truth here would protect future contributors from this non-obvious assumption.
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!
There was a problem hiding this comment.
Fixed in 6971203. Replaced legacy_aliases.values() as the source of truth with an explicit _NEWTON_PHYSICS_PRESETS registry; aliases are now only used to canonicalize legacy names. This keeps names such as newton_renderer out of physics validation and makes future solver additions explicit.
There was a problem hiding this comment.
Excellent — an explicit registry is exactly what this needed. Much cleaner separation between "canonicalize legacy names" and "what counts as a physics preset". 👍
Keep unknown Newton-like preset names on the existing unknown-preset error path and use an explicit Newton physics preset registry instead of deriving solver names from legacy aliases.
2 participants
Description
This fixes Newton solver preset validation during Hydra config resolution.
When a user selects
presets=newton_mjwarporpresets=newton_kamino, the selected solver now has to match an activePhysicsCfgpreset. This prevents tasks that only expose non-physics presets with the same name, such as scalar actuator or sensor variants, from silently creating mixed PhysX/Newton configurations.Fixes #
Type of change
Screenshots
Not applicable.
Checklist
pre-commitchecks with./isaaclab.sh --format./isaaclab.sh -fwas attempted twice, but the local environment is missinggit-lfs, socheck Git LFS pointersfails before it can run.SKIP=check-git-lfs-pointers ./isaaclab.sh -fpasses.source/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists thereVerification
./isaaclab.sh -p -m pytest source/isaaclab_tasks/test/core/test_hydra.py -q-> 78 passedIsaac-Navigation-Flat-Anymal-C-v0withpresets=newton_mjwarpraisesSelected Newton physics preset(s) newton_mjwarp did not match any active physics preset.Isaac-Velocity-Flat-Anymal-C-v0,Isaac-Velocity-Rough-Anymal-C-v0,Isaac-Velocity-Flat-Anymal-D-v0, andIsaac-Velocity-Rough-Unitree-Go2-v0still resolvepresets=newton_mjwarptoNewtonCfg../isaaclab.sh -f-> failed only becausegit-lfsis not installed in this environment.SKIP=check-git-lfs-pointers ./isaaclab.sh -f-> passed.