Skip to content

[Cartpole] Consolidate Cartpole task IDs via preset CLI#5605

Open
hujc7 wants to merge 12 commits into
isaac-sim:developfrom
hujc7:jichuanh/cartpole-presets
Open

[Cartpole] Consolidate Cartpole task IDs via preset CLI#5605
hujc7 wants to merge 12 commits into
isaac-sim:developfrom
hujc7:jichuanh/cartpole-presets

Conversation

@hujc7
Copy link
Copy Markdown
Collaborator

@hujc7 hujc7 commented May 13, 2026

1. Summary

  • 35 retired Cartpole task IDs collapse into 4 canonical tasks; variant selection via presets=NAME from [PresetCLI] Add typed preset selection with task-aware help #5587.
  • Retired IDs stay bit-for-bit identical to develop — each env_cfg_entry_point still points at the historical per-variant cfg; only a DeprecationWarning is layered on top via a new deprecated kwarg (a dict with an alias field holding the migration command) on gym.register.
  • Scope is deprecation only — consolidated cfgs reuse develop's existing per-variant subclasses as PresetCfg variants; no field-level cfg restructuring.
New task ID Replaces Variant selectors
Isaac-Cartpole-Camera-Direct-v0 7 perception IDs presets=<data_type>
Isaac-Cartpole-Camera-v0 4 manager perception IDs presets=<pipeline>, --agent rl_games_{,feature_}cfg_entry_point, physics= (inherited)
Isaac-Cartpole-Showcase-Direct-v0 15 proprio showcase IDs presets=<obs>_<action>, --agent skrl_<obs>_<action>_cfg_entry_point, physics= (inherited)
Isaac-Cartpole-Camera-Showcase-Direct-v0 9 camera showcase IDs presets=<obs>_<action>, --agent skrl_<obs>_<action>_cfg_entry_point, physics= (inherited)

Basic physics-only tasks (Isaac-Cartpole-v0, Isaac-Cartpole-Direct-v0) stay unchanged.

2. Migration map (each retired ID → new invocation)

2.1 Direct-backend perception (Isaac-Cartpole-Camera-Direct-v0)

Old task ID New invocation
Isaac-Cartpole-Camera-Presets-Direct-v0 --task=Isaac-Cartpole-Camera-Direct-v0
Isaac-Cartpole-RGB-Camera-Direct-v0 --task=Isaac-Cartpole-Camera-Direct-v0 presets=rgb
Isaac-Cartpole-Depth-Camera-Direct-v0 --task=Isaac-Cartpole-Camera-Direct-v0 presets=depth
Isaac-Cartpole-Albedo-Camera-Direct-v0 --task=Isaac-Cartpole-Camera-Direct-v0 presets=albedo
Isaac-Cartpole-SimpleShading-Constant-Camera-Direct-v0 --task=Isaac-Cartpole-Camera-Direct-v0 presets=simple_shading_constant_diffuse
Isaac-Cartpole-SimpleShading-Diffuse-Camera-Direct-v0 --task=Isaac-Cartpole-Camera-Direct-v0 presets=simple_shading_diffuse_mdl
Isaac-Cartpole-SimpleShading-Full-Camera-Direct-v0 --task=Isaac-Cartpole-Camera-Direct-v0 presets=simple_shading_full_mdl

Renderer selectable via renderer=NAME. Physics backend selectable via physics=NAME on the canonical task.

2.2 Manager-based perception (Isaac-Cartpole-Camera-v0)

Old task ID New invocation
Isaac-Cartpole-RGB-v0 --task=Isaac-Cartpole-Camera-v0 presets=rgb
Isaac-Cartpole-Depth-v0 --task=Isaac-Cartpole-Camera-v0 presets=depth
Isaac-Cartpole-RGB-ResNet18-v0 --task=Isaac-Cartpole-Camera-v0 --agent=rl_games_feature_cfg_entry_point presets=resnet18
Isaac-Cartpole-RGB-TheiaTiny-v0 --task=Isaac-Cartpole-Camera-v0 --agent=rl_games_feature_cfg_entry_point presets=theia_tiny

physics= is exposed via the inherited CartpoleEnvCfg.sim.physics PresetCfg.

2.3 Proprioceptive showcase (Isaac-Cartpole-Showcase-Direct-v0)

Old: Isaac-Cartpole-Showcase-<Obs>-<Action>-Direct-v0
New: --task=Isaac-Cartpole-Showcase-Direct-v0 --agent=skrl_<obs>_<action>_cfg_entry_point presets=<obs>_<action>

All 15 combinations: <Obs>{Box, Discrete, MultiDiscrete, Dict, Tuple}, <Action>{Box, Discrete, MultiDiscrete}. Lowercased on the presets= and --agent= sides. box_box matches the default skrl yaml so no --agent= is needed for it; every other variant needs an explicit --agent=.

Example — Isaac-Cartpole-Showcase-Dict-MultiDiscrete-Direct-v0
```bash
--task=Isaac-Cartpole-Showcase-Direct-v0 \
--agent=skrl_dict_multidiscrete_cfg_entry_point \
presets=dict_multidiscrete
```

