Fixes hydra to discover presets in deeply nested dictionary

[ooctipus]

Description

This PR robustify hydra to discover presets in deeply nested dictionary

and provide test cases to verify the behavior.

Before deeply nested preset are undiscovered

Type of change

• Bug fix (non-breaking change which fixes an issue)

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and the corresponding version in the extension's config/extension.toml file
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[greptile-apps]

Greptile Summary

This PR fixes collect_presets to recursively discover PresetCfg instances inside deeply nested dicts (e.g., EventTerm.params.terms.*.params) by extracting a new _collect_from_dict helper. It also adds extensive unit test coverage for the Hydra preset system.

• Bug fix in collect_presets: The old code only iterated one level of dict values for dataclass fields. The new _collect_from_dict helper recurses into nested dicts to find deeply buried PresetCfg instances.
• Missing symmetric fix in resolve_preset_defaults: The same shallow-dict-traversal pattern exists in resolve_preset_defaults (lines 284-287), but was not updated. This means deeply nested PresetCfg instances discovered by the fixed collect_presets won't be resolved to their defaults before serialization, likely causing test failures and runtime issues.
• Test coverage: Adds ~380 lines of tests covering deep nesting, preset factory, error handling, _parse_val, idempotency, and class-vs-instance attribute priority.

Confidence Score: 2/5

• The fix is incomplete — resolve_preset_defaults has the same shallow-dict bug that was fixed in collect_presets, which will cause deeply nested presets to not be resolved.
• While the collect_presets fix is correct, the symmetric fix in resolve_preset_defaults was missed. This means the newly added deep-nesting tests that call resolve_preset_defaults are expected to fail, and runtime usage with deeply nested dict presets will also break. The score of 2 reflects a partially correct fix with a critical gap.
• Pay close attention to source/isaaclab_tasks/isaaclab_tasks/utils/hydra.py — the resolve_preset_defaults function needs the same recursive dict traversal fix that was applied to collect_presets.

Important Files Changed

Filename	Overview
source/isaaclab_tasks/isaaclab_tasks/utils/hydra.py	Adds _collect_from_dict for recursive dict traversal in collect_presets, but the symmetric fix for resolve_preset_defaults is missing — deeply nested PresetCfg instances won't be resolved before serialization.
source/isaaclab_tasks/test/test_hydra.py	Adds comprehensive test coverage for deeply nested dicts, preset factory, error handling, and edge cases. Test test_resolve_preset_defaults_deep_nested_dicts likely fails due to missing fix in resolve_preset_defaults.
source/isaaclab_tasks/config/extension.toml	Version bump from 1.5.11 to 1.5.12, consistent with changelog.
source/isaaclab_tasks/docs/CHANGELOG.rst	Adds 1.5.12 changelog entry documenting the fix and new tests. Follows project RST conventions.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[collect_presets cfg] --> B{Is cfg PresetCfg?}
    B -->|Yes| C[Extract fields as preset dict]
    B -->|No| D[Iterate dir cfg attributes]
    D --> E{Has __dataclass_fields__?}
    E -->|Yes, PresetCfg| F[Extract preset dict + recurse alternatives]
    E -->|Yes, regular| G[Recurse collect_presets]
    E -->|No| H{isinstance dict?}
    H -->|Yes| I["_collect_from_dict (NEW)"]
    H -->|No| J[Skip]
    I --> K{dict_val has __dataclass_fields__?}
    K -->|Yes| L[Recurse collect_presets]
    K -->|No| M{dict_val is dict?}
    M -->|Yes| N["Recurse _collect_from_dict (NEW)"]
    M -->|No| O[Skip]

    style I fill:#90EE90
    style N fill:#90EE90
    style H fill:#90EE90

Loading

Comments Outside Diff (1)

1. source/isaaclab_tasks/isaaclab_tasks/utils/hydra.py, line 284-287 (link)

   resolve_preset_defaults also needs recursive dict traversal

   collect_presets was correctly updated to recurse into nested dicts via _collect_from_dict, but resolve_preset_defaults still only handles one level of dict nesting. When a dict value is itself a dict (e.g., params = {"terms": {"step_one": InnerTermCfg()}}), the inner dict is skipped because it doesn't have __dataclass_fields__.

   This means deeply nested PresetCfg instances (the exact scenario this PR targets) won't be resolved to their default values before serialization. The tests like test_resolve_preset_defaults_deep_nested_dicts should fail because OffsetCfg and FractionCfg inside the dict→dict→configclass→dict chain are never visited.

   A fix analogous to _collect_from_dict is needed here — when dict_val is itself a dict, recurse into it:

   With a helper like:

   def _resolve_from_dict(d: dict) -> None:
       for key, val in d.items():
           if isinstance(val, PresetCfg) and hasattr(val, "__dataclass_fields__"):
               default = getattr(val, "default", None)
               if default is not None:
                   d[key] = default
                   if hasattr(default, "__dataclass_fields__"):
                       resolve_preset_defaults(default)
           elif hasattr(val, "__dataclass_fields__"):
               resolve_preset_defaults(val)
           elif isinstance(val, dict):
               _resolve_from_dict(val)

