[WARP] Cleanup Part 1: Add frontend bridge for stable manager-based tasks#5504
[WARP] Cleanup Part 1: Add frontend bridge for stable manager-based tasks#5504hujc7 wants to merge 48 commits into
Conversation
PR isaac-sim#5297 (Decouple Renderer from Camera) replaced `sim.get_setting('/isaaclab/visualizer')` (returned a comma-separated string) with `sim.has_active_visualizers()` plus per-type queries; the warp env path was missed, so it crashes with "AttributeError: 'dict' object has no attribute 'split'" during env construction. Mirror the stable env's pattern: - `has_active_visualizers()` for the gating predicate - `has_kit()` so kitless Newton-only runs (`--viz rerun`) skip ViewportCameraController
Warp's ObservationTermCfg / RewardTermCfg / TerminationTermCfg had no behavioural difference from their stable counterparts — same fields, same base class. The override only carried Warp-first docstrings while keeping the classes as siblings of stable's, which broke `isinstance(stable_term, warp.TermCfg)` checks inside the experimental managers when a stable cfg is fed through the warp runtime. Re-export stable's term cfg classes directly. Term funcs still follow the warp-first `func(env, out, **params)` signature at runtime; the type annotation just lives on stable's class instead of warp's.
Lets a stable manager-based RL env cfg run on the experimental warp runtime without a separate warp task registration. Replaces the duplicate per-robot warp env cfgs by adapting the stable cfg in place. Components: - isaaclab_experimental/envs/warp_frontend.py: 'WarpFrontend' walks the stable cfg, swaps each `term.func` to its same-named warp twin (only accepting candidates whose `__module__` lives under the warp packages — the warp mdp module re-exports stable terms via `from … import *`, so a naive `getattr` would silently keep the stable function), swaps each action `class_type`, picks the `newton` field of any PresetCfg, drops sensors with no warp counterpart (`height_scanner`), and in-place class-promotes `SceneEntityCfg` instances so warp kernels see the `joint_mask` / *\_ids_wp` cached fields. A `WarpAdaptReport` records every term that had no warp twin and surfaces them via the logger; `strict=True` makes the same condition raise `LookupError` instead. - scripts/reinforcement_learning/rsl_rl/train.py: '--manager=warp' flag routes the constructed env through `WarpFrontend.build` instead of `gym.make`. The flag also auto-injects `presets=newton` into Hydra's argv so that PresetCfg wrappers resolve to the newton preset (Hydra's preset resolution runs *before* the adapter). Validated: cartpole and Anymal-D Flat both pass a 3-way comparison — - 'Isaac-Cartpole-v0' (stable manager): trains. - 'Isaac-Cartpole-Warp-v0' (existing direct path): reward 0.06 / ep 76. - 'Isaac-Cartpole-v0 --manager=warp': reward 0.06 / ep 76 (matches direct exactly). - 'Isaac-Velocity-Flat-Anymal-D-v0 presets=newton': -8.45 / 191. - 'Isaac-Velocity-Flat-Anymal-D-Warp-v0': -7.47 / 168. - 'Isaac-Velocity-Flat-Anymal-D-v0 --manager=warp': -7.69 / 174 (within run-to-run variance of the direct warp path). Both stable and warp frontends remain functional for every task; this PR adds a flag-based selector without removing the existing direct warp registrations.
Adds the isaaclab_experimental changelog fragment for the WarpFrontend adapter and the --manager flag, and applies the ruff-format pass that the pre-commit hook produced on warp_frontend.py.
![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 introduces a WarpFrontend adapter that allows stable manager-based RL task configs to run on the experimental warp runtime without requiring parallel -Warp-v0 task registrations. The implementation mutates configs in-place to swap stable MDP functions with warp twins, upgrades SceneEntityCfg instances, and drops unsupported sensors. The approach is architecturally sound for reducing task duplication, but there are several correctness issues that need attention before shipping.
Architecture Impact
The changes are relatively self-contained within isaaclab_experimental, with the main integration point being rsl_rl/train.py. The adapter operates at config mutation time before env construction, so it doesn't change the warp runtime itself. However:
- The
manager_term_cfg.pychange from custom classes to re-exports affects any code doingisinstancechecks against the warp term cfg types - The
--manager=warpflag is only added torsl_rl/train.py, not other trainers (skrl, rl_games) — documented as out of scope but creates API inconsistency - The in-place
__class__mutation pattern in_upgrade_scene_entity_cfgsis fragile if the warpSceneEntityCfgever diverges structurally from stable
Implementation Verdict
Minor fixes needed — The core design is solid, but there are edge cases and correctness issues that should be addressed.
Test Coverage
Insufficient. The PR adds no automated tests. The validation is entirely manual (described in PR description). Given the complexity of the adapter logic (module resolution, class swapping, term iteration), this needs at minimum:
- Unit tests for
WarpFrontend.adapt()with mock configs - Regression test for the visualizer fix in
manager_based_env_warp.py - Integration test that the
--manager=warpflag correctly invokes the adapter
CI Status
No CI checks available yet — cannot assess whether existing tests pass.
Findings
🔴 Critical: warp_frontend.py:278-287 — In-place __class__ mutation is unsafe without verifying inheritance
The code does obj.__class__ = _WarpSE after checking isinstance(obj, _StableSE), but if _WarpSE doesn't actually inherit from _StableSE (which the code assumes but doesn't verify), this will corrupt the object. The comment claims "warp inherits stable" but there's no runtime assertion. If someone refactors the warp SceneEntityCfg to not inherit from stable, this silently corrupts configs.
# Should verify at import time:
if not issubclass(_WarpSE, _StableSE):
raise TypeError("WarpFrontend requires warp SceneEntityCfg to inherit from stable")🔴 Critical: warp_frontend.py:95-97 — Iterating over dir(group) while mutating attributes
The apply_to method iterates over dir(group) and calls setattr(group, name, None) to drop terms. While this doesn't crash because the iteration is over a snapshot from dir(), it means subsequent code that relies on attribute presence (like the warp manager's term resolution) will see None values rather than missing attributes. The warp managers need to handle None term entries, or this should use delattr.
🟡 Warning: warp_frontend.py:316-329 — Module resolution swallows all exceptions
try:
import gymnasium as gym
entry = gym.spec(task_id).kwargs.get("env_cfg_entry_point")
except Exception:
entry = NoneThis catches all exceptions including KeyboardInterrupt and SystemExit. Should be except (ImportError, KeyError, AttributeError): or similar to avoid masking real errors.
🟡 Warning: train.py:100-101 — Preset injection doesn't check for conflicting explicit presets
if args_cli.manager == "warp" and not any(a.startswith("presets=") for a in remaining_args):
remaining_args.append("presets=newton")If user passes presets=physx, this silently does nothing (doesn't append newton), but the warp adapter may still try to run and fail confusingly later. Should warn when --manager=warp is used with a non-newton preset.
🟡 Warning: warp_frontend.py:215-217 — Physics preset extraction logic is redundant
The comment says "by the time we run, Hydra's resolve_presets has already collapsed any PresetCfg wrapper" — but then the code checks hasattr(physics, "newton") anyway. If the preset is already resolved, this condition will always be False. This is dead code that adds confusion.
🔵 Improvement: warp_frontend.py:170-177 — DEFAULT_TERM_GROUPS misses commands group
The default term groups don't include ("commands",), which is a manager group in the stable env. If a task has command terms with SceneEntityCfg params, they won't be upgraded. Either add it or document why it's excluded.
🔵 Improvement: manager_term_cfg.py — Re-export change may break type annotations
The change from explicit @configclass definitions to from ... import * means type checkers can no longer see what's exported from this module. Code doing from isaaclab_experimental.managers.manager_term_cfg import RewardTermCfg will work at runtime but may fail type checking. Consider adding explicit __all__ or type stub.
🔵 Improvement: warp_frontend.py:256-261 — build() ignores the adapt report
The build() method calls adapt() but doesn't return or expose the report. If the caller wants to inspect what was dropped/missing, they have to call adapt() separately first. Consider returning a tuple (env, report) or attaching the report to the env.
Greptile SummaryThis PR introduces a Confidence Score: 4/5Safe to merge with the two robustness fixes applied; the warp adapter path is gated behind an optional import so the default torch path is unaffected. The source/isaaclab_experimental/isaaclab_experimental/envs/frontend.py — Important Files Changed
Reviews (3): Last reviewed commit: "Rename CLI preset hint to newton_mjwarp" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def build(self, cfg: Any, task_id: str): | ||
| """Adapt ``cfg`` and return a :class:`ManagerBasedRLEnvWarp` instance. | ||
|
|
||
| Always logs the missing-twin report (if any). With ``strict=True``, | ||
| any missing twin raises before the env is constructed. | ||
| """ | ||
| # Lazy: this is the first warp-lib load. Caller must already be inside | ||
| # the SimulationApp context, i.e. inside ``launch_simulation``. | ||
| from isaaclab_experimental.envs import ManagerBasedRLEnvWarp | ||
|
|
||
| self.adapt(cfg, task_id) | ||
| return ManagerBasedRLEnvWarp(cfg=cfg) |
There was a problem hiding this comment.
WarpFrontend.build always constructs ManagerBasedRLEnvWarp with render_mode=None, but ManagerBasedRLEnvWarp.__init__ accepts render_mode and stores it. When --video is combined with --manager=warp, gym.wrappers.RecordVideo is applied to the returned env, but because render_mode was never set to "rgb_array", gymnasium will raise an error. The warp path silently drops the flag that the stable path (gym.make(..., render_mode="rgb_array" if args_cli.video else None)) correctly sets.
| def build(self, cfg: Any, task_id: str): | |
| """Adapt ``cfg`` and return a :class:`ManagerBasedRLEnvWarp` instance. | |
| Always logs the missing-twin report (if any). With ``strict=True``, | |
| any missing twin raises before the env is constructed. | |
| """ | |
| # Lazy: this is the first warp-lib load. Caller must already be inside | |
| # the SimulationApp context, i.e. inside ``launch_simulation``. | |
| from isaaclab_experimental.envs import ManagerBasedRLEnvWarp | |
| self.adapt(cfg, task_id) | |
| return ManagerBasedRLEnvWarp(cfg=cfg) | |
| def build(self, cfg: Any, task_id: str, render_mode: str | None = None): | |
| """Adapt ``cfg`` and return a :class:`ManagerBasedRLEnvWarp` instance. | |
| Always logs the missing-twin report (if any). With ``strict=True``, | |
| any missing twin raises before the env is constructed. | |
| """ | |
| # Lazy: this is the first warp-lib load. Caller must already be inside | |
| # the SimulationApp context, i.e. inside ``launch_simulation``. | |
| from isaaclab_experimental.envs import ManagerBasedRLEnvWarp | |
| self.adapt(cfg, task_id) | |
| return ManagerBasedRLEnvWarp(cfg=cfg, render_mode=render_mode) |
| # SimulationApp is already alive avoids racing pxr extension init. | ||
| from isaaclab_experimental.envs.warp_frontend import WarpFrontend | ||
|
|
||
| env = WarpFrontend().build(env_cfg, task_id=args_cli.task) |
There was a problem hiding this comment.
render_mode, so gym.wrappers.RecordVideo applied on line 222 will fail when --video --manager=warp is used. Pass the same render_mode the stable path uses.
| env = WarpFrontend().build(env_cfg, task_id=args_cli.task) | |
| env = WarpFrontend().build(env_cfg, task_id=args_cli.task, render_mode="rgb_array" if args_cli.video else None) |
| def build(self, cfg: Any, task_id: str): | ||
| """Adapt ``cfg`` and return a :class:`ManagerBasedRLEnvWarp` instance. | ||
|
|
||
| Always logs the missing-twin report (if any). With ``strict=True``, | ||
| any missing twin raises before the env is constructed. | ||
| """ | ||
| # Lazy: this is the first warp-lib load. Caller must already be inside | ||
| # the SimulationApp context, i.e. inside ``launch_simulation``. | ||
| from isaaclab_experimental.envs import ManagerBasedRLEnvWarp | ||
|
|
||
| self.adapt(cfg, task_id) | ||
| return ManagerBasedRLEnvWarp(cfg=cfg) |
There was a problem hiding this comment.
build() discards the WarpAdaptReport returned by adapt(), so callers have no programmatic access to what was dropped. In the default non-strict mode, silently-dropped terms cause the env to train with a smaller effective term set than expected. A caller who wants to enforce a "fail if anything was dropped" policy must call adapt() separately, which then re-mutates the already-mutated cfg when build() calls it again internally.
| modules: list[ModuleType] = [] | ||
| try: | ||
| import gymnasium as gym | ||
|
|
||
| entry = gym.spec(task_id).kwargs.get("env_cfg_entry_point") | ||
| except Exception: | ||
| entry = None |
There was a problem hiding this comment.
except Exception swallows any error from gymnasium.spec(task_id) — including gymnasium.error.NameNotFound if the task ID is misspelled and real programming errors. When this fires, entry is set to None and only the fallback mdp module is used, so no task-specific warp twins are discovered, causing every term to go missing without any indication of why.
The adapter is now a sequence of CompatRule objects (resolve preset, drop sensors, promote SceneEntityCfg, swap mdp funcs, swap action class). New incompatibilities are added by writing a small rule subclass instead of editing the dispatcher. The CLI flag is renamed --manager → --frontend because the dispatch also covers direct envs: a stable manager-based cfg is adapted onto ManagerBasedRLEnvWarp; a direct task is verified to point at a warp env class and dispatched via gym.make. A stable direct cfg + --frontend=warp raises IncompatibleEnvError with the offending entry_point and a hint at the *-Direct-Warp-v0 alternative. Other fixes: - Forward render_mode through build() so --video keeps working. - Attach the CompatReport on env.unwrapped.warp_compat_report so callers can inspect what was dropped or left unresolved. - Assert the warp SceneEntityCfg subclasses the stable one before doing the in-place __class__ promotion; the rule fails loudly if the hierarchy is ever broken. - Narrow the bare except in mdp-module discovery so real ImportErrors from broken cfgs propagate. - presets=newton is now only auto-injected for stable manager-based tasks; direct warp tasks (which don't carry presets) are left alone. - Warn when the user passes presets=<other> with --frontend=warp. - Add the commands group to the rule that promotes SceneEntityCfg.
The earlier check inspected gym.spec(task).entry_point, which for stable
manager-based tasks is "isaaclab.envs:ManagerBasedRLEnv" — the env class
path, not the cfg path. So the startswith("isaaclab_tasks.manager_based")
test always failed and presets=newton was never injected. Hydra then
resolved every PresetCfg in the cfg tree (physics, contact_forces, etc.)
to its default field, leaving the warp runtime with PhysX class_types it
can't load.
Switch to spec.kwargs["env_cfg_entry_point"], which actually points at the
task cfg module (e.g. "isaaclab_tasks.manager_based.locomotion.velocity.
config.anymal_d.flat_env_cfg:AnymalDFlatEnvCfg"), and the prefix check
selects the right tasks.
The single-file warp_frontend.py grew into a real subsystem worth splitting out, so move it into a frontend/ package with explicit abstractions: - frontend/base.py: Frontend ABC, CompatRule (check + transform via a unified run() method), TaskResolver (centralised gym.spec introspection -> TaskMeta), Workflow / Runtime / Severity enums, Issue / Change / Report record types, register_frontend / get_frontend registry. Helpers walk_attrs / resolve_warp_twin / iter_term_attrs are shared utilities used by rules. - frontend/torch.py: TorchFrontend, the default. Pass-through to gym.make with one rule (WarnIfTaskIsWarpRegistered) for the contradiction case. - frontend/warp.py: WarpFrontend with the full rule pipeline. Includes a new CheckPhysicsIsNewton blocking rule that surfaces the PhysX-with-warp incompatibility (asset class_type strings resolve to isaaclab_physx.* classes that depend on omni.physics.tensors.api, which the warp runtime does not initialise). CLI: rename --frontend stable -> torch since the axis is *runtime*, not *stability tier*. The frontend selector now reads cleanly: --frontend torch -> default gym.make path --frontend warp -> experimental warp runtime via WarpFrontend train.py becomes a thin dispatcher: get_frontend(name) gives a Frontend instance, frontend.preprocess_hydra_args(...) handles preset injection, frontend.build(cfg, task) returns the env. No more inline conditional imports; no more inline preset-injection logic. env.unwrapped.frontend_report is the inspection point - callers and tests can read what changed and what was missing without re-running adapt(). To add a new compatibility check, write a CompatRule subclass and append it to the relevant frontend's `rules` tuple. To add a new runtime, subclass Frontend and call register_frontend(name, cls).
- SwapMdpFunctions: skip terms whose ``func.__module__`` is already under the warp prefixes. Without this, running the bridge against a task already registered under ``isaaclab_tasks_experimental`` (e.g. ``Isaac-Cartpole-Warp-v0 --frontend=warp``) would silently drop terms whose warp twin happens not to live in the resolved fallback module. Also tighten ``_mdp_modules`` to require the trailing dot when matching ``isaaclab_tasks`` so we don't double-replace the prefix and end up importing ``isaaclab_tasks_experimental_experimental.*``. - ResolvePhysicsPreset: scope to MANAGER_BASED via ``applies_to``. Direct cfgs aren't expected to carry ``PresetCfg`` wrappers; running this rule on them was a no-op but the scoping makes the contract explicit. - CheckPhysicsIsNewton: positively accept ``isaaclab_newton.*`` modules, block on ``isaaclab_physx.*``, warn on anything else. The previous rule only rejected PhysX, so a custom or third-party physics cfg in an unrelated module would slip through silently. - PromoteSceneEntityCfg: catch ``TypeError`` from the in-place ``__class__`` reassignment and surface it as a blocking issue. ``issubclass`` does not guarantee Python permits the layout change (slots, layout flags); failing loud at this seam is better than a cryptic crash mid-pipeline. - WarpFrontend.preprocess_hydra_args: normalise leading dashes when inspecting ``presets=``, so ``--presets=foo`` is treated the same as ``presets=foo`` (Hydra accepts both forms). - Frontend.resolve: hard-block when ``gym.spec`` returned no spec. Previously ``meta.runtime`` was ``UNKNOWN`` and ``construct`` would fail later with a less specific error; now the block fires before any rule runs. - train.py: import the frontend lazily and tolerate ``ImportError`` when ``--frontend=torch`` (the default). The experimental package is optional, so a missing install used to break the default path; now it falls back to ``gym.make`` for torch and only fails for ``warp``. - frontend/__init__.py: trim ``__all__`` and the wildcard re-export so helpers (``walk_attrs``, ``iter_term_attrs``, ``resolve_warp_twin``, ``WARP_ROOT_PREFIXES``) are no longer advertised as the public framework surface. They remain importable from ``frontend.base`` for users writing their own rules.
- TaskResolver._classify_runtime: also accept class/callable entry points by inspecting __module__. gym.register accepts both ``"module:Class"`` strings and class objects; the old check only handled strings, so a warp-registered task using the class form classified as Runtime.UNKNOWN and the warn / verify rules silently disengaged. - SwapMdpFunctions._mdp_modules: narrow the exception match from ImportError to ModuleNotFoundError where ``exc.name`` matches the module being looked up. Previously a real ImportError raised inside an existing mdp module (broken import inside the package) would be silently swallowed and the rule would fall through to the fallback module, producing misleading "no warp twin" reports.
hujc7
changed the title
…ger-bridge # Conflicts: # scripts/reinforcement_learning/rsl_rl/train.py # source/isaaclab_experimental/isaaclab_experimental/managers/manager_term_cfg.py
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot — Re-review after Updates
Summary
This PR introduces a well-designed frontend framework (isaaclab_experimental.envs.frontend) that enables runtime backend selection for IsaacLab tasks via --frontend torch|warp. The architecture is clean: a Frontend base class with pluggable CompatRule pipelines, proper separation between task resolution (TaskResolver) and env construction, and clear error reporting via Report/Issue/Change records.
Since the initial review (May 5), the author has addressed all critical findings through commits 8934fe649a60 ("Tighten frontend framework against review findings") and 67050a863d5e ("Tighten frontend framework against second review pass"). The code quality has improved significantly.
Architecture Assessment
Strengths:
- Clean plugin architecture: new compatibility rules are easy to add without modifying core framework
- Proper error surfacing via structured
Issuerecords with severity levels - Good idempotency handling in
SwapMdpFunctions(skips already-warp terms) - Train script integration is backwards-compatible (falls back to
gym.makewhenisaaclab_experimentalisn't installed) - The
manager_term_cfg.pysimplification (re-exporting stable classes) correctly fixes theisinstancecheck issue
Design Decisions (reasonable but worth noting):
- In-place cfg mutation rather than copy — documented and appropriate for the use case
frontend_reportis set as a dynamic attribute onenv.unwrapped— works but isn't declared on the env class
Implementation Verdict
Ship it ✅ — The previous critical issues have been resolved. The remaining items are minor improvements that don't block merging.
Resolved Issues from Previous Review
✅ issubclass(Warp, Stable) check now present (line 200)
✅ TypeError from __class__ promotion is caught and surfaced as blocking issue (line 225)
✅ Non-newton presets now emit a warning (lines 508-509)
✅ commands group added to both PromoteSceneEntityCfg and SwapMdpFunctions
✅ Module import errors now properly distinguish "module doesn't exist" from "module has internal error" (lines 338-343)
Remaining Findings
🔵 Improvement: No automated tests for the frontend framework
The PR adds substantial logic (rule pipeline, task resolution, cfg mutation) but no unit tests. The manual validation in the PR description is thorough, but automated tests would prevent regressions. Consider adding in a follow-up:
- Unit tests for
TaskResolver.resolve()with mock specs - Unit tests for each
CompatRulewith mock configs - Integration test for
WarpFrontend.build()happy path
🔵 Improvement: base.py:365 — Consider declaring frontend_report attribute
The frontend_report attribute is set dynamically:
env.unwrapped.frontend_report = reportWhile this works, type checkers and IDE autocompletion won't know about it. Consider adding a type stub or documenting that users should expect this attribute.
🔵 Improvement: warp.py:165 — DropUnsupportedSensors could emit an Issue
Currently drops sensors silently (only emits a Change). A WARNING severity Issue might be more appropriate since dropping a sensor affects task behavior and users should be clearly informed.
CI Status
Checks are still pending. Recommend waiting for CI green before merge.
Test Coverage
The PR description includes thorough manual validation (Cartpole, Anymal-D Flat parity tests, negative cases). However, no automated tests are included for the new framework code. This is acceptable for an experimental package but should be addressed in a follow-up.
Re-reviewed at commit 429794db61ad after the refactoring commits addressed previous findings.
🤖 Update (e2cb744)
Major architectural simplification — The plugin-based CompatRule framework has been replaced with a much simpler functional approach:
Changes Summary
-
Framework consolidation: ~1000+ lines across 4 files → ~373 lines in a single
frontend.pymodule- Deleted:
frontend/__init__.py,frontend/base.py,frontend/torch.py,frontend/warp.py - Added:
frontend.py(flat module with direct functions)
- Deleted:
-
Behavioral changes:
presets=newtonauto-injection removed — user must explicitly pass it (documented in changelog)- Missing MDP twins are now a hard failure (was WARNING/drop in non-strict mode) — this is safer
SceneEntityCfg.from_stable()classmethod replaces the__class__reassignment hack
-
Train script simplified: Direct
build(frontend, env_cfg, task_id, render_mode=...)call
Previous Findings Status
✅ "No automated tests" — Now addressed! Comprehensive test_frontend.py added covering:
- Enum surface tests
SceneEntityCfg.from_stablefield copy_require_newton_physicshard-check_promote_scene_entity_cfgswalks term params_swap_mdpfunc/class_type swap + missing twin error paths- Direct workflow guard tests
✅ frontend_report attribute — No longer applies (removed with the Report/Issue/Change system)
✅ DropUnsupportedSensors silent drop — Rule removed entirely; the new approach requires all twins to exist
Assessment
The simplification is a good architectural decision:
- Easier to understand and maintain
- Harder to misuse (fail-fast on missing twins)
- Tests now included
- The plugin extensibility wasn't being used and added complexity
Ship it ✅ — No new issues found in this update.
🤖 Update (bd27af4)
Dynamic term discovery — This update replaces hardcoded term path lists with recursive type-based discovery:
Changes Summary
-
_walk_terms()refactoring: New recursive walker that discoversManagerTermBaseCfginstances by type viaisinstance(), not by hardcoded attribute names like("observations", "policy"). This makes the framework future-proof — new observation groups, nested cfgs, or custom term containers are picked up automatically. -
Deleted
manager_term_cfg.py: The experimental package no longer re-exports stable term cfgs; managers import directly fromisaaclab.managers.manager_term_cfg. Cleaner dependency graph. -
Type hint updates:
funccallables inObservationTermCfg,RewardTermCfg,TerminationTermCfgnow allow| Nonereturn types, supporting warp-style out-parameter functions. -
Test improvements: Tests now use real
@configclassfixtures instead ofSimpleNamespacemocks, and explicitly verify that arbitrarily-named observation groups (e.g.,perception) are discovered. -
Naming cleanup:
_require_direct_is_warp_task→_assert_direct_warp_registration(clearer intent).
Assessment
This is a solid incremental improvement:
- Dynamic discovery is more robust than hardcoded paths
- Better test fixtures catch real configclass edge cases
- Import simplification reduces surface area
Ship it ✅ — No new issues introduced.
🤖 Update (21e8bba)
Changelog housekeeping — Documentation-only commit:
- Compressed
isaaclab_experimentalchangelog fragment from 47 lines to 12 lines (follows terse-fragment convention) - Added
isaaclab/changelog.d/warp-manager-bridge.rstdocumenting theCallable[..., torch.Tensor | None]type relaxation - Added
isaaclab_tasks_experimental/changelog.d/warp-manager-bridge.skip(internal humanoid import redirect)
No code changes. Previous review findings unchanged.
Ship it ✅
🤖 Update (b932897)
Warp env API alignment — Syncs warp experimental envs with stable API changes:
Changes Summary
-
direct_rl_env_warp.py:- Enabled
ViewportCameraController(was commented out) — now initializes when Kit viewport exists - Added
has_kit()guard to skip ViewportCameraController in kitless Newton-only runs - Replaced manual
get_setting("/isaaclab/has_gui")calls withself.sim.has_gui/self.sim.has_offscreen_renderproperties
- Enabled
-
manager_based_rl_env_warp.py:- Same render-check modernization:
get_setting()→ cachedSimulationContextproperties
- Same render-check modernization:
These changes align with upstream stable PRs #4646 (cached render properties) and #5103 (visualizer API update).
Assessment
Clean sync with stable — No new issues. The inline comments from previous reviews (render_mode passthrough, exception handling) remain open but are in separate files not touched by this commit.
Ship it ✅
🤖 Update (d33a9bd)
Merge from develop + preset rename — Two branch-specific changes since b932897d:
-
0f344754— Renames CLI preset hint frompresets=newtontopresets=newton_mjwarpin error messages, comments, and the corresponding test assertion. Cosmetic only; aligns with an upstream rename of the preset identifier. -
d33a9bdf— Merge oforigin/developinto the feature branch (200+ upstream commits). No merge conflicts with the branch's own files; the diff is entirely upstream changes flowing in.
Previous Findings Status
All previous findings remain addressed. No regressions introduced.
Assessment
No new issues. The branch-specific delta is a trivial string rename that keeps documentation/error messages consistent with upstream preset naming. The develop merge is clean.
Ship it ✅
🤖 Update (d33a9bd, second pass)
✅ P1 fix confirmed: The ModuleNotFoundError check in _warp_mdp_modules (line 351) now uses warp_mod.startswith(exc.name) instead of the previous exc.name == target. This correctly handles the case where a parent package (e.g. isaaclab_tasks_experimental) is missing, not just the leaf module. The graceful fallback path now works as intended.
All inline comments from my previous review have been addressed.
🤖 Update (855588f)
Merge from develop (clean) — The new HEAD is a merge of origin/develop into the feature branch, incorporating ~50+ upstream commits (OvPhysX assets/sensors, Newton deformables, teleop replay MCAP migration, preset CLI improvements, visualizer fixes, packaging cleanup, etc.). No merge conflicts with the PR's own files. The branch-specific delta since d33a9bdf (last reviewed) is zero new feature code — the preset rename to newton_mjwarp (commit 0f344754) was already noted in the previous update.
Previous Findings Status
All previous findings remain addressed. No regressions introduced by the merge.
Assessment
No new issues. The branch is a clean forward-merge of upstream develop with no changes to the PR's own experimental frontend framework code.
Ship it ✅
Collapses the CompatRule / Frontend / Report / Issue / Change framework (509+72+513+95 LOC across base/torch/warp/__init__) into one module (frontend.py, ~370 LOC). Net diff: 1270 → 864 LOC. Behavior changes: - Drop the pre-Hydra presets=newton injection. The warp frontend now hard-checks cfg.sim.physics is NewtonCfg at build time and tells the user to pass presets=newton on the CLI when it isn't. - Drop DropUnsupportedSensors. The Newton RayCaster (isaac-sim#5510) makes the height_scanner case obsolete; any future incompatible sensor should fail loudly with the sensor name rather than silently set None. - Merge SwapMdpFunctions + SwapActionClassType into one pass that swaps term.func or term.class_type uniformly for every group including actions. - Missing warp twin is always a hard failure (no strict / non-strict toggle). Partial swaps would leave torch funcs in a cfg consumed by warp managers, which only accept the kernel-style signature. - Replace SceneEntityCfg.__class__ = WarpSceneEntityCfg with a proper WarpSceneEntityCfg.from_stable() classmethod that copies every selection field through __init__. Train.py is unchanged in shape — two call sites, now using the top-level build(frontend, cfg, task_id) function.
Frontend (envs/frontend.py) - Replace _TERM_PATHS hardcode (with its policy-only observation limitation) with _walk_terms: a recursive ManagerTermBaseCfg discovery that descends any configclass and yields each term with its path. New cfg layouts and observation sub-groups (perception, critic, camera_images, ...) are picked up automatically with no framework change. - _promote_scene_entity_cfgs and _swap_mdp consume the new walker; delete _walk_attrs / _iter_term_attrs / _TERM_PATHS. - Per-promotion log now lists the actual SceneEntityCfg paths. - Rename _require_direct_is_warp_task to _assert_direct_warp_registration. Update _detect_workflow with a note for adding new cfg roots. Manager term cfg contract - Relax stable ObservationTermCfg / RewardTermCfg / TerminationTermCfg func annotation to Callable[..., torch.Tensor | None] so the warp func(env, out) -> None contract type-checks alongside the existing torch return-value contract. - Drop the now-redundant warp-side manager_term_cfg.py shim and redirect the 9 relative imports onto isaaclab.managers.manager_term_cfg. Tests - New TestWalkTerms cases verify the recursive type-driven discovery. - TestPromoteSceneEntityCfgs / TestSwapMdp use real term cfgs and configclass fixtures so they exercise the actual walker contract.
The isaaclab_experimental fragment shrinks from 47 lines of nested narrative to 12 lines per the existing terse-fragment rule. Adds the matching fragments for the two other touched packages: isaaclab (the term-cfg type-hint relax) and isaaclab_tasks_experimental (.skip — the humanoid import redirect is internal).
Two missed-sync fixes carried alongside the manager_based viewport
sync that already landed earlier in this branch:
1. DirectRLEnvWarp viewport controller now mirrors stable
DirectRLEnv: '(has_gui or has_active_visualizers()) and has_kit()'
gate, instantiate ViewportCameraController accordingly. Replaces the
stale get_setting('/isaaclab/has_gui'/'render/offscreen') check and
removes the commented-out instantiation that left
viewport_camera_controller unconditionally None.
2. render(mode='rgb_array') in ManagerBasedRLEnvWarp and
DirectRLEnvWarp now read SimulationContext.has_gui and
has_offscreen_render properties instead of get_setting on keys that
were removed when the simulation manager was refactored. Without
this the gate silently fell through on None values.
|
@greptileai review |
| for depth in range(len(parts), 0, -1): | ||
| target = ".".join(parts[:depth] + ["mdp"]) | ||
| try: | ||
| modules.append(importlib.import_module(target)) | ||
| break | ||
| except ModuleNotFoundError as exc: | ||
| # Only swallow "this candidate doesn't exist" — a real | ||
| # import error inside an existing module is a bug. | ||
| if exc.name == target: | ||
| continue | ||
| raise |
There was a problem hiding this comment.
exc.name == target check is too strict — crashes when parent package is absent
ModuleNotFoundError.name is set to the specific module that was missing, which may be a parent package rather than the leaf target. For example, importing "isaaclab_tasks_experimental.locomotion.velocity.mdp" when isaaclab_tasks_experimental is not installed raises ModuleNotFoundError(name="isaaclab_tasks_experimental"). Since "isaaclab_tasks_experimental" != "isaaclab_tasks_experimental.locomotion.velocity.mdp", the condition is False and the exception is re-raised unconditionally — every depth in the walk fails the same way, so _warp_mdp_modules throws instead of falling back to the generic module. This turns the graceful "no task-specific twin module found" path into a hard crash any time isaaclab_tasks_experimental is not installed.
Change the guard to if exc.name and target.startswith(exc.name). This keeps the intent ("only swallow 'this candidate doesn't exist'") while correctly handling the case where a parent package is the missing link.
The legacy bare 'newton'/'kamino' preset names were renamed on develop to 'newton_mjwarp'/'newton_kamino' to disambiguate from the Newton backend label. Update the warp-frontend hard-check error message, the `_adapt_cfg_for_warp`/`_require_newton_physics` docstrings, the rsl_rl/train.py comment, and the matching test assertion to spell the canonical preset name so users get the right CLI token in errors.
Direct warp tasks duplicated their stable task configuration in a parallel *-Direct-Warp-v0 registration, so every stable cfg change had to be mirrored by hand. A stable direct registration can now declare its warp implementation with a warp_entry_point kwarg (mirroring env_cfg_entry_point); --frontend warp constructs that class with the stable cfg and swaps nothing else. Declared for Isaac-Cartpole-Direct, Isaac-Ant-Direct, and Isaac-Humanoid-Direct, whose warp cfgs matched the stable ones field-for-field; their *-Direct-Warp-v0 registrations and duplicated cfgs are removed and the env classes annotate the stable cfg types. The Allegro reorient warp task keeps its own registration: its cube is modeled as an articulation, unlike the stable task's rigid object, so the configurations genuinely differ.
Extract the --frontend flag into add_frontend_args so other entrypoints (e.g. play) can reuse it without pulling in all training arguments; add_common_train_args keeps calling it so every train and benchmark CLI registers the flag. Also derive SceneEntityCfg.from_stable from the stable dataclass fields instead of a hand-maintained list, drop single-use aliases in the wrench observation twin, and trim render comments to one line.
The experimental task package still used the manager_based/direct split
that the stable package left behind when it consolidated each task under
core/<task>. That mismatch was the only reason the warp MDP routing
needed non-obvious mappings.
Move the warp task packages to the same layout — core/cartpole,
core/locomotion/{ant,humanoid}, core/velocity, core/reach, and
core/reorient for the Allegro task — with each task package holding both
its manager-based MDP twins and its direct warp env class, exactly like
its stable counterpart. Routes and warp_entry_point declarations now
read as the mechanical mirror they are.
Group the warp-side machinery — route registry, cfg adaptation steps, twin resolution, and both env build paths — under a WarpFrontend class instead of a flat set of module functions. The module-level build(), adapt_cfg_for_warp(), and register_mdp_route() API is unchanged for callers; the class gives the machinery one named owner, keeps the route registry as explicit class state, and leaves room for future frontends to subclass or replace it.
The shared RL CLI already peels off the torch path before importing the optional isaaclab_experimental package, so the frontend's own torch branch (module-level build() with a Frontend enum) was dead code that duplicated the gym.make dispatch. Remove it: create_isaaclab_env owns the torch/warp dispatch and calls WarpFrontend.build_env directly, and the module's public surface shrinks to WarpFrontend, Workflow, register_mdp_route, and the error type. Callers use WarpFrontend.adapt_cfg in place of the adapt_cfg_for_warp alias.
The experimental tree now mirrors the stable tree exactly, so twin routing needs no declarations: mirror the module root (isaaclab -> isaaclab_experimental, isaaclab_tasks -> isaaclab_tasks_experimental) and look the symbol up on the nearest .mdp package of the mirrored path — the cfg's own task namespace first, then the symbol's defining package. Resolution is a pure function of the installed package tree; misses accumulate across the whole cfg and are reported in a single hard failure listing every missing twin and all searched modules. This deletes register_mdp_route, the route registry, the twin-provider side-effect import, and all five task-package registration calls. The humanoid MDP twins move to core/locomotion/mdp — the true mirror of the stable shared locomotion package — which also removes the Ant special case (both tasks now resolve mechanically).
Add warp adapters for the two stable startup randomization events (randomize_rigid_body_material, randomize_rigid_body_mass): the stable terms already dispatch to the active physics backend, so the adapters only convert the warp event manager's env-mask calling convention to the stable env-ids one and inherit the warp ManagerTermBase so the managers accept them as class terms. With those twins in place the stable flat velocity tasks adapt cleanly, so the 18 velocity *-Warp-v0 registrations and their delta configs are deleted — no manager-based warp registration remains, and the conversion test pins that end state. The Allegro reorient task id drops its stale -v0 suffix (Isaac-Reorient-Cube-Allegro-Direct-Warp).
Add one-line comments at the dense decision points of WarpFrontend (twin lookup order, mirror walk, collect-all-misses reporting). Replace the removed -Warp-v0 ids in the benchmarking guide with a same-task A/B that differs only in --frontend, with identical Newton physics on both runs. Add the missing changelog entry for the RewardManager merging class-term reset() extras into its episode logs.
The stable cfg already resolves its cube to an articulation under the newton_mjwarp preset, so the duplicated warp cfg diverged only by pinning ls_iterations=15; the stable preset values now apply. Isaac-Reorient-Cube-Allegro-Direct declares the warp env class via warp_entry_point; the separate -Warp registration and its cfg package are deleted. A new end-state pin asserts no -Warp task ids remain, and the frontend stub-task fixture now unregisters its ids so later test modules see the real registry.
hujc7
changed the title
Play backends constructed environments with a direct gym.make, so warp-trained policies could not be played through the official CLI: the shared parser accepted --frontend but play ignored it. All four dispatched play backends now build their environment via create_isaaclab_env, mirroring their train twins' MARL-conversion semantics, and a unit test pins the routing. The rlinf integration constructs environments inside the external framework and remains frontend-unroutable (documented in the changelog).
The Warp observation manager applies noise through the experimental noise cfg family, so stable noise cfgs on bridged tasks were silently skipped and training ran without the configured corruption. The frontend now swaps stable constant/uniform/gaussian noise cfgs to their warp-native twins during cfg adaptation (data fields copied, warp func default); cfgs it cannot represent — class-based noise models, customized funcs, tensor-valued parameters — are an aggregated hard error instead of a silent MDP change. The observation manager rejects non-warp noise cfgs at construction and plumbs the shared per-env RNG state to function-style noise kernels, wiring the contract the noise module documented. Audit: every function-style noise cfg in the repo has a swappable warp twin; class-based models are loud unsupported.
…nager-bridge # Conflicts: # docs/source/overview/environments.rst # source/isaaclab_rl/isaaclab_rl/entrypoints/backends/play_rl_games.py # source/isaaclab_rl/isaaclab_rl/entrypoints/backends/play_rsl_rl.py # source/isaaclab_rl/isaaclab_rl/entrypoints/backends/play_sb3.py # source/isaaclab_rl/isaaclab_rl/entrypoints/backends/play_skrl.py # source/isaaclab_rl/test/test_entrypoints_common.py
Use the presets= Hydra group name in the PhysX limitation note and drop performance rows for flat-velocity tasks removed by the catalog update (AnymalB, AnymalC, A1, Go1); rename the Go2 row to the UnitreeGo2 task name.
physics=newton_mjwarp is the precise form for the Newton-only limitation note: unlike the general presets= group, the typed selector validates that the resolved cfg is a physics block.
Add one-line docstrings to the previously undocumented @wp.kernel functions across the cartpole, locomotion, reach, reorient, and velocity task implementations.
2 participants
Review Map
1. Summary
--frontend {torch,warp}on the shared RL CLI (train and play) runs stable task ids on the warp runtime — no duplicated configs, no parallel warp task registrations at all (end state pinned by test). Therlinfintegration constructs environments inside the external framework and is not frontend-routable (known limitation).WarpFrontendadapts the stable cfg in place; MDP twins resolve by name on the mirrored experimental package tree — deterministic, registration-free, missing twins reported in one aggregated failure.warp_entry_point; only the env class swaps, the cfg is shared.isaaclab_tasks_experimentalmirrors the stablecore/<task>layout;Metrics/success_raterestored under warp.2. Migration map
Isaac-{Cartpole,Humanoid,Ant}-Warp-v0,Isaac-Reach-Franka-Warp-v0(+-Play)--frontend warp presets=newton_mjwarpIsaac-Velocity-Flat-<Robot>-Warp-v0(+-Play), all robots--frontend warp presets=newton_mjwarpIsaac-{Cartpole,Ant,Humanoid}-Direct-Warp-v0,Isaac-Reorient-Cube-Allegro-Direct-Warp-v0--frontend warp presets=newton_mjwarp(viawarp_entry_point)3. Test plan
develop;./isaaclab.sh -fclean on all files.Isaac-Velocity-Flat-UnitreeGo2,Isaac-Cartpole-Direct) re-run green; pre-commit clean on all files.Training time:present, no traceback):Isaac-Cartpole(torch);Isaac-Cartpole,Isaac-Humanoid,Isaac-Cartpole-Direct,Isaac-Ant-Direct,Isaac-Velocity-Flat-AnymalD,Isaac-Velocity-Flat-H1(warp; randomization events active);Isaac-Reorient-Cube-Allegro-Direct(warp).Metrics/success_ratereported under warp.3.1 Frontend A/B benchmark
Same stable task id and cfg on both sides,
presets=newton_mjwarpon both (identical Newton physics — only--frontenddiffers); rsl_rl, 4096 envs, 300 iterations, headless, one L40 per run, single run per cell. Metric: rsl_rlTraining time:— learn-loop wall-clock (env stepping + PPO update; warp includes its one-time CUDA-graph capture). PPO-update cost is common to both, so the env-pipeline gain exceeds the end-to-end ratio.Isaac-CartpoleIsaac-HumanoidIsaac-AntIsaac-Velocity-Flat-AnymalD4. Out of scope / follow-ups
TerrainImporterAPIs; no twins for IK/OSC actions,height_scan, or Digit/Spot-specific terms.DirectRLEnvWarpand stableDirectRLEnvinto a common base.