[Cartpole] Remove deprecated task IDs#5851
Closed
hujc7 wants to merge 14 commits into
Closed
Conversation
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.
…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.
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.
DeprecationWarning + stacklevel=2 silently suppressed the warning in any call path where the immediate caller of load_cfg_from_registry is a library module (parse_env_cfg, hydra.register_task, isaaclab_contrib/isaaclab_mimic wrappers). Python's default DeprecationWarning filter only displays warnings attributed to __main__. - Generalize hydra._user_stacklevel from file-scoped to package-scoped (walks past anything under isaaclab_tasks.utils.*) so a single helper serves callers in any utils submodule. - Import _user_stacklevel from hydra into parse_cfg.load_cfg_from_registry and use it for the deprecation-alias warning. - Switch the warning category from DeprecationWarning to FutureWarning so the warning is shown by Python's default filter regardless of attribution (the existing convention for end-user-facing deprecations in hydra.py preset aliases and manager_based_env / direct_rl_env cfg-field warnings). Verified under default Python filter: paths 1 (direct call), 2 (parse_env_cfg from __main__), and 3 (third-party wrapper outside the utils package) all display the warning.
Followup to isaac-sim#5605 which marked 35 per-variant Cartpole task IDs as deprecated aliases for the four consolidated tasks. With 3.0 acting as the breaking-changes window, drop the deprecation cycle entirely. - Delete 35 retired gym.register entries across the four cartpole/cartpole_showcase __init__ files. - Drop the ``deprecated`` gym.register kwarg machinery in parse_cfg.load_cfg_from_registry and the package-scoped _user_stacklevel widening in hydra.py. - Repoint remaining references in docs, ray hyperparameter scripts, rendering/visualizer tests, the multi-GPU CI workflow, and the benchmark configs to the consolidated tasks. Migrate to the consolidated tasks with presets=<name>: Isaac-Cartpole-Camera-Direct-v0, Isaac-Cartpole-Camera-v0, Isaac-Cartpole-Showcase-Direct-v0, and Isaac-Cartpole-Camera-Showcase-Direct-v0.
github-actions
Bot
added
documentation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR cleanly removes 35 deprecated per-variant Cartpole task IDs as a follow-up to #5605, consolidating them into four canonical tasks:
Isaac-Cartpole-Camera-Direct-v0(replaces 7 Direct-backend camera IDs)Isaac-Cartpole-Camera-v0(replaces 4 manager-based camera IDs)Isaac-Cartpole-Showcase-Direct-v0(replaces 15 proprioceptive showcase IDs)Isaac-Cartpole-Camera-Showcase-Direct-v0(replaces 9 camera showcase IDs)
The change is well-scoped for the 3.0 breaking-changes window, removing the deprecation cycle that was added in #5605.
Findings
🔵 Suggestion: Changelog fragment numbering
The optimize_stage_creation.rst doc still shows 20. for the next item after 5. when listing tasks:
4. Isaac-Cartpole-Showcase-Direct-v0
5. Isaac-Cartpole-v0
20. Isaac-Factory-GearMesh-Direct-v0
Consider renumbering for consistency.
🔵 Suggestion: Benchmark config reduction
The configs.yaml benchmarking file removed several task variants but only kept 2 consolidated entries (Isaac-Cartpole-Camera-Direct-v0 and Isaac-Cartpole-Camera-v0). This is likely intentional for simplification, but worth confirming that preset variants can still be benchmarked if needed via CLI overrides.
Breaking Change Documentation
✅ Excellent - The PR includes:
- A clear changelog fragment (
jichuanh-cartpole-remove-deprecated-tasks.minor.rst) marked as Breaking - A comprehensive migration table in the PR description mapping every removed ID to its consolidated replacement with
presets=<name>syntax - Clear explanation that 3.0 is the breaking-changes window
Completeness of Removal
✅ All references properly cleaned up across:
- 4
__init__.pytask registration files (35 gym.register entries removed) - 6 documentation files updated
- 2 Ray hyperparameter tuning scripts updated
- 1 CI workflow file updated
- 6 test files updated
- 1 benchmark config updated
- Deprecation machinery in
parse_cfg.load_cfg_from_registryandhydra.pyreverted
Test Coverage
✅ Appropriate for a removal PR:
test_rendering_registered_tasks.pyupdated to test the consolidated task withpresetsparametertest_tiled_camera_env.pyreferences updatedtest_train_scripts_deterministic.pyupdatedtest_preset_kit_decision.pyupdatedtest_visualizer_cartpole_integration.pyupdated
CI Status
✅ labeler check passed. Note: This PR is stacked on #5605, so full CI will be more meaningful after the base PR merges.
Verdict
No significant issues found. This is a clean breaking-change removal that:
- Properly removes all 35 deprecated task IDs
- Updates all references in docs, tests, scripts, and CI
- Provides clear migration documentation
- Is appropriately scoped for the 3.0 release window
The minor doc numbering issue is cosmetic. Ready to merge after #5605 lands and CI completes.
Contributor
|
@fatimaanes for viz |
Collaborator
Author
|
done in 8b01020 |
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
2 participants
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.
Summary
per-variant Cartpole task IDs, since 3.0 is the breaking-changes
window and a deprecation cycle is unnecessary.
Dependency
This PR is stacked on top of #5605. Until #5605 merges, the diff against
developshows that PR's additions interleaved with this PR's deletions.Once #5605 lands, the diff collapses to just the deletions + the
canonical-task repointing.
1. Removed
35 retired gym.register entries across the four cartpole init
files:
direct/cartpole/__init__.py— 7 per-data-type camera IDsmanager_based/classic/cartpole/__init__.py— 4 per-pipeline IDsdirect/cartpole_showcase/cartpole/__init__.py— 15 per-shape IDsdirect/cartpole_showcase/cartpole_camera/__init__.py— 9 per-shapecamera IDs
The
deprecatedgym.registerkwarg machinery:parse_cfg.load_cfg_from_registryreverted to develop (noFutureWarning emit).
hydra.py_user_stacklevelnarrowed back to file scope (thepackage-scoped widening was only needed to support warning
attribution from
parse_cfg).Stale references to retired task IDs in:
docs/source/features/reproducibility.rstdocs/source/how-to/optimize_stage_creation.rstdocs/source/overview/core-concepts/sensors/camera.rstdocs/source/overview/environments.rstdocs/source/overview/reinforcement-learning/performance_benchmarks.rstscripts/reinforcement_learning/ray/hyperparameter_tuning/vision_cartpole_cfg.pyscripts/reinforcement_learning/ray/tuner.pysource/isaaclab/test/sensors/test_tiled_camera_env.pysource/isaaclab_tasks/test/test_preset_kit_decision.pysource/isaaclab_tasks/test/test_rendering_registered_tasks.pysource/isaaclab_tasks/test/test_train_scripts_deterministic.pysource/isaaclab_visualizers/test/test_visualizer_cartpole_integration.py.github/workflows/test-multi-gpu.yamlsource/isaaclab_tasks/test/benchmarking/configs.yaml2. Migration
Use the consolidated tasks with
presets=<name>:Isaac-Cartpole-{RGB,Depth,Albedo,SimpleShading-*}-Camera-Direct-v0,Isaac-Cartpole-Camera-Presets-Direct-v0--task=Isaac-Cartpole-Camera-Direct-v0 [presets=<rgb|depth|albedo|simple_shading_*>]Isaac-Cartpole-{RGB,Depth}-v0--task=Isaac-Cartpole-Camera-v0 [presets=<rgb|depth>]Isaac-Cartpole-RGB-{ResNet18,TheiaTiny}-v0--task=Isaac-Cartpole-Camera-v0 --agent=rl_games_feature_cfg_entry_point presets=<resnet18|theia_tiny>Isaac-Cartpole-Showcase-<Obs>-<Action>-Direct-v0--task=Isaac-Cartpole-Showcase-Direct-v0 [--agent=skrl_<obs>_<action>_cfg_entry_point] presets=<obs>_<action>Isaac-Cartpole-Camera-Showcase-<Obs>-<Action>-Direct-v0--task=Isaac-Cartpole-Camera-Showcase-Direct-v0 [--agent=skrl_<obs>_<action>_cfg_entry_point] presets=<obs>_<action>3. Test plan
./isaaclab.sh -f).retired task IDs all raise
NameNotFound.