_{Last reviewed commit: 15d3db7}

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR fixes collect_presets and resolve_preset_defaults to discover and resolve PresetCfg instances inside deeply nested dicts (e.g., EventTerm.params.terms.*.params). It also introduces _collect_fields to pick up non-dataclass class attributes (like ObjectCfg.shapes) that the old __dataclass_fields__ loop missed, and consolidates the duplicated preset resolution logic from parse_cfg.py into hydra.py. The approach is correct and the test coverage is excellent.

Architecture Impact

• parse_cfg.py: Removes 4 functions (_is_preset_cfg, _is_old_style_preset, _resolve_presets_to_default, apply_named_preset) and replaces the resolution call in parse_env_cfg with resolve_preset_defaults from hydra.py. These removed functions were only used internally in parse_cfg.py and by env_test_utils.py, both of which are updated in this PR. No external callers are affected.
• env_test_utils.py: Replaces apply_named_preset with apply_overrides + collect_presets from hydra.py. This is functionally equivalent and now supports deep nesting.
• dexsuite_env_cfg.py: Adds newton = cube to ObjectCfg so that presets=newton has a matching alternative. Self-contained change.
• Old-style preset support removed: The _is_old_style_preset handler (for configs with a presets: dict attribute) is removed. No configs in the current codebase use this pattern, so this is safe.

Implementation Verdict

Minor fixes needed — Correct approach, 3 issues to address (1 CHANGELOG/versioning problem, 1 semantic correctness issue, 1 missing version bump).

Test Coverage

✅ Excellent. The PR adds 379 lines of tests covering:

• Deep nested dict preset discovery (test_collect_presets_deep_nested_dicts)
• Deep nested dict resolution (test_resolve_preset_defaults_deep_nested_dicts)
• Global preset application to deeply nested presets (test_deep_nested_dict_global_preset)
• Path-specific selection (test_deep_nested_dict_path_selection)
• Mixed global + path overrides (test_deep_nested_dict_mixed_global_and_path)
• _collect_fields class-vs-instance priority and dynamic class attrs
• preset() factory function edge cases
• Error handling in apply_overrides
• parse_overrides edge cases
• _parse_val type conversion
• Idempotency of resolve_preset_defaults

All tests directly exercise the specific code paths that were broken. The deep-nesting tests would fail on the develop branch (before the fix) and pass after. This is thorough, well-structured test coverage.

CI Status

• ✅ pre-commit: passed
• ✅ labeler: passed
• ✅ Build Base Docker Image: passed
• ⏳ isaaclab_tasks [1/3], [2/3], [3/3]: pending (awaiting GPU runners)
• ⏳ license-check: pending

Branch Status

⚠️ This branch is 4 commits behind develop. The CHANGELOG will likely conflict since both the PR and develop added entries at the top. The PR currently uses version 1.5.15 but develop already has a 1.5.15 entry (semantic segmentation additions). After rebasing, the new entry should be 1.5.16 and config/extension.toml should be bumped to match.

Findings

🟡 Warning: CHANGELOG.rst — Version number collision and missing extension.toml bump
The PR branch was based before develop's 1.5.15 entry was added. The new CHANGELOG entry uses version 1.5.15, which collides with the existing 1.5.15 on develop. After rebase, this should be 1.5.16, and source/isaaclab_tasks/config/extension.toml must be updated from 1.5.15 to 1.5.16 to match. The PR checklist claims the extension.toml was updated but the file is not in the changeset.

🟡 Warning: hydra.py:_collect_fields — cls_val is not None sentinel issue
The is not None check means that if a class-level preset value is legitimately None (e.g., after MyCfg.some_preset = None), the function incorrectly falls back to the instance attribute. While unusual in current usage, this breaks the stated contract of "class-level values take priority." Using a sentinel would be more correct.

🔵 Improvement: hydra.py — _resolve_from_dict placement inconsistency
_collect_from_dict and _collect_fields are nested inside collect_presets, but the analogous _resolve_from_dict is at module level. Both approaches work, but the inconsistency will confuse the next person reading this code. Either nest _resolve_from_dict inside resolve_preset_defaults (matching the pattern), or pull all helpers to module level.

[isaaclab-review-bot]

cls_val is not None silently falls back to the instance attr when the class-level value IS None. This breaks the stated contract ("class-level values take priority") for the edge case where someone sets MyCfg.some_preset = None at runtime. A sentinel makes the intent explicit:

