Simplify preset resolution with single-pass resolve_presets#5177
Simplify preset resolution with single-pass resolve_presets#5177AntoineRichard merged 13 commits intoisaac-sim:developfrom
Conversation
There was a problem hiding this comment.
Review: PR #5177 — Simplify preset resolution with single-pass resolve_presets
Summary
This PR replaces the two-phase resolve_preset_defaults → apply_overrides flow with a single-pass resolve_presets function that walks the config tree once, resolving every PresetCfg in-place. It also fixes two real bugs:
collect_presetsdidn't recurse into dict-valued alternatives — deeply nested configs were silently using wrong defaults.resolve_preset_defaultssilently skippedPresetCfg(default=None)— leaving thePresetCfgwrapper in the tree instead of replacing it withNone.
Both fixes are correct. The test coverage is comprehensive (new tests for dict-valued alternatives, deeply nested dicts, default=None, class-level overrides, and selected name propagation). The refactoring of test boilerplate into a shared _apply helper is clean. The __init__.pyi cleanup removing the duplicate import line is good.
Design Assessment
The approach is sound. Moving preset resolution earlier (in register_task with CLI selection) and making apply_overrides handle only path-based selections simplifies the mental model. The defensive resolution of alternatives inside the collected presets dict is a good touch.
One architectural note: resolve_cfg_presets in configclass.py (used by the gym.make() path) has the same dict-traversal bug being fixed here in collect_presets. That's out of scope for this PR but worth a follow-up.
Issues Found
See inline comments below.
| val = _pick_alternative(preset_obj, selected) | ||
| if val is preset_obj: | ||
| return | ||
| while isinstance(val, PresetCfg): |
There was a problem hiding this comment.
🟡 Potential infinite loop when a replacement PresetCfg has no default and no matching selected name.
If _pick_alternative(val, selected) returns val itself (no matching field, no default), this while loop never terminates because isinstance(val, PresetCfg) remains True.
Reproduction scenario:
@configclass
class InnerNoDefault(PresetCfg):
option_a: int = 1 # no "default" field
@configclass
class OuterCfg(PresetCfg):
default: InnerNoDefault = InnerNoDefault()resolve_presets(OuterCfg()) → picks default → InnerNoDefault() → while loop calls _pick_alternative(InnerNoDefault(), frozenset()) → returns InnerNoDefault() itself → infinite loop.
The root-level path (line 209) correctly handles this with if replacement is cfg: return cfg, but the _resolve callback's while loop lacks the same guard.
| while isinstance(val, PresetCfg): | |
| def _resolve(parent, key, preset_obj, _path): | |
| val = _pick_alternative(preset_obj, selected) | |
| if val is preset_obj: | |
| return | |
| while isinstance(val, PresetCfg): | |
| next_val = _pick_alternative(val, selected) | |
| if next_val is val: | |
| break | |
| val = next_val |
There was a problem hiding this comment.
@ooctipus the bot looks correct? Maybe this would never happen though
| # Preset resolution | ||
| # ============================================================================ | ||
|
|
||
|
|
There was a problem hiding this comment.
🟡 Non-deterministic selection order when multiple preset names match.
selected is typed as set[str] (unordered). The for name in selected loop picks the "first" match, but sets have no guaranteed iteration order in Python. If a user passes presets=newton,physx and a PresetCfg has both newton and physx fields, the result depends on Python's internal hash ordering.
This isn't a problem in practice today since PresetCfg alternatives are typically disjoint, but it's a latent bug that could surprise users. Consider using a list (or dict.fromkeys() to preserve insertion order while deduplicating) for selected:
| def _pick_alternative(preset_obj: PresetCfg, selected: list[str] | set[str]): |
And in register_task, build selected as a list:
selected: list[str] = []
for arg in sys.argv[1:]:
if "=" in arg:
key, val = arg.split("=", 1)
if key.lstrip("-") == "presets":
selected.extend(v.strip() for v in val.split(",") if v.strip() and v.strip() not in selected)| agent_cfg = resolve_presets(agent_cfg, selected) | ||
|
|
||
| # Also resolve presets inside collected alternatives so that apply_overrides | ||
| # never re-introduces unresolved PresetCfg objects when applying a selection. |
There was a problem hiding this comment.
🔵 Duplicated CLI preset parsing — register_task re-parses sys.argv for preset names.
parse_overrides (called later in _run_hydra) also extracts global presets from sys.argv. The register_task function re-implements this extraction to call resolve_presets early, before Hydra runs. This creates a maintenance hazard — if someone changes the presets= parsing logic in parse_overrides, they'd need to update register_task too.
Consider factoring the sys.argv → selected: set[str] extraction into a small shared helper, or calling parse_overrides early (it only needs the presets dict and args, both available at this point).
Greptile SummaryThis PR replaces the multi-phase Confidence Score: 4/5Safe to merge with minor issues — the primary fix and resolution path work correctly; remaining findings are edge-case bugs and style issues. The two logic findings in hydra.py (infinite loop and dict-key omission) are both P2 edge cases that do not affect the primary resolution path exercised by CI tests. They could silently misbehave for unusual preset shapes or path-based CLI overrides on dict-nested presets, but the core fix is sound and well-tested. source/isaaclab_tasks/isaaclab_tasks/utils/hydra.py — the _resolve while loop guard and collect_presets._record dict traversal Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[register_task] --> B[collect_presets]
B --> C{Node is PresetCfg?}
C -->|Yes| D[_record: store alternatives at path]
D --> E{Alternative is dataclass?}
E -->|Yes| F[Recurse collect_presets\nwith preset_path prefix]
E -->|No - alt is dict| G[Iterate dict values\nRecurse collect_presets\nwith preset_path — dict key skipped ⚠]
C -->|No - is dataclass or dict| H[_walk_cfg recursive traversal]
A --> I[resolve_presets with selected names]
I --> J[_walk_cfg cfg]
J --> K{Found PresetCfg node?}
K -->|Yes| L[_pick_alternative]
L --> M{Resolved val is PresetCfg?}
M -->|Yes - while loop| N{next_val is val?}
N -->|Yes - no default and no match| O[⚠ Infinite loop risk]
N -->|No| L
M -->|No| P[Replace node in parent]
P --> Q[Continue walk into replacement]
A --> R[apply_overrides]
R --> S{Path in presets dict?}
S -->|Dict-nested sub-preset| T[⚠ Key omitted — path mismatch, silently ignored]
S -->|Standard paths| U[Apply chosen alternative]
|
AntoineRichard
left a comment
There was a problem hiding this comment.
I would look into the bot's feedback, otherwise it looks like it's doing what it should. Though I feel like the hydra thing is starting to be heavy.
|
|
||
| _HYDRA_AVAILABLE = True | ||
| except ImportError: | ||
| raise ImportError("Hydra not installed. Run: pip install hydra-core") |
There was a problem hiding this comment.
Do we want to support environments without HYDRA? At this point HYDRA is so important to lab that I don't see why we would bother supporting non-HYDRA environments.
| val = _pick_alternative(preset_obj, selected) | ||
| if val is preset_obj: | ||
| return | ||
| while isinstance(val, PresetCfg): |
There was a problem hiding this comment.
@ooctipus the bot looks correct? Maybe this would never happen though
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot — Follow-up Review
Changes Since Last Review
The author addressed the main concern from our previous review:
✅ Infinite loop / unresolvable PresetCfg — _pick_alternative now raises ValueError when no default field exists and no selected name matches. Both the root-level and nested resolution paths use seen: set[int] to detect cyclic chains. Four new test cases cover these scenarios (test_resolve_presets_errors_on_no_default, test_resolve_presets_errors_on_chained_no_default, test_resolve_presets_errors_on_cyclic_preset, test_resolve_presets_errors_on_cyclic_preset_at_root).
Current State
The code is in good shape. The remaining items from the initial review are minor and non-blocking:
-
Non-deterministic set iteration (line 180) —
selectedisset[str], so when multiple preset names match the samePresetCfg, the winner depends on Python hash ordering. Low risk since preset alternatives are typically disjoint, but worth documenting. -
Duplicated CLI parsing (line 367-375) —
register_taskre-parsessys.argvfor preset names independently ofparse_overrides. A shared helper would reduce maintenance risk but isn't a blocker.
CI Status
CI failures (isaaclab (core), isaaclab_tasks [1/3], [2/3]) are unrelated to this PR — they're sensor/camera rendering test flakes (test_first_frame_textured_rendering, test_tiled_camera). The hydra tests (test_hydra.py) run in isaaclab_tasks [3/3] which passed. Pre-commit also passed.
Verdict
No new blocking issues. The cycle detection and error handling improvements are solid. LGTM pending the CI re-runs on unrelated failures.
| elif isinstance(alt, dict): | ||
| for v in alt.values(): | ||
| if hasattr(v, "__dataclass_fields__"): | ||
| result.update(collect_presets(v, preset_path)) |
There was a problem hiding this comment.
🟢 Nice fix — This correctly discovers PresetCfg nodes inside dict-valued alternatives (e.g. default={"strategy_a": InnerTermCfg()} where InnerTermCfg contains further PresetCfg fields). Without this, deeply nested presets inside dict alternatives were silently missed.
Minor note: the dict key (k in for k, v in alt.items()) isn't included in the recursive preset_path, so the discovered paths won't include the dict key segment. This works because _walk_cfg handles full path construction during resolution, and collect_presets paths are only used for override matching (where the dict key is part of the parent's path). Just flagging for awareness.
| while isinstance(val, PresetCfg): | ||
| if id(val) in seen: | ||
| raise ValueError( | ||
| f"Cyclic PresetCfg chain detected at '{_path}': {type(val).__name__} was already visited." |
There was a problem hiding this comment.
🟢 Previous concern resolved — The cycle detection via seen: set[int] correctly handles the case where a chained PresetCfg loops back to a previously visited node. Combined with _pick_alternative raising ValueError for no-default presets, this eliminates both the infinite loop and the silent failure modes from the previous implementation.
3395321 to
ad84bd6
Compare
…m#5177) # Description - Add `resolve_presets(cfg, selected)` — a single-pass function that walks the config tree and replaces each `PresetCfg` with the CLI-selected or default alternative. This fixes bugs where presets inside dict-valued alternatives and `PresetCfg(default=None)` were not being discovered or resolved, causing deeply nested configs to silently use wrong defaults. - Unknown or unresolvable preset names now raise `ValueError` with a grouped listing of available presets and the paths they affect. ## Type of change - Bug fix (non-breaking change which fixes an issue) - New feature (non-breaking change which adds functionality) ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there --------- Co-authored-by: Antoine RICHARD <antoiner@nvidia.com>
Description
Add
resolve_presets(cfg, selected)— a single-pass function that walks the config tree and replaces eachPresetCfgwith the CLI-selected or default alternative. This fixes bugs where presets inside dict-valued alternatives andPresetCfg(default=None)were not being discovered or resolved, causing deeply nested configs to silently use wrong defaults.Unknown or unresolvable preset names now raise
ValueErrorwith a grouped listing of available presets and the paths they affect.Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there