Skip to content

Commit d6e8a21

Browse files
committed
Delegate shim cfg resolution to resolve_presets
Replace the custom flat-getattr + nested-setattr default resolution inside deprecated_task_alias with a single call to isaaclab_tasks.utils.hydra.resolve_presets(cls(), selected). The resolver already walks every nested PresetCfg in the cfg tree and picks the variant matching one of the selected names (falling back to each preset's default field), which is exactly what the canonical task's Hydra path does. The shim now returns bit-for-bit what the canonical task plus presets=<name> would have produced. Side effects: * Drop the cfg_factory parameter from deprecated_task_alias -- the generic resolver handles the 2-axis (nested) case that previously required a custom callable. * Delete the local _resolve_camera_variant helper from direct/cartpole/__init__.py and the cfg_factory= argument from its 5 deprecated-shim call sites. * selected is now the union of every presets=NAME[,...] token's names, so a future caller passing presets=a,b correctly resolves both rather than truncating to the first. No measurable startup-time delta: instantiation of the consolidated PresetCfg (~22 ms, dominated by configclass deepcopy of a ~1400-node tree) is the shared cost; the tree walk on top is sub-millisecond. 73 cartpole deprecation tests still pass.
1 parent 7e355b9 commit d6e8a21

2 files changed

Lines changed: 23 additions & 51 deletions

File tree

source/isaaclab_tasks/isaaclab_tasks/direct/cartpole/__init__.py

Lines changed: 4 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -45,28 +45,10 @@
4545
# Retired per-data-type camera task IDs. Each is registered as a deprecation
4646
# shim that emits a DeprecationWarning naming the consolidated task with the
4747
# equivalent presets=<name>, then loads the corresponding variant of
48-
# CartpoleCameraPresetsEnvCfg. The nested tiled_camera attribute must be
49-
# pinned alongside the root preset -- see _resolve_camera_variant.
50-
def _resolve_camera_variant(preset_name: str):
51-
"""Lazy 2-axis resolver: pin both the root cfg variant and the nested
52-
``tiled_camera`` preset. Without this the nested PresetCfg's default
53-
(rgb) wins and a deprecated albedo task would load albedo's
54-
``observation_space`` with rgb ``data_types``.
55-
56-
Returns a zero-arg callable so the import of
57-
:class:`CartpoleCameraPresetsEnvCfg` is deferred to ``gym.make()``.
58-
"""
59-
60-
def call():
61-
from .cartpole_camera_presets_env_cfg import CartpoleCameraPresetsEnvCfg
62-
63-
cfg = CartpoleCameraPresetsEnvCfg()
64-
result = getattr(cfg, preset_name)
65-
result.tiled_camera = getattr(result.tiled_camera, preset_name)
66-
return result
67-
68-
return call
69-
48+
# CartpoleCameraPresetsEnvCfg. The shim's default cfg resolution walks every
49+
# nested PresetCfg via ``resolve_presets``, so both the root variant and the
50+
# nested ``tiled_camera`` preset are pinned to ``presets=<name>`` without
51+
# per-call-site wiring.
7052

7153
gym.register(
7254
id="Isaac-Cartpole-RGB-Camera-Direct-v0",
@@ -77,7 +59,6 @@ def call():
7759
old_task_id="Isaac-Cartpole-RGB-Camera-Direct-v0",
7860
new_command=["--task=Isaac-Cartpole-Camera-Direct-v0", "presets=rgb"],
7961
consolidated_cfg_path=f"{__name__}.cartpole_camera_presets_env_cfg:CartpoleCameraPresetsEnvCfg",
80-
cfg_factory=_resolve_camera_variant("rgb"),
8162
),
8263
"rl_games_cfg_entry_point": f"{agents.__name__}:rl_games_camera_ppo_cfg.yaml",
8364
"skrl_cfg_entry_point": f"{agents.__name__}:skrl_camera_ppo_cfg.yaml",
@@ -93,7 +74,6 @@ def call():
9374
old_task_id="Isaac-Cartpole-Albedo-Camera-Direct-v0",
9475
new_command=["--task=Isaac-Cartpole-Camera-Direct-v0", "presets=albedo"],
9576
consolidated_cfg_path=f"{__name__}.cartpole_camera_presets_env_cfg:CartpoleCameraPresetsEnvCfg",
96-
cfg_factory=_resolve_camera_variant("albedo"),
9777
),
9878
"rl_games_cfg_entry_point": f"{agents.__name__}:rl_games_camera_ppo_cfg.yaml",
9979
"skrl_cfg_entry_point": f"{agents.__name__}:skrl_camera_ppo_cfg.yaml",
@@ -109,7 +89,6 @@ def call():
10989
old_task_id="Isaac-Cartpole-SimpleShading-Constant-Camera-Direct-v0",
11090
new_command=["--task=Isaac-Cartpole-Camera-Direct-v0", "presets=simple_shading_constant_diffuse"],
11191
consolidated_cfg_path=f"{__name__}.cartpole_camera_presets_env_cfg:CartpoleCameraPresetsEnvCfg",
112-
cfg_factory=_resolve_camera_variant("simple_shading_constant_diffuse"),
11392
),
11493
"rl_games_cfg_entry_point": f"{agents.__name__}:rl_games_camera_ppo_cfg.yaml",
11594
"skrl_cfg_entry_point": f"{agents.__name__}:skrl_camera_ppo_cfg.yaml",
@@ -125,7 +104,6 @@ def call():
125104
old_task_id="Isaac-Cartpole-SimpleShading-Diffuse-Camera-Direct-v0",
126105
new_command=["--task=Isaac-Cartpole-Camera-Direct-v0", "presets=simple_shading_diffuse_mdl"],
127106
consolidated_cfg_path=f"{__name__}.cartpole_camera_presets_env_cfg:CartpoleCameraPresetsEnvCfg",
128-
cfg_factory=_resolve_camera_variant("simple_shading_diffuse_mdl"),
129107
),
130108
"rl_games_cfg_entry_point": f"{agents.__name__}:rl_games_camera_ppo_cfg.yaml",
131109
"skrl_cfg_entry_point": f"{agents.__name__}:skrl_camera_ppo_cfg.yaml",
@@ -141,7 +119,6 @@ def call():
141119
old_task_id="Isaac-Cartpole-SimpleShading-Full-Camera-Direct-v0",
142120
new_command=["--task=Isaac-Cartpole-Camera-Direct-v0", "presets=simple_shading_full_mdl"],
143121
consolidated_cfg_path=f"{__name__}.cartpole_camera_presets_env_cfg:CartpoleCameraPresetsEnvCfg",
144-
cfg_factory=_resolve_camera_variant("simple_shading_full_mdl"),
145122
),
146123
"rl_games_cfg_entry_point": f"{agents.__name__}:rl_games_camera_ppo_cfg.yaml",
147124
"skrl_cfg_entry_point": f"{agents.__name__}:skrl_camera_ppo_cfg.yaml",
@@ -157,7 +134,6 @@ def call():
157134
old_task_id="Isaac-Cartpole-Depth-Camera-Direct-v0",
158135
new_command=["--task=Isaac-Cartpole-Camera-Direct-v0", "presets=depth"],
159136
consolidated_cfg_path=f"{__name__}.cartpole_camera_presets_env_cfg:CartpoleCameraPresetsEnvCfg",
160-
cfg_factory=_resolve_camera_variant("depth"),
161137
),
162138
"rl_games_cfg_entry_point": f"{agents.__name__}:rl_games_camera_ppo_cfg.yaml",
163139
"skrl_cfg_entry_point": f"{agents.__name__}:skrl_camera_ppo_cfg.yaml",

source/isaaclab_tasks/isaaclab_tasks/utils/task_deprecation.py

Lines changed: 19 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@
2626
from collections.abc import Callable
2727
from typing import Any
2828

29+
from isaaclab_tasks.utils.hydra import resolve_presets
30+
2931

3032
def _user_stacklevel() -> int:
3133
"""Compute a ``warnings.warn`` stacklevel that lands on the first frame
@@ -50,7 +52,6 @@ def deprecated_task_alias(
5052
old_task_id: str,
5153
new_command: list[str],
5254
consolidated_cfg_path: str,
53-
cfg_factory: Callable[[], Any] | None = None,
5455
) -> Callable[[], Any]:
5556
"""Wrap a retired gym task ID with a :class:`DeprecationWarning` + cfg resolution.
5657
@@ -68,14 +69,17 @@ def deprecated_task_alias(
6869
``--task=`` first, ``--agent=`` next when present, and the
6970
``presets=NAME`` selector at the end.
7071
71-
Default cfg resolution: imports *consolidated_cfg_path* via
72-
:mod:`importlib`, instantiates the class, and returns
73-
``getattr(instance, <preset>)`` when *new_command* contains a
74-
``presets=<preset>`` token, else the instance itself. The import is
75-
lazy -- it runs on first ``gym.make()``, not at registration time --
76-
matching gym's own handling of string ``"module:Name"`` entry points.
77-
Override via *cfg_factory* when resolution needs custom logic (e.g.
78-
a nested ``PresetCfg`` walk).
72+
Cfg resolution: imports *consolidated_cfg_path* via :mod:`importlib`,
73+
instantiates the class, and delegates to
74+
:func:`~isaaclab_tasks.utils.hydra.resolve_presets` with the union of
75+
every ``presets=NAME[,NAME,...]`` token's names. The resolver walks
76+
every nested :class:`PresetCfg` in the tree and picks the matching
77+
variant (falling back to each preset's ``default`` field). This matches
78+
the canonical task's resolution path; the cfg the deprecated shim
79+
returns is bit-for-bit what the canonical task plus ``presets=<name>``
80+
would have produced. The import is lazy -- it runs on first
81+
``gym.make()``, not at registration time -- matching gym's own handling
82+
of string ``"module:Name"`` entry points.
7983
8084
Args:
8185
old_task_id: The deprecated gym task ID, quoted in the warning body.
@@ -85,11 +89,6 @@ def deprecated_task_alias(
8589
consolidated_cfg_path: ``"module.path:ClassName"`` string for the
8690
consolidated :class:`PresetCfg` subclass. Same format gym
8791
accepts for ``env_cfg_entry_point``. Resolved lazily.
88-
cfg_factory: Optional zero-arg callable that builds the env cfg the
89-
retired task should load. Use when default resolution doesn't
90-
fit -- e.g. a two-axis nested ``PresetCfg`` that needs both
91-
the root and a nested attribute pinned. When set, takes
92-
precedence over *consolidated_cfg_path*'s default resolution.
9392
9493
Returns:
9594
A zero-arg callable suitable for use as ``env_cfg_entry_point``.
@@ -102,16 +101,13 @@ def factory():
102101
DeprecationWarning,
103102
stacklevel=_user_stacklevel(),
104103
)
105-
if cfg_factory is not None:
106-
return cfg_factory()
107-
# Default resolution: pick the variant named by ``presets=<name>`` in
108-
# the new_command, or return the bare consolidated cfg when absent.
109-
preset = next(
110-
(tok.split("=", 1)[1].split(",")[0] for tok in new_command if tok.startswith("presets=")),
111-
None,
112-
)
104+
# Union of every ``presets=NAME[,...]`` token's names, mirroring how the
105+
# hydra layer folds typed selectors before broadcast.
106+
selected = {
107+
name for tok in new_command if tok.startswith("presets=") for name in tok.split("=", 1)[1].split(",")
108+
}
113109
mod_name, cls_name = consolidated_cfg_path.split(":")
114110
cls = getattr(importlib.import_module(mod_name), cls_name)
115-
return getattr(cls(), preset) if preset else cls()
111+
return resolve_presets(cls(), selected)
116112

117113
return factory

0 commit comments

Comments
 (0)