Remove post-Hydra preset resolution - #7303
Conversation
Greptile SummaryThis PR makes registered task composition the sole preset-resolution boundary and requires runtime consumers to receive concrete physics, renderer, camera, and task configurations.
Confidence Score: 5/5The PR appears safe to merge because no concrete changed-code failure remains after checking the new composition boundary and its principal runtime consumers. Supported callers compose registered tasks before runtime use, concrete built-in backend configurations remain recognized, and the removed raw-configuration fallback is an explicitly documented breaking contract. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Registered task ID] --> B[resolve_task_config or parse_env_cfg]
O[CLI or programmatic overrides] --> B
B --> C[Concrete environment configuration]
C --> D[Application launch scan]
C --> E[Environment construction]
C --> F[Benchmark and RL summaries]
E --> G[Concrete physics, renderer, camera, and scene runtime]
Reviews (1): Last reviewed commit: "Remove post-Hydra preset resolution" | Re-trigger Greptile |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The composition-boundary refactor is coherent, but two API/compatibility issues need correction: benchmark capture rejects the runtime-supported default physics=None, and the public resolve_cfg_presets export is removed without the required deprecation period.
- Design and architecture: Centralizing preset composition in
resolve_task_configandparse_env_cfgremoves late task-layer fallback dependencies cleanly. However, benchmark backend extraction must remain aligned withSimulationContext, where an unset physics configuration deterministically selects PhysX. - API: The new programmatic
overridesparameter is backward-compatible, but deleting the exportedisaaclab.utils.resolve_cfg_presetshelper violates the repository requirement for prior deprecation and migration. Retain a deprecated compatibility wrapper for the required transition period.run_config_from_env_cfgshould also treatsim.physics is Noneas the supported default PhysX backend rather than raising. - Implementation: Updated scripts, entrypoints, and tests consistently route registered tasks through composition. The remaining implementation defect is
_backends_from_env_cfg/run_config_from_env_cfgfailing to represent the existingSimulationCfg.physics=Noneruntime path, which can abort benchmark capture for otherwise valid configurations.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
| ValueError: If the config does not contain a supported concrete physics backend. | ||
| """ | ||
| physics, rendering = _backends_from_env_cfg(env_cfg) | ||
| if physics is None: |
There was a problem hiding this comment.
🟡 Warning · Api — Benchmark capture now fails on unset physics
SimulationCfg.physics is still PhysicsCfg | None (see the added signature in simulation_context.py, which defaults None to PhysxCfg), and the launcher scan tracks a not has_physics case. For such a config _backends_from_env_cfg returns None, so run_config_from_env_cfg raises ValueError where the previous helper reported physx, aborting every benchmark entrypoint. Keep the physx result for None and raise only for unrecognized concrete configs.
There was a problem hiding this comment.
Updated in 8a501ac: benchmark capture now maps sim.physics=None to newton_mjwarp, matching the corrected SimulationCfg and SimulationContext default. It still raises for an unrecognized concrete physics object, and the focused capture suite passes.
| from .types import ArticulationActions | ||
| from .version import has_kit, get_isaac_sim_version, compare_versions | ||
| from .configclass import checked_apply, configclass, resolve_cfg_presets | ||
| from .configclass import checked_apply, configclass |
There was a problem hiding this comment.
🟡 Warning · Api — Public helper removed without deprecation window
resolve_cfg_presets was an explicitly exported public symbol of isaaclab.utils (listed in __all__) and is deleted here along with its definition, so from isaaclab.utils import resolve_cfg_presets now raises ImportError. Repository rules require a prior deprecation and migration path before removing a public API; a breaking changelog entry is not that path. Keep a deprecated forwarding wrapper that points callers at task composition for one release.
There was a problem hiding this comment.
This removal is intentional and documented as breaking in a major fragment. A forwarding wrapper cannot direct callers to task composition because resolve_cfg_presets accepted an arbitrary already-constructed object, while the replacement boundary requires a registered task ID plus Hydra composition. Retaining that wrapper would preserve the exact late-resolution path this change removes. The migration is resolve_task_config or parse_env_cfg before environment construction.
| def _resolve_physics_cfg(physics_cfg: PhysicsCfg | None, use_isaac_sim: bool) -> PhysicsCfg: | ||
| """Resolve a simulation physics config to a concrete backend.""" | ||
| if physics_cfg is None: | ||
| from isaaclab_physx.physics import PhysxCfg |
There was a problem hiding this comment.
If no physics confic, maybe we can resolve newton now?
There was a problem hiding this comment.
Corrected in 8a501ac after confirming the intended default. When SimulationCfg.physics is None, SimulationContext now resolves NewtonCfg() with MJWarp. Passing an explicit PhysxCfg retains Isaac Sim PhysX. The configuration docs, benchmark metadata, changelog, and focused regression coverage now match that contract.
| int(cfg.observation_space[0]) * cfg.frame_stack, | ||
| int(camera_cfg.height), | ||
| int(camera_cfg.width), | ||
| int(cfg.tiled_camera.height), |
There was a problem hiding this comment.
Originally this code was modified to be this way since overriding the camera width and height was not correctly updating observation space. One issue was that the preset had not resolved yet. Please double check that from the command line it correctly picks up that observation space is updated and there is no attribute error
There was a problem hiding this comment.
Verified through the command-line composition path and added a focused regression test in ac557d7. The test passes env.tiled_camera.height=45, env.tiled_camera.width=80, and env.frame_stack=1 through sys.argv/Hydra, then exercises CartpoleCameraEnv initialization far enough to assert observation_space == [3, 45, 80]. The camera config is concrete at that point and there is no attribute error.
## Summary - make resolve_task_config and parse_env_cfg the task configuration composition boundary, with explicit programmatic overrides - remove late preset fallback handling from environments, simulation, launch scanning, camera validation, benchmarks, and RL summaries - require runtime consumers to receive concrete physics, renderer, and camera configurations - route scripts, tools, integrations, and relevant tests through registered task composition This is a separate follow-up to isaac-sim#7301; it does not mix the earlier preset ownership cleanup into this PR. ## Validation - 129 Hydra and Shadow Hand camera tests passed - 31 benchmark capture and RL entrypoint tests passed - 23 experimental frontend tests passed, 1 skipped - 5 custom-coupling tests passed - representative composition audit confirmed six task trees contain no remaining PresetCfg nodes - Python compile checks passed for all changed runtime and script paths - ruff and ruff-format passed The full format command passes every hook except the changelog comparison, which uses the stale local origin/develop ref and flags six pre-existing upstream fragments that this branch does not modify. This PR includes all required package fragments. (cherry picked from commit 393fc37)
…#7303 (#7313) # Description Backports six merged PRs to `release/3.0.0` as separate provenance-preserving cherry-picks: | Source PR | Source commit | Backport commit | Scope | | --- | --- | --- | --- | | #7272 | `0081477fea2` | `c9fc1997476` | Fix scene-wide gravity distributions for PhysX and OvPhysX. | | #7121 | `9f65e3d4fd2` | `cce8acd84e1` | Fix backend-factory fallback before simulator initialization. | | #7183 | `3fcc9c6b824` | `99d58d3d3c4` | Streamline the getting-started documentation. | | #7306 | `6aad90be0ae` | `9a786cc028a` | Optimize Newton test runtime. | | #7305 | `1c9ba908cf1` | `a2dc8ce948e` | Disable Warp autodiff in the test suite. | | #7303 | `393fc37d2b0` | `249a5cb97d8` | Remove post-Hydra preset resolution. | #7272 forwards the validated `uniform`, `log_uniform`, or `gaussian` distribution through both scene-wide backend paths. Newton behavior is unchanged. #7121 makes factory resolution fall back to Newton when no `SimulationContext` exists while retaining the visualizer contract of reporting no active backend before context creation. #7183 is documentation-only. #7306 and #7305 change test infrastructure only; they do not change runtime behavior. #7303 makes `resolve_task_config` and `parse_env_cfg` the task-composition boundary. Runtime consumers now require concrete physics, renderer, and camera configurations and no longer attempt late preset fallback after Hydra composition. The branch was synchronized with the current `release/3.0.0` tip containing #7301 before #7303 was applied. All six source cherry-picks applied without conflicts. A file-by-file existence, mode, and blob audit confirmed that every #7303 path matches its merged source commit exactly. ## Validation ### #7272 - Regression verification against the unpatched `release/3.0.0` tip — both PhysX and OvPhysX cases failed as expected. - `uv run --extra test --frozen python -m pytest -q source/isaaclab/test/envs/test_gravity_randomization.py source/isaaclab/test/envs/test_mdp_event_selectors.py` — 7 passed. - `uv run --extra test --extra ovphysx --frozen python -m pytest -q source/isaaclab_ov/test/physics/test_ovphysx_gravity.py` — 1 passed. ### #7121 - Regression verification before the #7121 cherry-pick reproduced the original `NoneType.physics_manager` failure. - `uv run --extra test --frozen python -m pytest -q source/isaaclab/test/utils/test_backend_utils.py source/isaaclab/test/visualizers/test_visualizer.py` — 18 passed. ### #7183 - `uv run --frozen --extra test python -m pytest --noconftest -q tools/test/test_environ_docs.py` — 26 passed. ### #7306 - All 9 affected paths match the merged source PR exactly. - The full simulator-backed Newton suite is left to backport CI because this local worktree does not include the full Isaac Sim runtime. ### #7305 - Loaded the root `conftest.py` successfully with Warp unavailable. - Loaded it with Warp installed and confirmed `wp.config.enable_backward is False`. ### #7303 - Hydra and Shadow Hand camera suites — 129 passed. - Benchmark capture and RL entrypoint suites — 31 passed. - Experimental frontend and custom-coupling suites — 74 passed, 1 skipped. - Ruff and Ruff-format passed for every changed Python path. - Source/backport audit — 54 paths checked, 0 mismatches. - The Isaac Sim-backed integration case is left to CI because this local worktree is kitless. ### Repository gates - `git diff --check upstream/release/3.0.0...HEAD` — passed. --------- Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com> Co-authored-by: Antoine RICHARD <antoiner@nvidia.com> Co-authored-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com> Co-authored-by: Mustafa H <34825877+StafaH@users.noreply.github.com>
3 participants
Summary
This is a separate follow-up to #7301; it does not mix the earlier preset ownership cleanup into this PR.
Validation
The full format command passes every hook except the changelog comparison, which uses the stale local origin/develop ref and flags six pre-existing upstream fragments that this branch does not modify. This PR includes all required package fragments.