[PresetCLI] Refactor preset into reusable class#5535
Closed
hujc7 wants to merge 15 commits into
Closed
Conversation
github-actions
Bot
added
documentation
hujc7
changed the title
hujc7
changed the title
hujc7
changed the title
6 tasks
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Well-architected refactor that elevates preset selection from a Hydra implementation detail to a first-class CLI concept. The three-layer design (registry → cfg → CLI) provides good separation of concerns.
Strengths
preset_namedecorator keeps canonical names co-located with their cfg classes — prevents vocabulary drift- Kind taxonomy via class inheritance rather than name-matching is robust and extensible
- Backwards compatibility preserved: legacy
presets=...override andfrom isaaclab_tasks.utils.hydra import PresetCfgstill work - Comprehensive test coverage (616 lines) with edge cases for legacy aliases, unknown presets, and multi-kind validation
- Clean API surface:
setup_cli(parser)one-liner for script authors
Concerns
See inline comments. High-level:
- The
_BACKEND_MODULESforce-import list is a maintenance liability — adding a new backend requires editing this list PresetCli._task_loaderas a mutable class variable is a testing footgun (global state shared across test cases)- The
_user_stacklevel()function is duplicated across modules
| "isaaclab_newton.physics.kamino_manager_cfg", | ||
| "isaaclab_physx.renderers.isaac_rtx_renderer_cfg", | ||
| "isaaclab_newton.renderers.newton_warp_renderer_cfg", | ||
| "isaaclab_ov.renderers.ovrtx_renderer_cfg", |
There was a problem hiding this comment.
🟡 Hardcoded backend module list: _BACKEND_MODULES requires manual updates when new backends are added. Consider using entry points or a plugin registry pattern so backends self-register:
# In each backend's __init__.py or cfg module:
from isaaclab.utils.preset_meta import PresetRegistry
PresetRegistry.register(PresetKind.PHYSICS, "physx")(PhysxCfg)Then _ensure_backends_imported could discover registered entry points instead of maintaining a hardcoded list. This is a longer-term suggestion — the current approach is fine for now given the small number of backends.
| """ | ||
| cls._task_loader = loader | ||
|
|
||
| # -- public API: per-script wiring ------------------------------------ |
There was a problem hiding this comment.
🟡 Mutable class variable for task loader: _task_loader: ClassVar[Any] = None is mutable global state that persists across test cases. If tests register different loaders or forget to reset, they can leak state.
Consider using a contextmanager or explicit reset for testing:
@classmethod
@contextlib.contextmanager
def override_task_loader(cls, loader):
prev = cls._task_loader
cls._task_loader = loader
try:
yield
finally:
cls._task_loader = prev| the helper that emitted the warning. | ||
|
|
||
| Kept module-private because it inspects ``__file__`` of *this* module; | ||
| the call chain on the CLI side has its own copy in ``hydra.py``. |
There was a problem hiding this comment.
💡 _user_stacklevel duplication: This helper appears in both preset_cfg.py and hydra.py. Consider extracting it to a shared utility (e.g., isaaclab.utils._warnings):
# isaaclab/utils/_warnings.py
def stacklevel_outside(module_file: str, max_walk: int = 16) -> int: ...Minor, but reduces the chance of the two implementations drifting.
| obj.legacy_aliases = dict(legacy_aliases) if legacy_aliases else {} | ||
| return obj | ||
|
|
||
| PHYSICS = ("physics", {"newton": "newton_mjwarp", "kamino": "newton_kamino"}) |
There was a problem hiding this comment.
💡 PresetKind enum with dual-value constructor: The __new__ override with (value, legacy_aliases) tuple is clever but non-obvious. A first-time reader seeing PHYSICS = ("physics", {"newton": "newton_mjwarp", ...}) might not immediately understand the structure.
Consider adding a brief code comment above the first member:
# Members: (label_for_CLI_flag, {deprecated_alias: canonical_replacement})
PHYSICS = ("physics", {"newton": "newton_mjwarp", "kamino": "newton_kamino"})| from isaaclab.envs.utils.spaces import replace_env_cfg_spaces_with_strings, replace_strings_with_env_cfg_spaces | ||
|
|
||
| # ``configclass`` is imported here (and re-exported via __all__) so existing code that does | ||
| # ``from isaaclab_tasks.utils.hydra import configclass`` keeps working after the preset |
There was a problem hiding this comment.
💡 Good backwards compat: Re-exporting PresetCfg, preset, and collect_presets from their new home while keeping the old import path working is the right call. Consider adding a deprecation note in the module docstring or via __deprecated__ so IDEs can flag the old imports:
# Deprecated re-exports — use isaaclab.utils.preset_cfg directly in new code.Minimal layer on top of the existing Hydra preset flow. The typed flags
translate to the same ``presets=<csv>`` token the develop-branch
register_task / apply_overrides path already understands; no changes to
hydra.py, PresetCfg, or the per-target override semantics.
Components:
- ``isaaclab/utils/preset_registry.py`` (new): ``PresetTarget`` enum
(PHYSICS / RENDERER / DOMAIN) with per-member legacy alias dicts,
plus ``PresetRegistry`` class holding the ``{target: {name: cls}}``
map and the ``register`` decorator. Single source of truth for
targets and their canonical names.
- ``isaaclab_tasks/utils/preset_cli.py`` (new): ``setup_cli(parser)``
function. Adds one argparse flag per PresetTarget (DOMAIN gets the
``--presets`` catch-all CSV; others get ``--{target.value}``).
Loads the task's env config from ``--task=X`` to populate the
registry, validates each typed flag against it (rejects unknown
names with a list of valid options), normalizes legacy aliases with
a single FutureWarning, then folds everything into one
``presets=<csv>`` token at the front of ``sys.argv``. A custom
``_HelpAction`` enriches ``--help`` output with the task's actual
registered preset names.
- 7 backend cfg files: ``@register(PresetTarget.X, "name")``
decorator above the existing ``@configclass``. Two new lines per
file (decorator + import). The decorator attaches ``_preset_name``
/ ``_preset_target`` to the class.
- ``isaaclab_tasks/test/test_preset_cli.py`` (new): 17 tests covering
the translator, decorator binding, validation, alias normalization,
``--help`` enrichment, and a cross-env drift lint that walks every
registered Isaac- task to check PresetCfg subclasses use canonical
field names.
What this does NOT do (intentionally): it does not refactor
``hydra.py``, does not move ``PresetCfg`` out of
``isaaclab_tasks.utils.hydra``, and does not introduce a ``PresetCli``
class, ``PresetOverrides`` dataclass, ``_PresetRequest`` dataclass, or
parser-state IntFlag. The translator is a single function. Adding a
new preset target = appending one ``PresetTarget`` enum member; no
second list elsewhere needs updating.
Tests: 93 passed (76 original hydra tests + 17 new).
hujc7
force-pushed
the
jichuanh/preset-cli
branch
from
fcf4134 to
f2683fc
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
Well-architected PR that introduces a decorator-driven preset registry (preset_registry.py) and typed CLI translator (preset_cli.py). Backend cfg classes self-register via @register(PresetTarget.PHYSICS, "physx"), and the CLI layer translates --physics physx into the existing presets=<csv> Hydra token. The module placement is correct (registry in isaaclab.utils to avoid circular deps, CLI in isaaclab_tasks.utils), backwards compatibility is preserved, and tests are comprehensive. Three issues should be addressed before merge.
Design Assessment
Design is sound. The decorator-driven registry is the right pattern — canonical names live next to the cfg class, the CLI layer has no hardcoded vocabulary, and the existing presets= Hydra flow is unchanged. Module placement correctly respects the dependency DAG.
Findings
🟡 Warning: _load_task_backends catches all exceptions silently — source/isaaclab_tasks/isaaclab_tasks/utils/preset_cli.py:59-66
The bare except Exception: pass swallows ImportError, SyntaxError, and any other error from backend cfg imports. When a backend fails to load (e.g., missing dependency, broken import), the user sees "(no backends registered for this task)" instead of the actual error. This is especially confusing when combined with _validate_typed_flag — a user typing --physics physx gets rejected with a misleading "not recognized" message when the real problem is a broken import.
Recommendation: Narrow the catch to expected failures, or at minimum log the exception:
except (ValueError, KeyError):
pass # Unknown task — Hydra will surface later
except Exception as exc:
import logging
logging.getLogger(__name__).debug(
"Failed to load backends for %r: %s", task_name, exc
)🟡 Warning: Duplicate legacy alias definitions — two sources of truth — source/isaaclab/isaaclab/utils/preset_registry.py:60
The legacy alias map {"newton": "newton_mjwarp", "kamino": "newton_kamino"} now exists in PresetTarget.PHYSICS.legacy_aliases AND in _LEGACY_PRESET_ALIASES in hydra.py (unchanged by this PR). These are consumed by different code paths — the new CLI (PresetTarget.normalize()) and the existing Hydra flow (_normalize_preset_name()). If a maintainer adds a new alias, they must update both locations or behavior diverges between --physics newton and presets=newton.
Recommendation: Either make hydra.py read from PresetTarget.legacy_aliases (single source of truth), or add a CI test asserting the two maps are consistent.
🟡 Warning: Missing __init__.pyi / __all__ updates — new modules not discoverable via top-level imports
The codebase uses lazy_loader.attach_stub() which resolves exports from .pyi stubs. Without updates to isaaclab/utils/__init__.pyi and isaaclab_tasks/utils/__init__.pyi, from isaaclab.utils import PresetTarget won't work — users must use the full submodule path. The backend cfg files already use full paths so this isn't a runtime blocker, but setup_cli especially should be a top-level export of isaaclab_tasks.utils.
🔵 Suggestion: Add PresetRegistry._reset_for_testing() classmethod — source/isaaclab/isaaclab/utils/preset_registry.py
The mutable ClassVar _entries is process-global state populated by import-time side effects. The test test_register_rejects_duplicate_binding permanently adds _test_unique_a to the registry. While the current tests work (force-imports at module level), a cleanup mechanism would improve test isolation for future tests and avoid issues with pytest --count=2 or parallel test runners.
🔵 Suggestion: stacklevel=3 in normalize() may point to internal code — source/isaaclab/isaaclab/utils/preset_registry.py:85
The FutureWarning about deprecated aliases uses stacklevel=3. Depending on the call depth from _validate_typed_flag → normalize(), this may point to preset_cli.py internals rather than the user's script. Verify the warning attribution points to a useful location for the user.
Test Coverage
- CLI translation and flag merging: ✅ Thorough (10+ tests)
- Registry decorator and validation: ✅ Good
- Legacy alias normalization: ✅ Covered
- Drift detection across all tasks: ✅ Excellent CI guard
--rendererflag in isolation:⚠️ Only tested in combination_load_task_backendsfailure path: ❌ Not tested- Drift test minimum-tasks guard:
⚠️ Could pass vacuously if no tasks load
CI Status
Most checks pending.
Verdict
Minor fixes needed
Excellent architecture and implementation. The three warning-level items (silent except, duplicate alias maps, missing stub exports) should be addressed to prevent maintenance drift and debugging confusion. The suggestion items are non-blocking improvements.
A field name on a PresetCfg, e.g. ``newton_mjwarp_strict: MjwarpCfg`` alongside the canonical ``newton_mjwarp: MjwarpCfg``, is now accepted as a valid ``--physics`` value even though it isn't in the global PresetRegistry. ``setup_cli`` unions ``PresetRegistry.names_for(target)`` with field names harvested from the task's PresetCfg instances when validating typed flags and rendering ``--help``. The drift lint loosens accordingly: each group of fields that hold values of the same registered class must include at least one canonical-named anchor; other names in the group are accepted variants. A standalone variant without a canonical anchor still fails the lint. Other review fixes: - Drop the _HelpAction subclass; bake the listing into argparse ``help=`` strings via a pre-parse ``--task`` scan. - ``_load_task_env_cfg`` writes load failures to stderr instead of swallowing them silently, so a typoed ``--task`` surfaces the real error. - ``@register`` first-wins on chained decoration of the same class (a second ``@register`` no longer silently overwrites the first class's ``_preset_name``). - ``stacklevel=4`` on the legacy-alias FutureWarning so it lands on the user's ``setup_cli`` call rather than inside this module.
A PresetCfg whose only fields for a given backend class are variants
(no canonical-named anchor) is now rejected by the CLI as well as
the drift lint. Previously _collect_task_variants harvested every
field name unconditionally, so e.g.
@configclass
class PhysicsCfg(PresetCfg):
default: ... = MISSING
newton_mjwarp_strict: MjwarpCfg = MjwarpCfg(...) # variant only
made --physics newton_mjwarp_strict accept silently. The CLI now
groups fields by the value's canonical name within each PresetCfg
and drops the group unless at least one field name equals that
canonical (matching the cross-env drift lint).
The drift lint additionally flags name/type mismatch: a field whose
name is a registered canonical AND whose value's class is registered
under a different canonical (e.g. ``physx: OvrtxRendererCfg``) is
drift. The check only fires when both sides claim canonical
identities, so the IsaacLab dispatch pattern -- canonical names like
``physx`` / ``newton_mjwarp`` holding arbitrary backend-tuned values
(ArticulationCfg, RigidObjectCfg, ...) -- still passes.
The variant walker now reads class-level field values over instance values (matching ``isaaclab_tasks.utils.hydra._preset_fields``) and picks up dynamic class attrs added outside the dataclass mechanism. Robot-specific cfg modules reassign PresetCfg fields at class scope after instantiation; the typed-flag layer would previously reject names the resolver was about to apply. Other adjustments from review: - ``_validate_typed_flag`` checks task variants before legacy-alias normalization, so a real field named ``newton`` or ``kamino`` on the task's ``PresetCfg`` shadows the deprecated alias and passes through unchanged (no FutureWarning). - ``setup_cli`` falls back to the pre-scan task name when ``args.task`` is absent (subparser layouts). Retry path only re-loads the env cfg when the pre-scan didn't already produce one, avoiding a redundant double-load when the task has no variants. - ``by_canonical`` is keyed by ``(target, canonical)`` so future name reuse across targets can't cross-validate variants. - ``register`` docstring rewritten to accurately describe the per-class stamp behavior: chained decoration of the same class is first-wins (inner decorator stamps, outer skips), while a decorated subclass gets its own canonical (parent's value is inherited via MRO but absent from the subclass ``__dict__``). - Drift lint hard-fails on any task that couldn't be loaded for inspection unless the failure matches a tolerated environment pattern (``No module named 'carb'`` for headless / CI runs). - ``.skip`` changelog fragments for the four backend packages that gained ``@register`` decorator imports (the user-facing feature is in the ``isaaclab_tasks`` fragment). - New tests: chained-decoration first-wins, subclass-gets-own-canonical, class-level field reassignment picked up by the variant walker, and task-local field shadowing a deprecated alias.
…task change The variant walker now mirrors the resolver's traversal not only when collecting alternatives off a single ``PresetCfg`` but also when recursing into nested dataclass children. Without this a ``PresetCfg`` installed as a class-level override on a parent dataclass was reachable to the resolver but invisible to the variant walker. The cross-env drift lint had the same blind spot and is fixed for the same reason. Other adjustments: - ``_preset_alternatives_view`` is renamed to ``_resolver_view`` and generalized to any node with ``__dataclass_fields__`` so it can be shared by the variant walker, the recursion path, and the drift lint. The rule is unchanged: declared fields read class-over-instance and class-only attrs are picked up provided they aren't dunder / callable / already covered. - ``setup_cli`` now tracks which task name was used to populate ``env_cfg`` and reloads when the parsed ``args.task`` differs from it (subparser layouts, repeated ``--task`` flags). A successful load with no variants for the same task is no longer mistaken for a failure that would retrigger. - Two end-to-end resolver assertions added so the class-level override test and the alias-shadowing test verify ``resolve_presets`` actually applies the expected alternative, not just that the typed-flag layer rewrote ``sys.argv``.
Splits the single helper from round 2 into two views matching how Hydra actually traverses the cfg tree: * ``_preset_alternatives_view`` (class-over-instance, plus class-only attrs) -- used ONLY when reading alternatives off a single ``PresetCfg`` node. Mirrors ``hydra._preset_fields``, which is what the resolver uses to pick which alternative to apply. * ``_walk_cfg_items`` (``dir`` + ``getattr`` -> instance-first) -- used for tree recursion when descending through a parent dataclass to find nested ``PresetCfg`` nodes. Mirrors ``hydra._walk_cfg``, which is what ``resolve_presets`` uses to find them. Conflating the two would let the typed-flag layer accept variant names from a class-level ``PresetCfg`` override on a parent field that the resolver (instance-first) never reaches, so ``--physics X`` would be advertised but never applied. The cross-env drift lint is fixed for the same reason. Other adjustments: - ``setup_cli`` now resets ``task_variants`` and ``loaded_task`` from the result of every reload, success or failure. Previously a failed reload after ``args.task`` differed from ``pre_task`` left the pre-scan task's variants in place, validating the new task's flags against stale alternatives. - The alias-shadowing test moved from ``--renderer`` to ``--physics`` and uses ``newton`` as a real-alias variant alongside the ``newton_mjwarp`` canonical, so the variants-first short-circuit is exercised on a target that actually has aliases. - New regression: a class-level reassignment of a parent dataclass field is invisible to ``getattr``-based traversal, and the variant walker must also skip it. - New regression: when ``--task`` is repeated and the second load fails, the typed flag is rejected rather than validated against the first task's variants.
…natives When the variant walker is INSIDE a ``PresetCfg`` and recurses to find nested ``PresetCfg`` variants, it now reads alternatives via ``_preset_alternatives_view`` (class-first) -- matching how the resolver picks an alternative -- and only THEN descends into each alternative's subtree via ``_walk_cfg_items`` (instance-first). Previously the recursion used ``_walk_cfg_items`` directly on the ``PresetCfg``, which returned the instance value of each declared field via ``getattr`` and so missed any class-level override of an alternative. A class-level reassignment of a ``PresetCfg`` declared field (a ``PresetCfg`` alternative) whose new value carries nested ``PresetCfg`` variants would otherwise be advertised by the typed-flag layer only at the top level: the override was read class-first when collecting names, but the subtree was descended instance-first, missing the nested variants. The cross-env drift lint had the same blind spot and is fixed identically. Other adjustments: - ``_extract_task_from_argv`` returns the LAST occurrence of ``--task`` rather than the first, matching argparse's last-wins semantics for repeated single-value flags. ``--help`` exits before the post-parse reload path, so the pre-scan task name needs to agree with what argparse would have used. - New regression test for nested ``PresetCfg`` under a class-level alternative override on the parent ``PresetCfg``.
``_extract_task_from_argv`` now stops at argparse's ``--`` end-of-options marker so a ``--task`` token after ``--`` (which argparse treats as a positional argument) doesn't override the one before it. The docstring also documents the abbreviation limitation: argparse's default ``allow_abbrev=True`` accepts ``--tas Foo``, but the pre-scan only recognizes the literal ``--task``. The non-help path still works (``args.task`` triggers the post-parse reload), but ``--help`` exits before that runs, so abbreviated invocations show generic help. Use the full ``--task`` spelling for ``--help``, or pass ``allow_abbrev=False`` to the parser. Tests: refreshed the failed-load test's docstring (LAST-wins makes the pre-scan and argparse agree on the same value, so the test now exercises the simpler "load fails -> reject" path rather than a stale mismatch). Added two unit tests on ``_extract_task_from_argv``: one covering the ``--`` boundary, one covering LAST-wins.
…arse ``--task --`` is a syntax error to argparse (the option's value is missing). The previous pre-scan happily consumed ``--`` as the task value and triggered a bogus preload + warning before argparse emitted the real error. The scan now refuses to read ``--`` as the value, so argparse's diagnostic is the only message the user sees. New unit test covers exactly that argv shape.
The enum metaclass collects the whole class namespace before instantiating members, so the ``__new__`` override is picked up regardless of where it appears. Members-first reads more naturally for an enum class: the public vocabulary is what you see first, and the constructor lives below as machinery.
Records a design idea explored during review: lifting the ``if target is PresetTarget.DOMAIN`` branches in ``setup_cli`` into per-kind classes held as enum values. The refactor is roughly line-neutral but improves cohesion (each kind's behavior lives on its own class). Not pursued in this PR because the structural cost isn't justified by the current 3 branches. Worth revisiting when a third kind appears (logger / teleop / curriculum) that doesn't fit either of the existing shapes.
The four flat helpers ``_canonical_and_target``, ``_preset_alternatives_view``, ``_walk_cfg_items``, and ``_collect_task_variants`` are now methods of a single ``_CfgTree`` namespace class with a clear shape: * Two views (``preset_alternatives_view`` and ``walk_cfg_items``) match the two phases of the resolver's traversal (class-first inside a PresetCfg, instance-first outside). * A single traversal primitive ``walk_presets`` switches between the views by node kind and calls a visitor at every PresetCfg. * ``collect_task_variants`` is a thin visitor on top of ``walk_presets`` applying the strict-anchor rule. The drift lint in tests previously duplicated the same traversal in ``_walk_preset_cfgs`` and the canonical-name lookup in ``_canonical_for``. Both duplicates are deleted; the lint now calls ``_CfgTree.walk_presets`` and ``_CfgTree.canonical_and_target`` directly, so the production and lint paths cannot diverge on what counts as "reachable" or what counts as "registered." The argv-pre-scan helpers (``_extract_task_from_argv``, ``_load_task_env_cfg``) and the flag handlers (``_help_text``, ``_validate_typed_flag``) stay flat -- they're only used by ``setup_cli`` and have no cross-file reuse to consolidate.
Three sections inside the class -- "Decoration lookup", "Per-node views", "Tree traversal" -- make the grouping visible without reading docstrings. The class docstring summarizes the layout up front so jumping into the file shows the shape at a glance.
…lpers Four sections delimit the file: 1. Pre-parse setup: scan argv for ``--task``, load that task's env config. 2. Cfg-tree introspection: the ``_CfgTree`` class. 3. Per-flag handling: ``_help_text`` and ``_validate_typed_flag``. 4. Entry point: ``setup_cli``. Within section 3, ``_help_text`` now precedes ``_validate_typed_flag`` so the definition order matches ``setup_cli``'s call order (help text is consumed during ``add_argument``; validation runs after ``parse_known_args``). Inside ``_CfgTree`` the existing order is already callee-before-caller: the three leaf helpers, then ``walk_presets`` (which calls the views), then ``collect_task_variants`` (the visitor on top of ``walk_presets``).
The typed-flag layer no longer maintains a parallel cfg-tree introspection codebase. Everything that touches the preset semantics (reading alternatives off a PresetCfg, walking the cfg tree, identifying the registered class behind a value, resolving alternatives, normalizing legacy aliases) now lives in one place. The preset-cli module collapses to the argparse adapter it was always supposed to be. Concrete moves: * ``_preset_fields(preset_obj)`` -> ``PresetCfg.alternatives()`` method. * ``_preset_dominant_targets(preset_obj)`` -> ``PresetCfg.dominant_targets()``. * ``canonical_and_target(value)`` -> ``PresetRegistry.canonical_and_target`` (registry-side lookup belongs on the registry). * ``_walk_cfg`` / ``walk_presets`` -> ``CfgTree.walk_cfg`` / ``CfgTree.walk_presets`` static methods. Two walkers, two purposes, one home: ``walk_cfg`` is the single-sweep visit-once walker used by resolution and ``collect_presets``; ``walk_presets`` also descends through PresetCfg alternatives, used by ``collect_task_variants`` and the cross-env drift lint. * ``collect_task_variants(env_cfg)`` moved into ``hydra.py`` as a thin visitor on ``CfgTree.walk_presets``. * Legacy alias data: ``_LEGACY_PRESET_ALIASES`` constant deleted; callers read ``PresetTarget.all_legacy_aliases()`` directly so the per-target tables on the enum members are the single source of truth. Behavior changes (not just code moves): * ``_pick_alternative`` is now target-aware. When a selected name targets one of the PresetCfg's targets but isn't a field on this preset, raises with the available alternatives instead of silently falling back to ``default``. This closes the silent-fallback gap at the resolution layer where it belongs (every caller of ``resolve_presets`` benefits, not just ``setup_cli``). * ``_load_task_env_cfg`` narrows its catch from ``Exception`` to ``(ImportError, ModuleNotFoundError)``. A bad task name, a buggy task-config module, or a config ``__init__`` that raises now propagates as the real error rather than being hidden behind a misleading "not a recognized preset" downstream. * The ``--task is required`` error message derives the typed-flag list from ``PresetTarget`` so it doesn't drift when a new typed target is added. Other adjustments from review: * Pre-scan stops at argparse's ``--`` end-of-options marker and rejects ``--task --`` (no value -- argparse syntax error). * Argparse-abbreviation guard rejects ``--tas Foo`` rather than silently reloading; the pre-scan and the parsed value must agree. * Drift lint hard-fails on tasks that couldn't be loaded for inspection unless the failure matches a tolerated environment pattern (carb missing in CI). * ``setup_cli`` reload path collapses: pre-scan task is the single source of truth; no defensive secondary load. Tests + lint: section headers grouping the file by purpose; the ``_drift_violations`` lint and the ``test_preset_cli`` suite call into the hydra-side helpers directly so there's only one implementation of every check. Tests get a default ``load_cfg_from_registry`` stub on the ``stub_app_launcher`` fixture so placeholder task ids like ``Foo-v0`` don't surface as real gym ``NameNotFound`` (the narrowed catch makes those propagate now -- correct for end users, noise for tests that only exercise CLI mechanics).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal
Make preset selection a first-class CLI surface. Replace bare
presets=newton_mjwarpHydra-style overrides with typed flags (--physics,--renderer,--presets) that show up in--help, validate before Hydra runs, and migrate legacy names with a deprecation warning.The PR is a small additive layer on top of the existing Hydra preset flow. It does not refactor
hydra.py, movePresetCfg, or change the override semantics. Typed flags translate to the samepresets=<csv>token thatregister_task/apply_overridesalready understand.Design (single source of truth)
PresetTargetenum — every CLI-flag category lives here, with its legacy aliases attached to the enum member:Adding a new flag = appending one enum member.
setup_clidiscovers it viafor target in PresetTarget; no second list elsewhere needs updating.PresetRegistryclass — container +registerdecorator + lookups, all together:Backend cfg classes declare themselves once:
setup_cli(parser)— single function inisaaclab_tasks/utils/preset_cli.py. Adds one argparse flag perPresetTarget(DOMAINgets--presetscatch-all CSV; others get--{target.value}). Loads the task's env config from--task=Xto populate the registry, validates each typed flag against it, normalizes legacy aliases with a singleFutureWarning, then folds everything into onepresets=<csv>token at the front ofsys.argv.Before / after
CLI surface
python train.py --task=... presets=newton_mjwarp,newton_renderer(Hydra-style, no--, undiscoverable in--help)python train.py --task=... --physics=newton_mjwarp --renderer=newton_renderer. Both--flag valueand--flag=valuework. Legacypresets=...form still works.--helplisting--help.--task=<X> --helplists per-task valid names per flag.newton)FutureWarning+ auto-rewrite tonewton_mjwarp.Error message (after)
--help (after)
Env author experience
Same
PresetCfgsyntax. New CI lint walks the gym registry and asserts that every PresetCfg subclass uses canonical field names where the alternative's value type is bound to a canonical (target, name). Catches drift likefoo: PhysxCfg = PhysxCfg()(instead of the canonicalphysx:).Backend author experience
Adding a new physics solver:
@register(PresetTarget.PHYSICS, "newton_fastsolver")--help, validation accepts it, drift lint approves field namingNo changes needed to
preset_cli.pyor any vocabulary list.What's not changed
env.sim.dt=0.001,agent.seed=42) untouched.presets=newton_mjwarp,...global broadcast still works exactly as before.PresetCfgstays where it is inisaaclab_tasks/utils/hydra.py; no relocation.hydra.pyis not modified by this PR.setup_cli(parser)in place of their existing argparse boilerplate.Files changed (10 files, +714/−0)
source/isaaclab/isaaclab/utils/preset_registry.pysource/isaaclab_tasks/isaaclab_tasks/utils/preset_cli.py@register)physx_manager_cfg.py,ovphysx_manager_cfg.py,mjwarp_manager_cfg.py,kamino_manager_cfg.py,isaac_rtx_renderer_cfg.py,newton_warp_renderer_cfg.py,ovrtx_renderer_cfg.pysource/isaaclab_tasks/test/test_preset_cli.py(17 tests, including the cross-env drift lint)Test plan
./isaaclab.sh -p -m pytest source/isaaclab_tasks/test/test_preset_cli.py— 17/17 passed./isaaclab.sh -p -m pytest source/isaaclab_tasks/test/test_hydra.py— 76/76 passed (unchanged)./isaaclab.sh -f) clean--task=Isaac-Velocity-Flat-Anymal-C-v0 --helplists--physics: newton_mjwarp, physx--physics=super_solverrejected pre-Hydra with the actual valid names--physics=newton(legacy) emitsFutureWarningand rewrites tonewton_mjwarpReference: alternative design preserved
A larger refactoring iteration of this work — including
PresetCliclass,PresetOverrides/_PresetRequestdataclasses,_ParserStateIntFlag, hydra.py refactor,PresetCfghoisted to core, naming-pass globally — is preserved on a separate branch for reference: https://github.com/hujc7/IsaacLab/tree/jichuanh/preset-cli-full-synthesis. That branch is not part of this PR; it exists only as a record of what an end-to-end refactor of the preset surface would look like.