[Workflow] Default locomotion core tasks to Newton - #7064
Conversation
This reverts commit deae63c.
Greptile SummaryThe PR changes the Ant, Humanoid, and Velocity task defaults to Newton MJWarp and consolidates core environment smoke testing across explicit physics presets.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "Revert "Split environment smoke tests by..." | Re-trigger Greptile |
| # Local imports should be imported last | ||
| from env_test_utils import _run_environments, setup_environment # isort: skip | ||
|
|
||
| @pytest.mark.parametrize("physics_preset_name", ["newton_mjwarp", "physx", "isaacsim_physx"]) |
There was a problem hiding this comment.
Matrix silently uses default backend
The matrix applies every backend name to every core task without checking that the preset exists. For tasks such as Fourbar Pole, which only exposes newton_kamino, the newton_mjwarp, physx, and isaacsim_physx cases silently fall back to Kamino, so passing tests report backend coverage they did not exercise.
Knowledge Base Used: isaaclab_tasks: Task Registration and Organization
There was a problem hiding this comment.
Fixed in 52e373f: the smoke matrix now derives only task/backend pairs whose raw config declares the requested PhysicsCfg preset, so unsupported Fourbar combinations are omitted rather than falling back.
There was a problem hiding this comment.
Isaac Lab Review Bot
The review traced the Newton MJWarp default changes across Ant, Humanoid, and velocity configurations, along with the consolidated three-backend smoke-test matrix and teardown cleanup. The proposed Play-environment finding is not supported as a defect because the helper’s filter removal is explicit and the patch does not establish that the newly collected Play variants are redundant.
- Design and architecture: Physics defaults are consistently redirected to existing
newton_mjwarppresets while retaining explicit PhysX, OvPhysX, and Newton Kamino selections. Manager-based Ant and Humanoid observation defaults are aligned with the selected backend. The shared rough-terrain default intentionally affects all configurations derived fromRoughPhysicsCfg, which remains the principal breadth-of-impact consideration. - API: The existing physics preset names remain available for explicit selection, and the changelog identifies the default-backend change as breaking with migration guidance. The removed
include_playandnewton_mjwarp_envsparameters belong to the test utility surface, and the supplied patch updates the shown callers consistently. - Implementation: The deleted Newton-specific test module is replaced by a single core test parametrized over
newton_mjwarp,physx, andisaacsim_physx; the workflow cache-warmer input is updated accordingly. Removing the Play filter broadens test collection, but the evidence does not demonstrate that this intended coverage is materially redundant or erroneous. The addedgc.collect()runs before simulation-context teardown, matching its stated Warp finalizer ordering requirement.
No blocking issues. No inline issue met the actionable-evidence threshold; the assessment above records the review feedback.
Automated review; human maintainers own approval decisions.
AntoineRichard
left a comment
There was a problem hiding this comment.
LGTM. We may need to communicate that to QA. Also should update the environment docs? It may list the defaults? (I'm not sure) LGTM otherwise.
|
@StafaH Could you update the AnyMal-D visualizer test factory to select the explicit In preset_key = "newton_mjwarp" if backend_kind == "newton" else "default"This PR changes the AnyMal-D Please make the PhysX branch resolve the named |
|
@StafaH The AnyMal-D determinism failures should also be fixed by keeping these existing tests on explicit PhysX.
Could you pass |
|
@StafaH One more issue with the consolidated backend matrix in Global preset resolution falls back to Could you generate only valid The removal of the Play-task filter also collects the Play variants in addition to their base tasks and substantially expands this already tripled matrix. The third task shard was still running after roughly 80 minutes. Unless separate Play coverage is intentional here, please restore the Play filter to avoid duplicate environment smoke coverage. |
|
Addressed the remaining review findings in 52e373f: the core smoke matrix now filters to PhysicsCfg presets actually declared by each task; AnyMal-D visualizer tests select explicit |
…locomotion # Conflicts: # .github/workflows/build.yaml # source/isaaclab_tasks/test/contrib/test_cartpole_showcase_environments_with_stage_in_memory.py # source/isaaclab_tasks/test/contrib/test_contrib_environments_smoke.py # source/isaaclab_tasks/test/contrib/test_pickplace_stack_environments.py # source/isaaclab_tasks/test/contrib/test_teleop_environments.py # source/isaaclab_tasks/test/contrib/test_teleop_environments_with_stage_in_memory.py # source/isaaclab_tasks/test/core/test_environments_isaacsim_physx.py # source/isaaclab_tasks/test/core/test_environments_newton.py # source/isaaclab_tasks/test/core/test_environments_ovphysx.py # source/isaaclab_tasks/test/core/test_environments_with_stage_in_memory.py # source/isaaclab_tasks/test/core/test_record_video.py # source/isaaclab_tasks/test/env_test_utils.py
Splits the Ant, Humanoid, and Velocity Newton defaults from #6980.\n\n- Defaults Ant, Humanoid, and Velocity task configurations to Newton MJWarp.\n- Includes the shared three-backend core smoke-test matrix and teardown reliability cleanup.\n- Removes obsolete Play/MJWarp-discovery test utility logic.\n\nThe shared test commit is intentionally identical to the other environment shards; rebase/drop it after the first shard merges.\n\nSupersedes the locomotion portion of #6980.