Log resolved task presets - #6908
Conversation
Keep the banner strings next to their only consumer so they do not add module-level state for an internal logging detail.
Greptile SummaryAdds retained metadata and INFO-level startup logging for presets selected during task resolution.
Confidence Score: 5/5The PR appears safe to merge with no actionable correctness, compatibility, or security issues identified. The retained metadata survives both task-resolution branches, remains excluded from config serialization, and is logged only after successful resolution. Important Files Changed
Reviews (1): Last reviewed commit: "Log resolved task presets" | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The preset-resolution accumulator and INFO banner are integrated into the existing traversal without an additional pass. One maintainability issue remains: the returned environment config is assigned an invented dunder attribute, contrary to the repository’s PEP 8 requirement.
- Design and architecture: Recording at
_pick_alternativecentralizes collection for environment and agent presets and follows the existing accumulator pattern. The metadata storage mechanism should use a non-dunder private name with an explicit serialization-exclusion contract rather than relying on special-name handling. - API: Existing resolver signatures remain backward compatible, but
register_tasknow mutates every returned environment config with the invented__resolved_presets__attribute. Rename this metadata field to a single-underscore private name and explicitly ensure it remains excluded from serialization. - Implementation: The accumulator is threaded through both environment and agent resolution and the banner is emitted after successful resolution on both
_run_hydrapaths. The focused test verifies retention, serialization exclusion, and logging, but should be updated alongside the metadata-field correction.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| else: | ||
| hydra_args.append(arg) | ||
|
|
||
| setattr(env_cfg, "__resolved_presets__", tuple(resolved_presets)) |
There was a problem hiding this comment.
🔵 Suggestion · Api — Invented dunder attribute on returned env config
Every config returned by register_task/resolve_task_config now carries an invented __resolved_presets__ name; PEP 8 (a repository rule) says never to invent double-leading-and-trailing-underscore names. Its exclusion from to_dict()/ConfigStore also relies implicitly on class_to_dict skipping __-prefixed keys rather than an explicit rule. Prefer a single-underscore name with explicit serialization exclusion, or document the dependency at the assignment site.
|
The resolved-preset banner is currently invisible in normal entrypoints. I reproduced this with |
|
Closing in favor of #6933 |
Description
Retains each active
PresetCfgchoice during the existing task-resolution walk and emits a compact INFO-level startup banner after Hydra resolution succeeds.The banner prints one line per resolved preset, including physics, renderer, domain, and agent presets. Resolution metadata is stored privately on the environment config as
__resolved_presets__and remains excluded from Hydra and YAML serialization.No new dependencies or additional configuration traversals are introduced.
Type of change
Screenshots
Not applicable; example output:
Testing
./isaaclab.sh -p -m pytest source/isaaclab_tasks/test/core/test_hydra.py::test_resolve_task_config_logs_and_retains_resolved_newton_preset -q(1 passed, no Kit or simulator launch)./isaaclab.sh -f__resolved_presets__is absent, then passes with the implementation.Checklist
./isaaclab.sh -fisaaclab_taskschangelog fragmentCONTRIBUTORS.md