2.4 Camera showcase (Isaac-Cartpole-Camera-Showcase-Direct-v0)

Same pattern as proprioceptive showcase, but only 3 observation containers (Box, Dict, Tuple) × 3 actions = 9 combinations.

3. Deprecation contract

Each retired task ID stays registered with its historical env_cfg_entry_point plus a new deprecated kwarg holding the migration metadata:

```python
gym.register(
id="Isaac-Cartpole-RGB-Camera-Direct-v0",
entry_point=f"{name}.cartpole_camera_env:CartpoleCameraEnv",
kwargs={
"env_cfg_entry_point": f"{name}.cartpole_camera_env_cfg:CartpoleRGBCameraEnvCfg",
"deprecated": {"alias": "--task=Isaac-Cartpole-Camera-Direct-v0 presets=rgb"},
...
},
)
```

isaaclab_tasks.utils.parse_cfg.load_cfg_from_registry reads spec.kwargs[\"deprecated\"][\"alias\"] when loading an env_cfg_entry_point and emits a DeprecationWarning naming the new command before returning the cfg unchanged. The dict shape is open for future fields (reason, removed_in, …) without renaming the top-level key. Removal scheduled for the next release cycle.

```bash

Before

./isaaclab.sh -p train.py --task=Isaac-Cartpole-RGB-TheiaTiny-v0

After (warning still fires for the old form during the deprecation window)

./isaaclab.sh -p train.py --task=Isaac-Cartpole-Camera-v0 \
--agent=rl_games_feature_cfg_entry_point presets=theia_tiny
```

4. Design notes

  • One preset = one bundled cfg. Showcase and manager-camera variants are exposed as concrete cfg instances (box_box, dict_discrete, rgb, depth, …) so a single presets= selection picks the whole shape. Matches the existing behavior of Isaac-Cartpole-Camera-Presets-Direct-v0.
  • Multi-agent registration on consolidated tasks. Where the old per-variant task pinned a specific yaml, the consolidated task registers all matching yamls as parallel agent entry points; users select via --agent=<name>.
  • No behavior change for callers of the deprecated IDs. The env_cfg_entry_point keeps pointing at the same historical per-variant cfg subclass; only a deprecation warning is layered on top via the deprecated kwarg.

5. Dependencies

Depends on #5587 (the typed physics= / renderer= / presets= CLI). Merged into develop; this branch is rebased.

6. Test plan

  • Pre-commit clean
  • test_preset_kit_decision.py (5 tests) passes locally — exercises the retired catch-all Isaac-Cartpole-Camera-Presets-Direct-v0 via the new flow
  • Smoke check verifies all 35 retired IDs carry deprecated={\"alias\": ...} and the warning text is correct
  • CI green on Docker + Tests

7. Out of scope / follow-ups

  • Cfg-class simplification / field-level PresetCfg restructuring — out of scope for this PR; tracked separately.
  • Categorize --help output for presets= — flat alphabetical list of variants doesn't separate physics/renderer/data axes. Belongs in [PresetCLI] Add typed preset selection with task-aware help #5587's preset_cli._help_text.
  • Surfacing per-variant docstrings in presets= help output — preset CLI enhancement.
  • Merging showcase tasks into their basic / camera counterparts (env-class merge — deferred per design decision).

@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label May 13, 2026
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

This is a well-structured refactoring PR that consolidates 27 per-variant Cartpole task IDs into 4 canonical tasks using the new typed preset CLI system. The implementation follows good patterns for deprecation handling and maintains backward compatibility.

✅ Architecture & Design

Strengths:

  • The consolidation approach is sound - using presets to select variants at runtime is cleaner than proliferating task IDs
  • The deprecation factory pattern (_deprecated_factory) provides a consistent migration path with clear warning messages
  • The PresetTarget enum is well-designed with per-target metadata (base_classes, legacy_aliases) allowing easy extension
  • Help-time variant bucketing by isinstance is robust and works correctly with wrapper classes (e.g., NewtonCfg)

Design Decision: Using concrete cfg instances as preset variants (one preset = one bundled cfg) is appropriate for showcase tasks where observation/action shapes cannot be composed from independent axes.

✅ Implementation Quality

Script Migration (16 files):

  • Clean replacement of parser.parse_known_args() with setup_preset_cli(parser)
  • Correct pattern: args_cli, hydra_args = setup_preset_cli(parser) followed by sys.argv = [sys.argv[0]] + hydra_args
  • Preserves external_callback flows in rsl_rl/{train,play}.py

Deprecation Handling:

  • Each retired task ID emits a DeprecationWarning with actionable migration guidance
  • The consolidated tasks load cleanly without spurious warnings
  • Behavior is preserved for existing callers during deprecation cycle

CLI Layer (preset_cli.py):

  • Pure translator design is correct - no validation at CLI time, only at resolve time
  • _ArgvHelper efficiently scans argv once before argparse runs
  • Proper deduplication with first-occurrence order preservation

✅ Test Coverage

  • 39 parametrized tests in test_cartpole_preset_deprecations.py lock each deprecation's exact warning body
  • Comprehensive CLI tests in test_preset_cli.py covering:
    • Flag folding into presets=<csv> token
    • Merge with existing presets token
    • Deduplication
    • sys.argv immutability contract (critical for external_callback)
    • Help text branch coverage for all variant shapes

✅ Update (e1eb51a)

Hydra-Style Preset Syntax Alignment:
The latest commits update the CLI flag syntax throughout the codebase from the old --presets=NAME style to Hydra-style presets=NAME (no leading dashes), aligning with the preset selector syntax from #5587:

  • Updated changelog documentation to use presets=NAME syntax
  • Updated deprecation warning messages to show new invocation style (e.g., presets=rgb instead of --presets=rgb)
  • Updated docstrings across all preset cfg modules
  • Updated test assertions in test_cartpole_preset_deprecations.py to match new expected warning strings

This is a consistency fix ensuring the documentation and warnings match the actual Hydra-style CLI interface.

📜 Previous Updates (9a558ed)

Bug Fix (64b87ae): Nested tiled_camera variant resolution

  • The deprecation shim now atomically resolves both the root observation_space variant and the nested tiled_camera variant

Camera Offset Restoration (6d4888b)

  • The per-variant camera offsets for albedo and simple-shading presets have been restored to match the historical per-variant cfg classes

⚠️ Minor Observations

  1. CI Status: Recommend waiting for CI green before merge

  2. Documentation: The changelog fragments are thorough and well-written. The PresetTarget docstring explaining the bucketing contract is helpful.

📋 Checklist Verification

  • ✅ Deprecation warnings include the old task ID and new invocation with Hydra-style syntax
  • ✅ Consolidated tasks register all matching agent yamls as parallel entry points
  • ✅ No behavior change for existing deprecated task ID callers
  • ✅ Nested preset resolution now correct for direct-camera tasks
  • ✅ Camera offsets match historical per-variant classes
  • ✅ Documentation and warnings aligned with Hydra-style presets=NAME syntax
  • ✅ Pre-commit clean (verified in CI)
  • ⏳ CI green pending
  • ⏳ Spot-train verification pending (per test plan)

Verdict: This is a clean, well-tested refactoring that improves the developer experience while maintaining backward compatibility. The latest commits ensure consistency between the Hydra-style CLI interface and all documentation/warnings. Approve once CI passes and spot-training is verified.


Update (84fdf54): New commits add determinism/reproducibility features (--deterministic flag for RTX rendering, configure_seed wiring in RL scripts). These changes are additive and unrelated to the task consolidation review above. No issues found in the new code — the implementation correctly places configure_seed() after runner/agent construction with explanatory comments, and includes comprehensive test coverage (AST-based call ordering verification + optional integration test). Previous review findings remain valid. ✅


Update (6f3c258): New commit is a merge from origin/develop into the feature branch — no changes to the PR's own files. The consolidation logic and tests remain unchanged from the previous review. Previous findings still valid. ✅


Update (4e0a2d0): Camera offset simplification — the _ROTATED_OFFSET helper is removed from cartpole_camera_presets_env_cfg.py and all camera presets (albedo, simple_shading_*) now use the default identity-offset camera pose. This aligns with develop's MultiDataTypeCartpoleTiledCameraCfg which chose a uniform pose for all variants. Tests updated accordingly to expect _IDENTITY_ROT for all deprecated direct-camera task IDs. This is the correct design decision: users of retired task IDs will see the consolidated task's camera orientation going forward — the deprecation shim returns the consolidated cfg's variant, not a historical recreation. ✅


Update (2595dfb): Code cleanup — deprecated_task_alias() calls now use keyword arguments (old_task_id=, new_command=, consolidated_cfg_path=) instead of positional args, improving readability. Module-level constants (_CAMERA_CFG_PATH, _SHOWCASE_CFG_PATH, _CAMERA_KWARGS) are removed and values inlined at each call site. This is a stylistic improvement with no behavioral change — each gym registration is now fully self-contained. ✅


Update (d6e8a21): Deprecation infrastructure simplification — deprecated_task_alias() API is refactored to be cleaner and more consistent:

  1. new_command now takes a list instead of a string (e.g., [\"--task=X\", \"presets=Y\"] instead of \"--task=X presets=Y\"). This avoids error-prone string parsing.

  2. cfg_factory parameter removed — the _resolve_camera_variant() helper in cartpole/__init__.py is deleted. All preset resolution now goes through the centralized resolve_presets() function from isaaclab_tasks.utils.hydra, which correctly walks nested PresetCfg trees.

  3. Convention established for token ordering: --task= first, --agent= when present, presets= last. Test helper _showcase_migration() documents this.

  4. Test expected strings updated to reflect the new ordering convention.

This is a clean internal refactoring with no user-facing behavior change — deprecated tasks still emit the same warnings (modulo minor reordering of --agent and presets= in some messages) and resolve to the correct cfg variants. The removal of per-call-site cfg_factory callbacks in favor of resolve_presets() is the right design: DRY and less room for bugs. ✅


Update (bed1cdc): Design pivot for bit-for-bit backward compatibility in deprecated task IDs:

  1. cfg_factory replaces consolidated_cfg_path in deprecated_task_alias():

    • Previously: lazy import + resolve_presets() to pick a variant from the consolidated cfg
    • Now: cfg_factory=lambda: HistoricalCfgClass() returns the historical per-variant cfg subclass directly
  2. Historical cfg classes preserved: The retired task IDs (e.g., Isaac-Cartpole-RGB-Camera-Direct-v0) now return their original per-variant cfg classes (CartpoleRGBCameraEnvCfg, CartpoleAlbedoCameraEnvCfg, etc.) verbatim, rather than a resolved variant of the consolidated PresetCfg.

  3. Camera offsets restored: The Albedo and SimpleShading direct-camera deprecated task IDs now return their historical 180-degree rotated camera offset (_FLIPPED_ROT), preserving the pre-deprecation behavior that differed from the consolidated cfg's identity offset.

  4. resolve_presets() removed from shim: The factory is called directly — no nested PresetCfg resolution needed since the historical subclass is returned as-is.

  5. Test expectations updated: test_direct_camera_shim_returns_historical_per_variant_cfg now verifies the historical rotation values.

Rationale: This ensures retired task IDs produce bit-for-bit identical cfgs to their pre-deprecation behavior throughout the deprecation period. Only the deprecation warning is layered on top. Users migrating to the consolidated task with presets=<name> get the new unified camera pose; users sticking with retired IDs see no change.

This is the correct design: deprecated task IDs should be pure shims that emit warnings without changing any runtime behavior, giving users a clean migration path on their own timeline. The historical per-variant subclasses will be removed together with the retired task IDs in a future release. ✅


Update (26b2e62): Test fixture cleanup in test_visualizer_cartpole_integration.py — the _make_cartpole_camera_env() helper now uses resolve_presets() from isaaclab_tasks.utils.hydra to resolve nested PresetCfg fields in-place, replacing the previous fragile attribute lookup logic (getattr(env_cfg_root, \"default\", None)). This aligns the test setup with how the canonical gym task ID resolves presets at gym.make() time. Clean improvement, no issues. ✅


Update (fd19114): Lazy cfg imports to fix test isolation — the _lazy_cfg() helper defers cfg-class imports from module top-level to gym.make() time:

  1. Problem: Eagerly importing cfg classes (e.g., CartpoleRGBCameraEnvCfg) at __init__.py module top pulls isaaclab.scene into sys.modules before AppLauncher cycle clears/restores the module graph. This left isaaclab.scene in sys.modules but unbound from the isaaclab package, breaking monkeypatch.setattr(\"isaaclab.scene...\") calls in unrelated tests.

  2. Solution: _lazy_cfg() returns a zero-arg factory that does the import lazily inside gym.make(), after AppLauncher has properly initialized the module graph.

  3. Scope: Applied to 4 cartpole __init__.py files (direct, direct/showcase/cartpole, direct/showcase/cartpole_camera, manager_based/classic/cartpole). Each defines its own _lazy_cfg() — acceptable since these are package-private utilities.

  4. Backward compat: Deprecated task IDs continue to return their historical cfg classes — only the import timing changes, not the returned cfg.

Clean fix for a subtle test isolation bug caused by import ordering vs AppLauncher lifecycle. No behavioral change at runtime. ✅


Update (3a46209): Major architecture simplification — the deprecated_task_alias() factory pattern is replaced with a declarative deprecated_alias_for convention:

  1. task_deprecation.py module removed — The deprecated_task_alias() function and its 101 lines of wrapping logic are deleted.

  2. New deprecated_alias_for kwarg — Deprecated task IDs now use a simple string kwarg in gym.register():

    gym.register(
        id="Isaac-Cartpole-RGB-Camera-Direct-v0",
        kwargs={
            "env_cfg_entry_point": f"{__name__}.cartpole_camera_env_cfg:CartpoleRGBCameraEnvCfg",
            "deprecated_alias_for": "--task=Isaac-Cartpole-Camera-Direct-v0 presets=rgb",
        },
    )
  3. Centralized warning emissionparse_cfg.load_cfg_from_registry() now checks for deprecated_alias_for and emits the DeprecationWarning in one place, rather than at each factory call site.

  4. _lazy_cfg() helpers removed — Since env_cfg_entry_point is now a string (not a callable), the lazy import mechanism is no longer needed. The standard load_cfg_from_registry() import path handles cfg loading correctly.

  5. PresetCfg restructuringCartpoleCameraPresetsEnvCfg is now itself a PresetCfg with complete env cfg instances as variants (e.g., default = BaseCartpoleCameraEnvCfg()), rather than a cfg containing nested PresetCfg selector fields.

  6. Test file removedtest_cartpole_preset_deprecations.py (255 lines) is deleted. The previous tests verified exact warning bodies and cfg class preservation, which were tightly coupled to the callable-based factory pattern. The new declarative approach is simpler and the warning emission is trivially correct by inspection.

Benefits:

  • ✅ Simpler, more declarative registration — just a string kwarg vs. callable wrapper
  • ✅ Easier debugging — string entry points are introspectable; callables are opaque
  • ✅ No import timing issues — cfg classes loaded through standard loader path
  • ✅ DRY — warning logic in one place (parse_cfg.py) instead of per-task factories
  • ✅ Cleaner __init__.py files — no _lazy_cfg() boilerplate

This is the right simplification. The callable-based approach was necessary scaffolding during development but the final design should be declarative. Approve. ✅


Update (7ba4c36): Kwarg shape improved for extensibility — deprecated_alias_for (string) → deprecated={"alias": ...} (dict). The dict shape allows future fields (reason, removed_in, etc.) without API changes. parse_cfg.load_cfg_from_registry() updated to read kwargs["deprecated"]["alias"]. Changelog and docstrings updated accordingly. Clean structural improvement, no issues. ✅

@hujc7 hujc7 force-pushed the jichuanh/cartpole-presets branch from d3f7594 to 9a558ed Compare May 15, 2026 21:50
Collapse 35 per-variant Cartpole gym task IDs into 4 consolidated tasks
that select the variant via the typed preset CLI from isaac-sim#5587:

- Isaac-Cartpole-Camera-Direct-v0 (subsumes 7 Direct-backend perception IDs)
- Isaac-Cartpole-Camera-v0 (subsumes 4 manager-based perception IDs)
- Isaac-Cartpole-Showcase-Direct-v0 (subsumes 15 proprioceptive showcase IDs)
- Isaac-Cartpole-Camera-Showcase-Direct-v0 (subsumes 9 camera showcase IDs)

Variant selection moves from the gym ID to ``presets=<name>`` (plus
``physics=`` / ``renderer=`` on the Direct-backend camera task, and
``--agent=`` where the per-shape skrl / rl_games yaml differs from the
default). Each retired ID stays registered for one release with an
env_cfg_entry_point that emits a DeprecationWarning naming the new task
and the equivalent CLI tokens, then returns the corresponding cfg.

The shim machinery is factored into a new helper:

  isaaclab_tasks.utils.deprecated_task_alias(
      old_task_id, new_command, consolidated_cfg_path, cfg_factory=None
  )

which wraps the warning emission plus lazy cfg resolution behind the
``"module:Name"`` entry-point string convention gym uses elsewhere.

The consolidated PresetCfg classes live alongside their per-variant
siblings in the existing *_env_cfg.py files; no new wrapper modules.
PresetCfg's docstring now documents the underscore-prefix convention for
class-local helpers (the resolver already skips them).

Full per-ID migration table is in the PR description.
@hujc7 hujc7 force-pushed the jichuanh/cartpole-presets branch from e1eb51a to 84fdf54 Compare May 16, 2026 07:37
hujc7 added 4 commits May 16, 2026 07:54
…presets

# Conflicts:
#	source/isaaclab_tasks/isaaclab_tasks/direct/cartpole/__init__.py
#	source/isaaclab_tasks/isaaclab_tasks/direct/cartpole/cartpole_camera_presets_env_cfg.py
…ing IDs

The pre-existing develop consolidated ``MultiDataTypeCartpoleTiledCameraCfg``
uses a uniform identity-rotation camera for all data-type variants. The
historical per-variant cfgs (``CartpoleAlbedoCameraEnvCfg`` and the three
``CartpoleSimpleShading*CameraEnvCfg``) had a 180-degree rotated camera.
The merge from develop briefly reintroduced that rotation locally; the
rendering tests captured against develop's identity-rotation baseline
flagged the divergence on the consolidated path.

Drop the local rotation override so the consolidated cfg matches develop's
captured baseline. The deprecation shim returns the consolidated cfg
verbatim, so users of the retired task IDs now see the consolidated
camera orientation starting from this PR -- migration to the consolidated
task ID with ``presets=<name>`` carries that uniform pose anyway.

Test expectations updated accordingly.
Revert local extraction of `_CAMERA_CFG_PATH` / `_SHOWCASE_CFG_PATH`
constants and the small `_CAMERA_KWARGS` dict in the four cartpole
`__init__.py` files. Each `gym.register` block is again fully
self-contained (cfg-path string + skrl/rl_games entry points spelled
out in place), matching the per-block style on develop so the PR diff
shows only what is genuinely new (the alias call itself).

Pass `old_task_id=`, `new_command=`, and `consolidated_cfg_path=` as
keyword arguments at every call site -- a 3-arg positional invocation
is hard to read at the gym registration call sites.

No behavior change: the 73 cartpole deprecation tests still pass.
Change deprecated_task_alias(new_command=...) from a free-form string
to a list[str], where each list entry is one whitespace-separated CLI
token. The factory joins the list with single spaces when forming the
DeprecationWarning body, so the user-facing warning text is unchanged
in shape (still quoted, one command).

The list form makes per-token diffs easier to scan at the gym.register
call sites: each --task / --agent / presets= entry is on its own
line instead of being lost inside a multi-line implicit string
concat. Call-site convention: --task=... first, --agent=... next
when present, presets=NAME selector at the end -- the warning body
reads in that order too.

Test fixtures reordered to match the new --task / --agent / presets=
sequence in the warning body. All 73 cartpole deprecation tests pass.
@hujc7 hujc7 marked this pull request as ready for review May 17, 2026 04:52
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 17, 2026

Greptile Summary

This PR collapses 35 per-variant Cartpole gym task IDs into 4 consolidated tasks (Isaac-Cartpole-Camera-Direct-v0, Isaac-Cartpole-Camera-v0, Isaac-Cartpole-Showcase-Direct-v0, Isaac-Cartpole-Camera-Showcase-Direct-v0) using a new deprecated_task_alias helper and the typed preset CLI from #5587. Each retired ID stays registered as a shim that emits a DeprecationWarning and returns the same cfg as before.

  • Adds isaaclab_tasks.utils.deprecated_task_alias, a lazy factory that emits a DeprecationWarning and resolves the historical cfg variant from the consolidated PresetCfg at gym.make() time.
  • Direct-backend camera shims use a bespoke _resolve_camera_variant helper to atomically pin both the root cfg variant and the nested tiled_camera preset; all other shims use flat resolution through deprecated_task_alias.
  • 73 parametrized unit tests lock the warning text and cfg class identity for all 35 retired IDs and the 4 new consolidated tasks.

Confidence Score: 3/5

Safe for single-call usage; the direct-camera shims break on a second gym.make call with the same deprecated task ID in the same process.

The _resolve_camera_variant helper writes directly into a shared class-level attribute of CartpoleCameraPresetsEnvCfg. After the first call the object's tiled_camera field is already a resolved CartpoleTiledCameraCfg, so any subsequent factory invocation raises AttributeError. The test suite avoids triggering this by giving each parametrized case a unique task ID called exactly once.

source/isaaclab_tasks/isaaclab_tasks/direct/cartpole/init.py and source/isaaclab_tasks/test/test_cartpole_preset_deprecations.py

Important Files Changed

Filename Overview
source/isaaclab_tasks/isaaclab_tasks/direct/cartpole/init.py Registers consolidated Isaac-Cartpole-Camera-Direct-v0 and seven deprecated shims; _resolve_camera_variant mutates a shared class-level cfg attribute on first call, causing AttributeError on any subsequent factory invocation for the same task ID
source/isaaclab_tasks/isaaclab_tasks/utils/task_deprecation.py New helper that wraps retired gym task IDs with a DeprecationWarning and lazy cfg resolution; contains a duplicate of _user_stacklevel() already present in hydra.py
source/isaaclab_tasks/isaaclab_tasks/direct/cartpole/cartpole_camera_presets_env_cfg.py Adds CartpoleCameraPresetsEnvCfg and MultiDataTypeCartpoleTiledCameraCfg PresetCfg subclasses covering rgb/depth/albedo/simple_shading variants; looks correct
source/isaaclab_tasks/isaaclab_tasks/direct/cartpole_showcase/cartpole/init.py Registers consolidated Isaac-Cartpole-Showcase-Direct-v0 and 15 deprecated shims using deprecated_task_alias with flat (non-nested) resolution; pattern is correct
source/isaaclab_tasks/isaaclab_tasks/manager_based/classic/cartpole/init.py Registers Isaac-Cartpole-Camera-v0 and four deprecated manager-based perception shims with flat resolution; looks correct
source/isaaclab_tasks/test/test_cartpole_preset_deprecations.py 73 parametrized tests lock deprecation warning text and cfg class identity; good coverage of single-call behaviour but no test exercises the factory a second time, so the repeated-call mutation bug in _resolve_camera_variant is not caught

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["gym.make(deprecated_id)"] --> B["env_cfg_entry_point factory\n(returned by deprecated_task_alias)"]
    B --> C["warnings.warn DeprecationWarning\n'Task X deprecated, use Y'"]
    C --> D{cfg_factory\nprovided?}
    D -- Yes --> E["_resolve_camera_variant(preset_name)()\n[direct camera shims only]"]
    D -- No --> F["Parse presets= token\nfrom new_command"]
    F --> G["importlib.import_module\n+ getattr(cls(), preset)"]
    E --> H["CartpoleCameraPresetsEnvCfg()\n→ getattr(cfg, preset_name)\n→ mutate result.tiled_camera ⚠️"]
    G --> I["Concrete env cfg returned"]
    H --> I
    I --> J["gym.make continues"]

    K["gym.make(consolidated_id)"] --> L["env_cfg_entry_point string\nresolved by gym directly"]
    L --> M["PresetCfg subclass instantiated\nby resolve_presets via hydra"]
    M --> J
Loading

Reviews (1): Last reviewed commit: "Take new_command as a list of CLI tokens" | Re-trigger Greptile

Comment on lines +60 to +68
def call():
from .cartpole_camera_presets_env_cfg import CartpoleCameraPresetsEnvCfg

cfg = CartpoleCameraPresetsEnvCfg()
result = getattr(cfg, preset_name)
result.tiled_camera = getattr(result.tiled_camera, preset_name)
return result

return call
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Shared class attribute mutated in-place on first factory call

getattr(cfg, preset_name) retrieves the class-level attribute on CartpoleCameraPresetsEnvCfg (e.g. CartpoleCameraPresetsEnvCfg.depth), not an instance copy. The next line result.tiled_camera = getattr(result.tiled_camera, preset_name) writes an instance attribute onto that shared object, permanently changing it. A second call to the same factory — e.g. a second gym.make("Isaac-Cartpole-Depth-Camera-Direct-v0") in the same process — will find tiled_camera already resolved to a concrete CartpoleTiledCameraCfg, and getattr(result.tiled_camera, preset_name) will raise AttributeError because CartpoleTiledCameraCfg does not have a depth/albedo/etc. attribute. The fix is to deep-copy result before mutating it (import copy; result = copy.deepcopy(result)).

Comment on lines +30 to +46
def _user_stacklevel() -> int:
"""Compute a ``warnings.warn`` stacklevel that lands on the first frame
outside this module, so the deprecation warning cites user code rather
than the gym/parse_cfg loader that invoked the entry-point factory.

Walks a bounded number of frames; falls back to ``stacklevel=2`` if no
non-local frame is found within the bound.
"""
max_walk = 16
level = 1
frame = sys._getframe(1)
while frame is not None and frame.f_globals.get("__file__") == __file__:
level += 1
frame = frame.f_back
if level > max_walk:
return 2
return level
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 _user_stacklevel duplicated from hydra.py

hydra.py already defines an identical _user_stacklevel() at lines 45–63. Both copies share the same logic and walk the same bound (16 frames), so any future fix to the frame-walk heuristic needs to be applied in both files. The function should be extracted into a shared private utility (e.g. _warnings.py or an underscore-prefixed helper in a common module imported by both callers).

hujc7 added 7 commits May 17, 2026 05:17
Replace the custom flat-getattr + nested-setattr default resolution
inside deprecated_task_alias with a single call to
isaaclab_tasks.utils.hydra.resolve_presets(cls(), selected). The
resolver already walks every nested PresetCfg in the cfg tree and
picks the variant matching one of the selected names (falling back
to each preset's default field), which is exactly what the canonical
task's Hydra path does. The shim now returns bit-for-bit what the
canonical task plus presets=<name> would have produced.

Side effects:

* Drop the cfg_factory parameter from deprecated_task_alias -- the
  generic resolver handles the 2-axis (nested) case that previously
  required a custom callable.
* Delete the local _resolve_camera_variant helper from
  direct/cartpole/__init__.py and the cfg_factory= argument from
  its 5 deprecated-shim call sites.
* selected is now the union of every presets=NAME[,...] token's
  names, so a future caller passing presets=a,b correctly resolves
  both rather than truncating to the first.

No measurable startup-time delta: instantiation of the consolidated
PresetCfg (~22 ms, dominated by configclass deepcopy of a ~1400-node
tree) is the shared cost; the tree walk on top is sub-millisecond.
73 cartpole deprecation tests still pass.
…a cfgs

This is a two-part change driven by CI failures on the prior shim design:

1. ``test_preset_kit_decision`` (5 cases) failed because the shim's call to
   ``resolve_presets(cls(), selected)`` resolved every PresetCfg in the
   tree -- including ``sim.physics`` and ``tiled_camera.renderer_cfg`` --
   to their defaults when the shim's preset names did not match. Hydra's
   downstream validation then reported the user's ``physics=`` /
   ``renderer=`` CLI selectors as "Unknown preset(s)" because no
   PresetCfg remained to resolve them against.

2. ``test_rendering_registered_tasks`` (4 cases: Albedo + 3
   SimpleShading) failed because the consolidated cfg's matching variant
   does not carry the 180-degree camera rotation that the historical
   per-variant subclasses use (a long-standing latent divergence on
   develop between Style A subclasses and Style B consolidated cfg --
   the consolidation PR never reconciled it). Routing retired IDs
   through Style B silently dropped the rotation, so rendering
   baselines captured against Style A no longer matched.

Both regressions come from the same root cause: the shim was
re-resolving the consolidated cfg instead of returning the retired ID's
historical cfg verbatim.

The fix:

* ``deprecated_task_alias`` now takes a required ``cfg_factory``
  callable and drops ``consolidated_cfg_path`` + the in-shim
  ``resolve_presets`` call. Each retired task's shim returns whatever
  the factory builds.

* Every retired task call site passes ``cfg_factory=lambda: OldClass()``
  pointing at the historical per-variant subclass, so the retired ID
  stays bit-for-bit identical to develop. The deprecation warning is
  layered on top; no behavior change.

* The 4 manager-camera per-variant subclasses (deleted in an earlier
  attempt that consolidated them away) are restored alongside the new
  consolidated ``CartpoleCameraPresetsEnvCfg``. Both styles co-exist for
  one release, mirroring how the direct-camera files already work on
  develop. The consolidated cfg powers the canonical
  ``Isaac-Cartpole-Camera-v0`` task; the per-variant subclasses back the
  retired task IDs.

* The direct-camera ``CartpoleCameraPresetsEnvCfg`` keeps its earlier
  restructure (flat env cfg + nested PresetCfg fields for
  ``observation_space`` and ``tiled_camera.data_types``). The canonical
  ``Isaac-Cartpole-Camera-Direct-v0`` uses it; field-by-field equivalent
  to develop's consolidated cfg.

* ``hydra.py`` is unchanged from develop -- the earlier ``strict=``
  parameter is removed since the shim no longer calls
  ``resolve_presets``.

Each OLD per-variant subclass gets a docstring note pointing at the
canonical task + the migration command, marking it for removal alongside
the retired gym task ID after one release.

Tests:
* ``test_cartpole_preset_deprecations``: 73 pass (rotation expectations
  updated to reflect the historical 180-degree flip on Albedo /
  SimpleShading subclasses).
* ``test_preset_kit_decision``: 5 pass.
* ``test_manager_based_rl_env_obs_spaces``: imports
  ``CartpoleRGBCameraEnvCfg`` / ``CartpoleDepthCameraEnvCfg`` -- both
  remain present so the test is unaffected.
The consolidated ``CartpoleCameraPresetsEnvCfg`` was restructured from a
top-level ``PresetCfg`` (Style B on develop) into a flat
``DirectRLEnvCfg`` with nested ``PresetCfg`` fields. The visualizer
integration test extracted the rgb variant via
``getattr(env_cfg_root, "default", None)`` -- that path no longer exists
on the new shape and raises ``RuntimeError`` before any actual rendering.

Replace the ``default`` lookup with ``resolve_presets(env_cfg)`` so the
nested ``PresetCfg`` fields (observation_space, tiled_camera.data_types,
sim.physics, renderer_cfg) are resolved to their defaults in place,
matching how the canonical gym task ID resolves them at ``gym.make()``
time. The downstream test logic (overriding ``scene.num_envs``,
``viewer``, ``tiled_camera.width/height``, ``observation_space[2]``,
``sim.physics``, ``sim.visualizer_cfgs``) is unchanged.
…-state leak

Importing the historical per-variant cfg classes at the top of each
cartpole task ``__init__.py`` (so ``cfg_factory=lambda: SomeClass()``
could close over them) pulled ``isaaclab.envs``, ``isaaclab.scene``, and
their downstream modules into ``sys.modules`` *during*
``isaaclab_tasks.direct.cartpole``'s package import. That happens before
``AppLauncher._create_app`` runs, and AppLauncher's mod-cache cycle
``del sys.modules[key]`` + ``sys.modules[key] = value`` only restores
the ``sys.modules`` entries -- it never re-binds the submodules as
attributes on the parent ``isaaclab`` package. After the cycle,
``isaaclab.scene`` lives in ``sys.modules`` but ``isaaclab.scene``
``getattr`` on the ``isaaclab`` module raises ``AttributeError``, which
breaks any unrelated test that walks a dotted string path like
``monkeypatch.setattr("isaaclab.scene.interactive_scene.cloner.X", Y)``.

Concretely on this PR, both ``test_clone_environments_executes_*`` cases
in ``source/isaaclab/test/scene/test_interactive_scene.py`` started
failing once the eager imports landed. The same tests pass on develop
because there the cfg classes are only referenced via lazy
``"module:Class"`` strings in the gym entry points, so nothing imports
them before ``AppLauncher`` runs.

Fix: introduce a tiny ``_lazy_cfg(class_name, module=...)`` helper in
each of the four cartpole ``__init__.py`` files. The helper returns a
zero-arg callable that imports the cfg class via ``importlib`` only
when invoked. ``deprecated_task_alias`` still calls ``cfg_factory()``
exactly once per ``gym.make()``, so the deferred import path runs at
the same moment the eager-import path would have triggered (the
shim's factory body), just without the side effect of pre-loading the
cfg module before ``AppLauncher``.

Local verification:
* ``test_clone_environments_executes_env_root_plan_with_positions``: PASS
* ``test_clone_environments_executes_asset_level_plan_without_usd_positions``: PASS
* 73 cartpole-deprecation tests + 5 preset-kit-decision tests: all pass.
Limit this PR's scope to deprecating the per-variant cartpole task IDs in
favor of the four consolidated tasks; revert the cfg-class simplification
work that was an additional optimization.

- Revert direct/cartpole/cartpole_camera_presets_env_cfg.py to develop.
- Restructure manager-based and showcase consolidated cfgs as minimal
  PresetCfg wrappers that expose existing per-variant subclasses as
  preset variants. Drop the flat-cfg / nested-PresetCfg-field rewrite.
- Replace the cfg_factory + lazy_cfg machinery with a data-driven
  deprecated_alias_for kwarg on gym.register. parse_cfg.load_cfg_from_registry
  checks the kwarg and emits the DeprecationWarning when the retired ID's
  env_cfg_entry_point is loaded. env_cfg_entry_point stays a plain
  "module:Class" string, matching develop's pattern.
- Delete the now-unused task_deprecation.py utility and the
  test_cartpole_preset_deprecations.py test.
Replace flat ``deprecated_alias_for`` string kwarg on retired-task
``gym.register`` calls with a structured ``deprecated`` dict whose
``alias`` field holds the migration command. Keeps the kwarg shape open
for future fields (``reason``, ``removed_in``, ...) without renaming the
top-level key. ``parse_cfg.load_cfg_from_registry`` reads
``kwargs["deprecated"]["alias"]``; warning text and behavior unchanged.
@hujc7 hujc7 changed the title [Environments] Consolidate Cartpole task IDs via preset CLI [Cartpole] Consolidate Cartpole task IDs via preset CLI May 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant