[Workflow] Default manipulation core tasks to Newton - #7065
Conversation
This reverts commit 847ebce.
Greptile SummaryThe PR changes Cabinet, Lift, and Kuka Allegro task defaults from Isaac Sim PhysX to Newton MJWarp and consolidates core environment smoke testing into a shared three-backend matrix.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains in the eligible follow-up review scope. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Registry[Registered core task] --> Config[Load environment configuration]
Config --> Presets[Collect supported physics presets]
Presets --> Newton[Newton MJWarp]
Presets --> AutoPhysX[Automatic PhysX]
Presets --> IsaacPhysX[Isaac Sim PhysX]
Newton --> Smoke[Core environment smoke test]
AutoPhysX --> Smoke
IsaacPhysX --> Smoke
Smoke --> Close[Close environment]
Close --> GC[Collect device-backed objects]
GC --> Teardown[Clear simulation context]
Reviews (2): Last reviewed commit: "Revert "Split environment smoke tests by..." | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
Reviewed the Newton MJWarp default switch for Cabinet, Lift, and Kuka Allegro, together with the consolidated three-backend core smoke-test matrix and teardown cleanup. The changed defaults remain aligned with their related timestep and decimation settings, and explicit PhysX selections remain available.
- Design and architecture: Cabinet changes its simulation and decimation defaults together, preserving a 60 Hz policy rate with a 1/600 s timestep and decimation of 10. Lift and Kuka Allegro consistently reference their existing Newton MJWarp presets. The separate Newton test path is folded into the shared core matrix; this intentionally broadens Newton execution from discovered MJWarp-capable tasks to all core tasks, which is a non-blocking CI coverage and runtime tradeoff.
- API: The user-visible default change is documented as breaking with guidance to select an explicit physics preset to retain PhysX. Existing named presets—including
newton_mjwarp,physx, andisaacsim_physx—remain available, and the Kuka Allegro default references a declared preset. - Implementation: Traced preset definitions and consumers through Cabinet, Lift, Kuka Allegro, preset-invariant tests, smoke-test parametrization, and the warp-cache workflow update. The removed Newton-specific test file is replaced by the shared three-preset matrix. The added garbage collection occurs during test teardown before clearing the simulation context; its effectiveness depends on what
env.close()releases while the local reference remains, but this does not establish a merge-blocking defect.
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.
| 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.
This parametrizes every core task with all three global preset names, but preset resolution falls back to default when a requested preset is unavailable. Consequently, a case labeled physx can actually run the newly-default Newton backend (or another task default), so the matrix does not guarantee the backend it claims to cover. Please generate only valid (task, physics preset) pairs, or use an explicit strict physics selector that errors when the backend is unavailable.
This also removes include_play=False, substantially expanding collection on top of the three-way parameterization. The third task shard took about 1h46m. Please restore the Play filter unless testing every Play variant here is intentional.
There was a problem hiding this comment.
Fixed in 237d2f3: the matrix now emits only task/backend pairs that explicitly declare the requested PhysicsCfg preset. The Play filter remains removed because no registered core task IDs contain Play variants.
| Changed | ||
| ^^^^^^^ | ||
|
|
||
| * **Breaking:** Changed the Cabinet and Lift task families to use Newton MJWarp |
There was a problem hiding this comment.
This entry explicitly classifies the default-backend switch as breaking, but I could not find a prior deprecation for the Cabinet or Lift default behavior. Please reconcile this with the repository policy that breaking changes require a deprecation first, or clarify why this workflow-default change is exempt.
There was a problem hiding this comment.
The default switch is intentional for the 3.0 release. Per the release direction, we are documenting the direct breaking migration in the changelog rather than adding a deprecation alias or transitional behavior.
|
I traced the two current red checks at
I would rerun or isolate those jobs rather than modify the Newton manipulation configurations based on these failures. |
…manipulation # 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 Cabinet and Lift Newton defaults from #6980.\n\n- Defaults Cabinet, Lift, and Kuka Allegro 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 manipulation portion of #6980.