Skip to content

Commit befb2d4

Browse files
authored
Resolve nested callable references in manager term configs (#7513)
## Description After a Hydra / `from_dict()` round-trip, callable references nested outside `ManagerTermBaseCfg.params` can remain lazy string values. Observation modifiers are stored in `ObservationTermCfg.modifiers`, so the existing parameter recursion never reaches `ModifierCfg.func` before `ObservationManager` performs class detection and signature validation. This change extends the existing `ManagerBase._resolve_param_value` recursion instead of adding another resolution path: - `_process_term_cfg_at_play` walks every manager-term field rather than only `params` - `_resolve_param_value` traverses nested configuration dataclasses in addition to its existing dict, list, tuple, and nested-term cases - string-valued `func` fields are resolved at that single point - the root term uses the same resolver before its own signature validation - production code does not inspect or depend on the `ResolvableString` type - `ObservationManager`, `ModifierCfg`, and existing `params` forwarding behavior remain unchanged The modifier documentation now reflects the existing contract: function modifiers use `params` for call-time keyword arguments, while stateful class modifiers keep constructor settings on dedicated `ModifierCfg` subclasses. No second recursive walker, `class_type`, wrapper configuration, modifier dispatch branch, or callable-proxy introspection behavior is introduced. ## Release backport - [x] <!-- backport-active-release --> This PR already targets the active release branch; do not backport it again. ## Tests - Added an end-to-end observation modifier round-trip regression using a dedicated stateful modifier config and empty call-time `params` - Added a manager-level regression proving runtime traversal reaches term fields outside `params` - 92 focused manager, modifier, config, dictionary, and string tests passed - Formatting, lint, security, and RST checks passed - Changelog validation passed against the current upstream `develop` Fixes #6067
1 parent 23c6a68 commit befb2d4

6 files changed

Lines changed: 197 additions & 44 deletions

File tree

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Fixed
2+
^^^^^
3+
4+
* Fixed function and class-based observation modifiers failing after a configuration dictionary round-trip.

source/isaaclab/isaaclab/managers/manager_base.py

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,13 @@
1010
import weakref
1111
from abc import ABC, abstractmethod
1212
from collections.abc import Sequence
13+
from dataclasses import fields
1314
from typing import TYPE_CHECKING, Any
1415

1516
import isaaclab.utils.string as string_utils
1617
from isaaclab.physics import PhysicsEvent, PhysicsManager
1718
from isaaclab.utils import class_to_dict, string_to_callable
19+
from isaaclab.utils.modifiers import ModifierCfg
1820

1921
from .manager_term_cfg import ManagerTermBaseCfg
2022
from .scene_entity_cfg import SceneEntityCfg
@@ -334,8 +336,7 @@ def _resolve_common_term_cfg(self, term_name: str, term_cfg: ManagerTermBaseCfg,
334336
)
335337

336338
# get the corresponding function or functional class
337-
if isinstance(term_cfg.func, str):
338-
term_cfg.func = string_to_callable(term_cfg.func)
339+
term_cfg.func = self._resolve_param_value(term_name, "func", term_cfg.func, resolve_callable=True)
339340
# check if function is callable
340341
if not callable(term_cfg.func):
341342
raise AttributeError(f"The term '{term_name}' is not callable. Received: {term_cfg.func}")
@@ -381,7 +382,7 @@ def _process_term_cfg_at_play(self, term_name: str, term_cfg: ManagerTermBaseCfg
381382
This function is called when the simulation starts playing. It is used to process the term
382383
configuration at runtime. This includes:
383384
384-
* Resolving the scene entity configuration for the term.
385+
* Resolving scene entity configurations and nested terms throughout the term configuration.
385386
* Initializing the term if it is a class.
386387
387388
Since the above steps rely on PhysX to parse over the simulation scene, they are deferred
@@ -391,27 +392,49 @@ def _process_term_cfg_at_play(self, term_name: str, term_cfg: ManagerTermBaseCfg
391392
term_name: The name of the term.
392393
term_cfg: The term configuration.
393394
"""
394-
for key, value in term_cfg.params.items():
395-
self._resolve_param_value(term_name, key, value)
395+
for field in fields(term_cfg):
396+
value = getattr(term_cfg, field.name)
397+
resolved_value = self._resolve_param_value(
398+
term_name, field.name, value, resolve_callable=field.name == "func"
399+
)
400+
if resolved_value is not value:
401+
setattr(term_cfg, field.name, resolved_value)
396402

397-
# resolve string func references then initialize class-based terms
398-
if isinstance(term_cfg.func, str):
399-
term_cfg.func = string_to_callable(term_cfg.func)
403+
# initialize class-based terms
400404
if inspect.isclass(term_cfg.func):
401405
term_cfg.func = term_cfg.func(cfg=term_cfg, env=self._env)
402406

403-
def _resolve_param_value(self, term_name: str, key: str | int, value: Any):
404-
"""Recursively resolve a single param value (SceneEntityCfg, nested term cfgs, dicts, lists)."""
407+
def _resolve_param_value(
408+
self, term_name: str, key: str | int, value: Any, *, resolve_callable: bool = False
409+
) -> Any:
410+
"""Recursively resolve manager-owned values in a term configuration."""
411+
if resolve_callable and isinstance(value, str):
412+
return string_to_callable(value)
405413
if isinstance(value, SceneEntityCfg):
406414
try:
407415
value.resolve(self._env.scene)
408416
except ValueError as e:
409417
raise ValueError(f"Error while parsing '{term_name}:{key}'. {e}")
410418
elif isinstance(value, ManagerTermBaseCfg):
411419
self._process_term_cfg_at_play(f"{term_name}.{key}", value)
420+
elif isinstance(value, ModifierCfg):
421+
for field in fields(value):
422+
field_value = getattr(value, field.name)
423+
resolved_value = self._resolve_param_value(
424+
f"{term_name}.{key}", field.name, field_value, resolve_callable=field.name == "func"
425+
)
426+
if resolved_value is not field_value:
427+
setattr(value, field.name, resolved_value)
412428
elif isinstance(value, dict):
413429
for sub_key, sub_value in value.items():
414-
self._resolve_param_value(f"{term_name}.{key}", sub_key, sub_value)
415-
elif isinstance(value, (list, tuple)):
430+
value[sub_key] = self._resolve_param_value(f"{term_name}.{key}", sub_key, sub_value)
431+
elif isinstance(value, list):
416432
for i, item in enumerate(value):
417-
self._resolve_param_value(f"{term_name}.{key}", i, item)
433+
value[i] = self._resolve_param_value(f"{term_name}.{key}", i, item)
434+
elif isinstance(value, tuple):
435+
resolved_items = tuple(
436+
self._resolve_param_value(f"{term_name}.{key}", i, item) for i, item in enumerate(value)
437+
)
438+
if any(resolved is not original for resolved, original in zip(resolved_items, value, strict=True)):
439+
value = resolved_items
440+
return value

source/isaaclab/isaaclab/managers/observation_manager.py

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -396,7 +396,10 @@ def compute_group(self, group_name: str, update_history: bool = False) -> torch.
396396
# apply post-processing
397397
if term_cfg.modifiers is not None:
398398
for modifier in term_cfg.modifiers:
399-
obs = modifier.func(obs, **modifier.params)
399+
if isinstance(modifier.func, modifiers.ModifierBase):
400+
obs = modifier.func(obs)
401+
else:
402+
obs = modifier.func(obs, **modifier.params)
400403
if isinstance(term_cfg.noise, noise.NoiseCfg):
401404
obs = term_cfg.noise.func(obs, term_cfg.noise)
402405
elif isinstance(term_cfg.noise, noise.NoiseModelCfg) and term_cfg.noise.func is not None:
@@ -579,25 +582,22 @@ def _prepare_terms(self):
579582
if term_cfg.modifiers is not None:
580583
# initialize list of modifiers for term
581584
for mod_cfg in term_cfg.modifiers:
582-
# check if class modifier and initialize with observation size when adding
583-
if isinstance(mod_cfg, modifiers.ModifierCfg):
584-
# to list of modifiers - instantiate class-based modifiers
585-
if inspect.isclass(mod_cfg.func):
586-
mod_cfg.func = mod_cfg.func(cfg=mod_cfg, data_dim=obs_dims, device=self._env.device)
587-
# verify the instance is the correct type
588-
if not isinstance(mod_cfg.func, modifiers.ModifierBase):
589-
raise TypeError(
590-
f"Modifier function '{mod_cfg.func}' for observation term '{term_name}'"
591-
f" is not an instance of 'ModifierBase'. Received: '{type(mod_cfg.func)}'."
592-
)
593-
# add to list of class modifiers
594-
self._group_obs_class_instances.append(mod_cfg.func)
595-
else:
585+
if not isinstance(mod_cfg, modifiers.ModifierCfg):
596586
raise TypeError(
597587
f"Modifier configuration '{mod_cfg}' of observation term '{term_name}' is not of"
598588
f" required type ModifierCfg, Received: '{type(mod_cfg)}'"
599589
)
600590

591+
# construct stateful modifiers with the observation size
592+
if inspect.isclass(mod_cfg.func):
593+
mod_cfg.func = mod_cfg.func(cfg=mod_cfg, data_dim=obs_dims, device=self._env.device)
594+
if not isinstance(mod_cfg.func, modifiers.ModifierBase):
595+
raise TypeError(
596+
f"Modifier function '{mod_cfg.func}' for observation term '{term_name}'"
597+
f" is not an instance of 'ModifierBase'. Received: '{type(mod_cfg.func)}'."
598+
)
599+
self._group_obs_class_instances.append(mod_cfg.func)
600+
601601
# check if function is callable
602602
if not callable(mod_cfg.func):
603603
raise AttributeError(

source/isaaclab/isaaclab/utils/modifiers/modifier_cfg.py

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3,35 +3,38 @@
33
#
44
# SPDX-License-Identifier: BSD-3-Clause
55

6+
from __future__ import annotations
7+
68
from collections.abc import Callable
79
from dataclasses import MISSING
8-
from typing import Any
10+
from typing import TYPE_CHECKING, Any
911

1012
import torch
1113

1214
from isaaclab.utils.configclass import configclass
1315

14-
from . import modifier
16+
if TYPE_CHECKING:
17+
from .modifier import DigitalFilter, Integrator
18+
from .modifier_base import ModifierBase
1519

1620

1721
@configclass
1822
class ModifierCfg:
19-
"""Configuration parameters modifiers"""
20-
21-
func: Callable[..., torch.Tensor] = MISSING
22-
"""Function or callable class used by modifier.
23+
"""Configuration parameters for function and class modifiers."""
2324

24-
The function must take a torch tensor as the first argument. The remaining arguments are specified
25-
in the :attr:`params` attribute.
25+
func: Callable[..., torch.Tensor] | type[ModifierBase] | str = MISSING
26+
"""Function or :class:`ModifierBase` class used by the modifier.
2627
27-
It also supports `callable classes <https://docs.python.org/3/reference/datamodel.html#object.__call__>`_,
28-
i.e. classes that implement the ``__call__()`` method. In this case, the class should inherit from the
29-
:class:`ModifierBase` class and implement the required methods.
28+
Functions must take a tensor as their first argument. Classes must inherit from :class:`ModifierBase`; the
29+
observation manager constructs them with the configuration, observation dimensions, and device.
3030
"""
3131

3232
params: dict[str, Any] = dict()
33-
"""The parameters to be passed to the function or callable class as keyword arguments. Defaults to
34-
an empty dictionary."""
33+
"""Parameters used by the modifier. Defaults to an empty dictionary.
34+
35+
Function modifiers receive them as keyword arguments on each call. Class modifiers access them through this
36+
configuration during construction; they are not forwarded to the constructed instance.
37+
"""
3538

3639

3740
@configclass
@@ -41,7 +44,7 @@ class DigitalFilterCfg(ModifierCfg):
4144
For more information, please check the :class:`DigitalFilter` class.
4245
"""
4346

44-
func: type[modifier.DigitalFilter] = modifier.DigitalFilter
47+
func: type[DigitalFilter] | str = "{DIR}.modifier:DigitalFilter"
4548
"""The digital filter function to be called for applying the filter."""
4649

4750
A: list[float] = MISSING
@@ -72,7 +75,7 @@ class IntegratorCfg(ModifierCfg):
7275
For more information, please check the :class:`Integrator` class.
7376
"""
7477

75-
func: type[modifier.Integrator] = modifier.Integrator
78+
func: type[Integrator] | str = "{DIR}.modifier:Integrator"
7679
"""The integrator function to be called for applying the integrator."""
7780

7881
dt: float = MISSING

source/isaaclab/test/managers/test_manager_base.py

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
# ignore private usage of variables warning
77
# pyright: reportPrivateUsage=none
88

9-
"""Tests for recursive _process_term_cfg_at_play / _resolve_param_value.
9+
"""Tests for recursive manager term configuration resolution.
1010
1111
These tests exercise ManagerBase's parameter resolution logic and do NOT
1212
require an Isaac Sim launch, so they can run without AppLauncher.
@@ -22,6 +22,8 @@
2222
from isaaclab.envs import ManagerBasedEnv
2323
from isaaclab.managers import ManagerTermBase, ManagerTermBaseCfg
2424
from isaaclab.managers.manager_base import ManagerBase
25+
from isaaclab.utils import modifiers
26+
from isaaclab.utils.configclass import configclass
2527

2628
pytestmark = pytest.mark.integration
2729

@@ -69,6 +71,23 @@ def reset_dummy2_to_zero(env, env_ids: torch.Tensor):
6971
env.dummy2[env_ids] = 0
7072

7173

74+
@configclass
75+
class OpaqueCfg:
76+
"""Configuration that is outside ``ManagerBase`` ownership."""
77+
78+
func: str = f"{__name__}:reset_dummy2_to_zero"
79+
80+
81+
@configclass
82+
class NestedFieldTermCfg(ManagerTermBaseCfg):
83+
"""Manager term with owned and opaque fields outside ``params``."""
84+
85+
nested_term: ManagerTermBaseCfg = ManagerTermBaseCfg(func=increment_dummy1_by_one)
86+
modifier: modifiers.ModifierCfg = modifiers.ModifierCfg(func=increment_dummy1_by_one)
87+
metadata: dict[str, str] = {"func": f"{__name__}:reset_dummy2_to_zero"}
88+
opaque: OpaqueCfg = OpaqueCfg()
89+
90+
7291
class reset_dummy2_to_zero_class(ManagerTermBase):
7392
def __init__(self, cfg: ManagerTermBaseCfg, env: ManagerBasedEnv):
7493
super().__init__(cfg, env)
@@ -217,6 +236,24 @@ def test_string_func_in_nested_term_cfg(env):
217236
torch.testing.assert_close(env.dummy1, 11 * torch.ones_like(env.dummy1))
218237

219238

239+
def test_resolution_walks_declared_term_fields_outside_params(env):
240+
"""Resolve nested term and modifier callables without interpreting arbitrary ``func`` keys."""
241+
outer_cfg = NestedFieldTermCfg(
242+
func=increment_dummy1_by_one,
243+
nested_term=ManagerTermBaseCfg(func=f"{__name__}:reset_dummy2_to_zero"),
244+
)
245+
outer_cfg.from_dict(outer_cfg.to_dict())
246+
cfg = {"outer": outer_cfg}
247+
manager = SimpleManager(cfg, env)
248+
249+
term_cfg = manager._term_cfgs[0][1]
250+
assert isinstance(term_cfg, NestedFieldTermCfg)
251+
assert term_cfg.nested_term.func is reset_dummy2_to_zero
252+
assert term_cfg.modifier.func is increment_dummy1_by_one
253+
assert isinstance(term_cfg.metadata["func"], str)
254+
assert isinstance(term_cfg.opaque.func, str)
255+
256+
220257
def test_string_func_top_level_class_term(env):
221258
"""Test that a top-level string-based func pointing to a class is properly instantiated."""
222259
this_module = __name__

source/isaaclab/test/managers/test_observation_manager_unit.py

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,14 @@
99

1010
# ignore private usage of variables warning
1111
# pyright: reportPrivateUsage=none
12+
import inspect
1213
from typing import TYPE_CHECKING, cast
1314

1415
import pytest
1516
import torch
1617

1718
from isaaclab.managers import ObservationGroupCfg, ObservationManager, ObservationTermCfg
19+
from isaaclab.utils import modifiers
1820
from isaaclab.utils.configclass import configclass
1921

2022
pytestmark = pytest.mark.unit
@@ -46,6 +48,28 @@ def __init__(self, num_envs: int = 2) -> None:
4648
self.observation = torch.arange(num_envs, dtype=torch.float32).unsqueeze(-1)
4749

4850

51+
class StatefulBiasModifier(modifiers.ModifierBase):
52+
"""Stateful modifier used to verify lazy callable resolution."""
53+
54+
def __init__(self, cfg: modifiers.ModifierCfg, data_dim: tuple[int, ...], device: str) -> None:
55+
super().__init__(cfg, data_dim, device)
56+
self.value = cfg.params["value"]
57+
self.reset_count = 0
58+
59+
def reset(self, env_ids=None) -> None:
60+
self.reset_count += 1
61+
62+
def __call__(self, data: torch.Tensor) -> torch.Tensor:
63+
return data + self.value
64+
65+
66+
class InvalidModifier:
67+
"""Class with the modifier constructor contract but the wrong base type."""
68+
69+
def __init__(self, cfg, data_dim, device):
70+
pass
71+
72+
4973
@configclass
5074
class HistoryObservationsCfg:
5175
"""Observation configuration with group-level history."""
@@ -62,6 +86,68 @@ def __post_init__(self):
6286
policy: PolicyCfg = PolicyCfg()
6387

6488

89+
def test_class_modifier_roundtrip_preserves_func_and_params():
90+
"""Reproduce #6067 with a class modifier and non-empty parameters."""
91+
cfg = HistoryObservationsCfg()
92+
cfg.policy.history_length = None
93+
cfg.policy.dummy.modifiers = [modifiers.ModifierCfg(func=StatefulBiasModifier, params={"value": 2.0})]
94+
cfg.from_dict(cfg.to_dict())
95+
term_cfg = cfg.policy.dummy
96+
assert term_cfg.modifiers is not None
97+
modifier_cfg = term_cfg.modifiers[0]
98+
assert isinstance(modifier_cfg, modifiers.ModifierCfg)
99+
assert isinstance(modifier_cfg.func, str)
100+
assert modifier_cfg.params == {"value": 2.0}
101+
102+
env = DummyEnv()
103+
manager = ObservationManager(cfg, cast("ManagerBasedEnv", env))
104+
prepared_term_cfg = manager.cfg.policy.dummy
105+
assert prepared_term_cfg.modifiers is not None
106+
prepared_modifier_cfg = prepared_term_cfg.modifiers[0]
107+
assert isinstance(prepared_modifier_cfg.func, StatefulBiasModifier)
108+
observations = manager.compute()["policy"]
109+
torch.testing.assert_close(observations, env.observation + 2.0)
110+
111+
manager.reset()
112+
assert prepared_modifier_cfg.func.reset_count == 1
113+
114+
115+
def test_stateless_modifier_cfg_roundtrip_preserves_signature_validation():
116+
"""A stateless modifier remains callable and inspectable after a configuration round-trip."""
117+
cfg = HistoryObservationsCfg()
118+
cfg.policy.history_length = None
119+
cfg.policy.dummy.modifiers = [modifiers.ModifierCfg(func=modifiers.bias, params={"value": 2.0})]
120+
cfg.from_dict(cfg.to_dict())
121+
122+
env = DummyEnv()
123+
manager = ObservationManager(cfg, cast("ManagerBasedEnv", env))
124+
observations = manager.compute()["policy"]
125+
torch.testing.assert_close(observations, env.observation + 2.0)
126+
127+
128+
def test_class_modifier_validates_constructed_instance():
129+
"""Class modifier validation checks the constructed object."""
130+
cfg = HistoryObservationsCfg()
131+
cfg.policy.history_length = None
132+
cfg.policy.dummy.modifiers = [modifiers.ModifierCfg(func=InvalidModifier)]
133+
cfg.from_dict(cfg.to_dict())
134+
135+
with pytest.raises(TypeError, match="is not an instance of 'ModifierBase'"):
136+
ObservationManager(cfg, cast("ManagerBasedEnv", DummyEnv()))
137+
138+
139+
def test_modifier_resolution_stays_out_of_observation_manager():
140+
"""Observation-specific code receives resolved modifier callables from ``ManagerBase``."""
141+
source = inspect.getsource(ObservationManager._prepare_terms)
142+
assert "inspect.isclass(mod_cfg.func)" in source
143+
assert "string_to_callable" not in source
144+
145+
146+
def test_modifier_base_cfg_marker_does_not_exist():
147+
"""Stateful modifiers must not require a marker configuration subtype."""
148+
assert not hasattr(modifiers, "ModifierBaseCfg")
149+
150+
65151
def test_compute_updates_history_only_when_requested():
66152
"""Observation history changes only when ``update_history`` is enabled."""
67153
env = DummyEnv()

0 commit comments

Comments
 (0)