Resolve nested callable references in manager term configs

[ooctipus]

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

• 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

[greptile-apps]

Greptile Summary

This PR makes observation-modifier lifecycle selection depend on an explicit stateful configuration type rather than callable implementation introspection.

• Adds and exports ModifierBaseCfg, migrating the built-in digital-filter and integrator configurations.
• Constructs and validates stateful modifier instances during observation-manager preparation while keeping constructor parameters out of runtime calls.
• Makes ResolvableString expose its resolved callable signature for stateless parameter validation.
• Adds regression coverage for configuration round-trips, lifecycle dispatch, constructed-instance validation, and lazy signature inspection.

Confidence Score: 5/5

The PR appears safe to merge, with the documented breaking lifecycle migration consistently implemented and covered by focused regression tests.

Stateful modifiers are now selected through an explicit configuration subtype, stateless lazy callables expose their underlying signatures, and no reachable unacknowledged failure remains in the reviewed paths.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/managers/observation_manager.py	Replaces implementation-class introspection with configuration-type dispatch and separates stateful construction from stateless signature validation.
source/isaaclab/isaaclab/utils/modifiers/modifier_cfg.py	Introduces the explicit stateful modifier configuration base and migrates both built-in stateful configurations.
source/isaaclab/isaaclab/utils/string.py	Delegates signature inspection through lazy callable resolution so stateless validation sees the underlying function.
source/isaaclab/test/managers/test_observation_manager_unit.py	Covers stateful and stateless round-trips, runtime behavior, reset lifecycle, and invalid stateful factories.
source/isaaclab/test/utils/test_string.py	Verifies that lazy callable references expose the same signature as their resolved targets.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    C[Observation modifier configuration] --> T{Configuration type}
    T -->|ModifierBaseCfg| S[Resolve and construct ModifierBase]
    S --> V[Validate constructed instance]
    V --> SC[Call instance with observation tensor]
    T -->|ModifierCfg| F[Resolve function signature]
    F --> P[Validate configured parameters]
    P --> FC[Call function with observation and params]

Loading

_{Reviews (1): Last reviewed commit: "Use config type for stateful observation..." | Re-trigger Greptile}

[isaaclab-review-bot]

Isaac Lab Review Bot

The configuration-type lifecycle discriminator fixes lazy round-trip handling and is consistently used during preparation and dispatch. However, the change removes previously supported class-based ModifierCfg behavior without the required deprecation path, and the new stateful configuration inherits an incompatible stateless func contract.

• Design and architecture: Selecting stateful modifiers through ModifierBaseCfg avoids unreliable implementation introspection after serialization. The transition must retain deprecated handling for class-based ModifierCfg entries rather than silently routing them through stateless validation.
• API: ModifierBaseCfg is exported and accompanied by migration notes, but that does not satisfy the repository requirement for prior deprecation of a public API. Its inherited Callable[..., torch.Tensor] annotation and tensor-first documentation also conflict with the constructor contract used by the manager; func should be declared as a ModifierBase implementation type with matching documentation.
• Implementation: The new preparation and compute paths consistently distinguish configuration types, and ResolvableString.__signature__ supports stateless signature validation. Existing ModifierCfg(func=MyModifier, ...) configurations now reach stateless signature validation and fail with a misleading parameter error instead of receiving compatibility handling and a deprecation warning.

Significant concerns. Posted 2 actionable findings inline.

Automated review; human maintainers own approval decisions.

[isaaclab-review-bot]

🟡 Warning · Api — Class modifiers removed without deprecation path

ModifierCfg.func previously documented and supported ModifierBase subclasses, and the manager instantiated any class-valued func. Now only ModifierBaseCfg is constructed, so existing ModifierCfg(func=MyModifier, params=...) falls into the stateless branch and fails with an unrelated "expects mandatory parameters: ['data_dim', 'device']" error. Keep constructing ModifierBase subclasses from ModifierCfg with a deprecation warning pointing at ModifierBaseCfg, or at minimum raise an explicit migration error.

[isaaclab-review-bot]

🔵 Suggestion · Api — ModifierBaseCfg inherits stateless func contract

ModifierBaseCfg inherits func: Callable[..., torch.Tensor] and the inherited docstring stating the callable takes a tensor first, but the manager invokes it as func(cfg=..., data_dim=..., device=...) and requires a ModifierBase instance. The documented ModifierBaseCfg(func=MyModifier) usage therefore contradicts its own annotation. Override func with type[ModifierBase] and a matching docstring, as DigitalFilterCfg and IntegratorCfg already do.

[ooctipus]

run-ci

[StafaH]

AI Review:

This test avoids the configuration that triggered #6067. The reported usage is ModifierCfg(func=MyModifier, params={...}), where params are read by the modifier constructor.

After this PR resolves and instantiates the class, mod_cfg.params remains populated. compute_group() then invokes the instance as:

obs = modifier.func(obs, **modifier.params)

For the reported call(self, data) implementation, this raises TypeError on the first observation computation. The dedicated config subclass and empty params used here hide that failure.

Please make the regression reproduce #6067 with a non-empty ModifierCfg.params and ensure constructor parameters are not forwarded to the constructed modifier’s call.

[StafaH]

Can we use explicit dataclass/config-class check?

AI comment: The current duck-typing is difficult to read, skips slotted dataclasses, and attempts setattr() on frozen dataclasses

My comment: This type of hasattr setattr behaviour loses typing and adds misdirection that becomes unreadable

[StafaH]

can be replaced with dataclasses.is_dataclass() and iterating dataclasses.fields(). That avoids traversing unrelated runtime attributes stored in dict, but the discarded sequence results are the more worthwhile comment.

[StafaH]

Could we avoid calling string_to_callable() directly from ManagerBase? Since ResolvableString already represents a lazy callable reference, it would be cleaner for it to expose a resolve() method and own the conversion logic.

That would let the manager resolve only explicit callable fields on known config types such as ManagerTermBaseCfg and ModifierCfg, instead of interpreting every string under a dictionary key named "func" as an import path. The latter can incorrectly affect arbitrary dictionaries in params, for example {"func": "linear"}.

Raw strings could still be supported on explicit func fields for backward compatibility. This keeps the recursive traversal generic while moving callable resolution to the abstraction that owns it.

[StafaH]

The recursive calls for lists and tuples discard the returned value, so this only works when children happen to be mutated in place. Any resolver that replaces a value—such as resolving a ResolvableString into a callable—will not update the sequence.

Could we assign resolved list elements back and rebuild tuples? This would make _resolve_param_value() a consistent recursive transformation instead of relying on mutation as a side effect:

elif isinstance(value, list):
    for i, item in enumerate(value):
        value[i] = self._resolve_param_value(f"{term_name}.{key}", i, item)
elif isinstance(value, tuple):
    value = tuple(
        self._resolve_param_value(f"{term_name}.{key}", i, item)
        for i, item in enumerate(value)
    )

[StafaH]

can be replaced with dataclasses.is_dataclass() and iterating dataclasses.fields(). That avoids traversing unrelated runtime attributes stored in dict, but the discarded sequence results are the more worthwhile comment.

[StafaH]

Left some comments, but overall LGTM

[ooctipus]

run-ci

[ooctipus]

run-ci

[isaaclab-bot]

Backported to release/3.0.0 as 355dc9b.