Suggested change

	d[fn] = cls_val if cls_val is not None else getattr(preset_obj, fn)
	_MISSING = object()
	for fn in preset_obj.__dataclass_fields__:
	cls_val = getattr(cls, fn, _MISSING)
	d[fn] = cls_val if cls_val is not _MISSING else getattr(preset_obj, fn)

[isaaclab-review-bot]

Style inconsistency: _collect_from_dict and _collect_fields are nested inside collect_presets, but _resolve_from_dict is at module level. For consistency, either nest this inside resolve_preset_defaults or pull all helpers to module level.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR fixes collect_presets and resolve_preset_defaults to recursively traverse nested dicts (e.g. EventTerm.params.terms.*.params), discovering PresetCfg instances that were previously invisible. It also consolidates duplicate preset-resolution logic from parse_cfg.py into hydra.py, introduces a _collect_fields helper that prefers class-level attrs over instance attrs (for robot-specific module mutations), and adds a newton preset to dexsuite ObjectCfg.

Architecture Impact

• parse_cfg.py: Removes _resolve_presets_to_default, _is_preset_cfg, _is_old_style_preset, and apply_named_preset — all internal/private. The public-facing parse_env_cfg now delegates to hydra.resolve_preset_defaults instead of the local duplicate. No external callers of the removed functions exist.
• env_test_utils.py: Replaces apply_named_preset usage with apply_overrides + collect_presets from hydra.py, which is the canonical path.
• hydra.py: New _resolve_from_dict module-level function and two closures (_collect_fields, _collect_from_dict) inside collect_presets. Cross-module impact is minimal — changes are self-contained within the hydra/preset subsystem.

Implementation Verdict

Minor fixes needed — Core logic is correct and well-tested. Two issues to address: CHANGELOG version/date ordering and missing extension.toml version bump.

Test Coverage

Excellent. 19 new tests cover:

• Deep nested dict preset discovery (test_collect_presets_deep_nested_dicts)
• Deep nested dict resolution (test_resolve_preset_defaults_deep_nested_dicts)
• Full override pipeline with deep nesting (auto-default, global preset, path selection, mixed)
• _collect_fields class-vs-instance priority and dynamic class attrs
• preset() factory edge cases
• apply_overrides error handling (unknown group, unknown name, conflicts)
• parse_overrides edge cases
• _parse_val type conversion
• Scalar override within preset path
• resolve_preset_defaults idempotency

Tests directly exercise the exact code paths that were broken and would catch regressions.

CI Status

• ✅ pre-commit: passed
• ⏳ isaaclab_tasks [1/3, 2/3, 3/3]: in progress
• ⏳ environments_training: in progress
• ⏳ license-check, Build Latest Docs, Build cuRobo Docker Image: in progress
• All other checks: passed or skipped (not applicable)

Findings

🟡 Warning: CHANGELOG.rst — Version 1.5.16 has date 2026-03-24, older than 1.5.15 (2026-03-25). A newer version should have a newer or equal date. Also, the 1.5.14 entry (golden images update) was dropped — this existing entry from develop should be preserved.

🟡 Warning: extension.toml — Version not bumped. CHANGELOG adds 1.5.16 but config/extension.toml still says 1.5.15.

🔵 Improvement: hydra.py _collect_fields — None sentinel conflates "no class attr" with "class attr is None". If a class-level attr is explicitly set to None at runtime (overriding a non-None dataclass default), the fallback to the instance attr silently ignores the class mutation. Using a sentinel object instead of None would be more correct. Low-risk in practice since class-level mutations to None are unusual for presets.

[isaaclab-review-bot]

Two issues:

1. Date 2026-03-24 is older than the 1.5.15 entry below (2026-03-25). A higher version must have a newer-or-equal date.
2. The 1.5.14 entry (golden images update) was dropped from develop. It should be preserved between 1.5.15 and 1.5.13.
3. config/extension.toml still says version = "1.5.15" — bump it to 1.5.16.

Suggested change

	1.5.16 (2026-03-24)
	1.5.16 (2026-03-26)

[isaaclab-review-bot]

Using None as the sentinel for "class has no such attr" means an intentional cls.field = None mutation (overriding a non-None default) silently falls through to the instance value. A dedicated sentinel avoids this:

Suggested change

	cls_val = getattr(cls, fn, None)
	_MISSING = object()
	cls = type(preset_obj)
	d = {}
	for fn in preset_obj.__dataclass_fields__:
	cls_val = getattr(cls, fn, _MISSING)
	d[fn] = cls_val if cls_val is not _MISSING else getattr(preset_obj, fn)

Low-risk today, but prevents a subtle bug if anyone later sets a preset alternative to None.