[PresetCLI] Refactor preset into reusable class

[hujc7]

Goal

Make preset selection a first-class CLI surface. Replace bare presets=newton_mjwarp Hydra-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, move PresetCfg, or change the override semantics. Typed flags translate to the same presets=<csv> token that register_task / apply_overrides already understand.

Design (single source of truth)

PresetTarget enum — every CLI-flag category lives here, with its legacy aliases attached to the enum member:

class PresetTarget(enum.Enum):
    PHYSICS  = ("physics", {"newton": "newton_mjwarp", "kamino": "newton_kamino"})
    RENDERER = ("renderer",)
    DOMAIN   = ("domain",)

Adding a new flag = appending one enum member. setup_cli discovers it via for target in PresetTarget; no second list elsewhere needs updating.

PresetRegistry class — container + register decorator + lookups, all together:

class PresetRegistry:
    _entries: ClassVar[dict[PresetTarget, dict[str, type]]] = {}

    @classmethod
    def register(cls, target, name): ...
    @classmethod
    def names_for(cls, target): ...

register = PresetRegistry.register   # decorator alias

Backend cfg classes declare themselves once:

# source/isaaclab_physx/isaaclab_physx/physics/physx_manager_cfg.py
@register(PresetTarget.PHYSICS, "physx")
@configclass
class PhysxCfg(PhysicsCfg): ...

setup_cli(parser) — single function in isaaclab_tasks/utils/preset_cli.py. Adds one argparse flag per PresetTarget (DOMAIN gets --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, normalizes legacy aliases with a single FutureWarning, then folds everything into one presets=<csv> token at the front of sys.argv.

Before / after

CLI surface

	Before	After
Selecting a backend	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 value and --flag=value work. Legacy presets=... form still works.
--help listing	preset names absent	typed flags appear in --help. --task=<X> --help lists per-task valid names per flag.
Bad name	Hydra error mid-run	argparse rejects pre-Hydra with the actual valid names for the task.
Legacy name (newton)	silent at the CLI	FutureWarning + auto-rewrite to newton_mjwarp.

Error message (after)

error: --physics 'super_solver' is not a recognized physics preset.
  Valid physics presets: newton_mjwarp, physx

--help (after)

$ python train.py --task=Isaac-Velocity-Flat-Anymal-C-v0 --help
usage: train.py [-h] [--task TASK] [--physics NAME] [--renderer NAME] [--presets NAME[,NAME,...]]

preset selection:
  --physics NAME            Physics preset name (use '--task=<X> --help' to list valid names).
  --renderer NAME           Renderer preset name (use '--task=<X> --help' to list valid names).
  --presets NAME[,NAME,...] Comma-separated free-form preset names...

available preset names (for task 'Isaac-Velocity-Flat-Anymal-C-v0'):
  --physics: newton_mjwarp, physx
  --renderer: (none for this task)

Env author experience

Same PresetCfg syntax. 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 like foo: PhysxCfg = PhysxCfg() (instead of the canonical physx:).

Backend author experience

Adding a new physics solver:

1. Define class as before
2. Add one decorator: @register(PresetTarget.PHYSICS, "newton_fastsolver")
3. Done — name appears in --help, validation accepts it, drift lint approves field naming

No changes needed to preset_cli.py or any vocabulary list.

What's not changed

• Hydra config-override semantics (env.sim.dt=0.001, agent.seed=42) untouched.
• The presets=newton_mjwarp,... global broadcast still works exactly as before.
• PresetCfg stays where it is in isaaclab_tasks/utils/hydra.py; no relocation.
• hydra.py is not modified by this PR.
• Scripts continue to work; users opt into typed flags by calling setup_cli(parser) in place of their existing argparse boilerplate.

Files changed (10 files, +714/−0)

Area	File
New: enum + registry	source/isaaclab/isaaclab/utils/preset_registry.py
New: typed-flag translator	source/isaaclab_tasks/isaaclab_tasks/utils/preset_cli.py
Backend decorators (7 × 2 lines each: import + @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.py
Tests	source/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)
• Pre-commit (./isaaclab.sh -f) clean
• --task=Isaac-Velocity-Flat-Anymal-C-v0 --help lists --physics: newton_mjwarp, physx
• --physics=super_solver rejected pre-Hydra with the actual valid names
• --physics=newton (legacy) emits FutureWarning and rewrites to newton_mjwarp
• Cross-env drift lint walks every registered Isaac task and reports drift

Reference: alternative design preserved

A larger refactoring iteration of this work — including PresetCli class, PresetOverrides/_PresetRequest dataclasses, _ParserState IntFlag, hydra.py refactor, PresetCfg hoisted 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.

[isaaclab-review-bot]

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_name decorator 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 and from isaaclab_tasks.utils.hydra import PresetCfg still 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:

1. The _BACKEND_MODULES force-import list is a maintenance liability — adding a new backend requires editing this list
2. PresetCli._task_loader as a mutable class variable is a testing footgun (global state shared across test cases)
3. The _user_stacklevel() function is duplicated across modules

[isaaclab-review-bot]

🟡 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.

[isaaclab-review-bot]

🟡 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

[isaaclab-review-bot]

💡 _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.

[isaaclab-review-bot]

💡 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"})

[isaaclab-review-bot]

💡 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.

[isaaclab-review-bot]

🤖 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
• --renderer flag in isolation: ⚠️ Only tested in combination
• _load_task_backends failure 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.