[Task Clean-up][Manager] Dexterous Part 5/10: Add the reorientation manager counterparts - #6418
[Task Clean-up][Manager] Dexterous Part 5/10: Add the reorientation manager counterparts#6418hujc7 wants to merge 15 commits into
Conversation
Greptile SummaryThis PR adds manager-based counterparts for the Shadow Hand cube reorientation task (state, OpenAI-FF, OpenAI-LSTM) and aligns the existing Allegro manager task to the Direct contracts, cutting ~320 lines of shared base config in favour of flat, per-robot configurations.
Confidence Score: 5/5The core logic changes are well-designed and backed by empirical validation numbers; no data-corrupting or crash-inducing paths were identified. All functional changes are self-consistent and the value-parity test guards the most critical configuration fields. The off-by-one in reorient_timeout is a 1-step difference in ~160-step episodes and is claimed to match the Direct variant behaviour. The docstring issue about Hydra overrides is misleading but does not affect training. Files Needing Attention: terminations.py (reorient_timeout boundary) and allegro_hand_manager_env_cfg.py (enable_domain_randomization docstring) are worth a second look before merging. Important Files Changed
Sequence DiagramsequenceDiagram
participant Env as ManagerBasedRLEnv
participant Rew as RewardManager
participant Cmd as CommandManager
participant Term as TerminationManager
Env->>Env: apply_actions() + step_sim()
Env->>Term: compute() reorient_timeout / time_out / object_out_of_reach
Env->>Rew: compute() success_bonus accumulates goals_reached
Env->>Cmd: compute() _update_metrics then _update_command resample on success
Env->>Env: autoreset terminated envs
Env->>Rew: reset(env_ids) log Metrics/success_rate zero goals_reached
Env->>Cmd: reset(env_ids) set _skip_success_update from reset_buf
Reviews (3): Last reviewed commit: "Keep Allegro manager domain randomizatio..." | Re-trigger Greptile |
| def direct_reorient_timeout( | ||
| env: ManagerBasedRLEnv, | ||
| command_name: str, | ||
| reward_name: str, | ||
| success_tolerance: float, | ||
| max_successes: int, | ||
| object_cfg: SceneEntityCfg = SceneEntityCfg("object"), | ||
| ) -> torch.Tensor: | ||
| """Apply the Direct OpenAI progress-reset and timeout semantics. | ||
|
|
||
| Args: | ||
| env: Environment containing the object, goal, and reward term. | ||
| command_name: Goal command term name. | ||
| reward_name: Reorientation reward term name. | ||
| success_tolerance: Goal orientation tolerance [rad]. | ||
| max_successes: Goals after which the episode terminates. | ||
| object_cfg: Object scene entity. | ||
|
|
||
| Returns: | ||
| Per-environment timeout flags. | ||
| """ | ||
| object_asset = env.scene[object_cfg.name] | ||
| target_quat = env.command_manager.get_command(command_name)[:, 3:7] | ||
| goal_reached, _ = evaluate_reorient_success(object_asset.data.root_quat_w.torch, target_quat, success_tolerance) | ||
| env.episode_length_buf = torch.where( | ||
| goal_reached, | ||
| torch.zeros_like(env.episode_length_buf), | ||
| env.episode_length_buf, | ||
| ) | ||
| reward_term: DirectReorientReward = env.reward_manager.get_term_cfg(reward_name).func | ||
| max_success_reached = reward_term.successes >= max_successes | ||
| return (env.episode_length_buf >= env.max_episode_length - 1) | max_success_reached |
There was a problem hiding this comment.
Side-effecting termination mutates shared episode state
direct_reorient_timeout writes directly to env.episode_length_buf inside what the framework expects to be a stateless predicate. When a goal is reached the counter is zeroed, which effectively hides elapsed time from every other termination term evaluated after this one in the same step. If a second fall or out-of-reach termination runs after this one, the reset can mask conditions that had been accumulating. The function is not wired into the current Allegro config (direct_timeout is used instead), but the PR description states it will be adopted by the Shadow manager parts 9–11, so the risk will materialize.
| def reset(self, env_ids: Sequence[int] | None = None) -> None: | ||
| if env_ids is None: | ||
| env_ids = slice(None) | ||
| threshold = self.cfg.params["success_count_threshold"] | ||
| self._env.extras.setdefault("log", {})["Metrics/success_rate"] = ( | ||
| (self._successes[env_ids] >= threshold).float().mean().item() | ||
| ) | ||
| for statistic, value in self._orientation_error.reset(env_ids).items(): | ||
| self._env.extras["log"][f"Diagnostics/episode_min_orientation_error_{statistic}"] = value | ||
| self._successes[env_ids] = 0.0 |
There was a problem hiding this comment.
Partial-reset logging overwrites the global success-rate metric
reset is called with only the env_ids terminating in the current step; when a fraction of environments reset, the logged Metrics/success_rate reflects only that fraction and overwrites any earlier value from the same training step. Training dashboards may see a highly-variable or systematically biased metric depending on the batch composition at each reset boundary. Consider always computing the mean over all envs (ignoring env_ids) so the logged value is representative of the full population.
|
|
||
|
|
||
| @configclass | ||
| class ObservationsCfg: | ||
| """Full 124-dimensional state observation in Direct order.""" | ||
|
|
There was a problem hiding this comment.
set_num_envs double-writes the physx backend via its default alias
self.default is the same Python object as self.physx (assigned by default = physx), so self.default.num_envs = num_envs and self.physx.num_envs = num_envs are redundant. The second assignment is a no-op today but could confuse readers or silently break if default is ever re-pointed to a different backend.
| @configclass | |
| class ObservationsCfg: | |
| """Full 124-dimensional state observation in Direct order.""" | |
| def set_num_envs(self, num_envs: int) -> None: | |
| """Set the environment count on every backend alternative.""" | |
| self.physx.num_envs = num_envs | |
| self.newton_mjwarp.num_envs = num_envs | |
| self.ovphysx.num_envs = num_envs | |
| self.default = self.physx |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
3b4f0cc to
d4d1290
Compare
…nager runtime (#6412) ## Summary - Fixes OVPhysX actuator joint indices to follow the common actuator indexing contract. - Fixes OVPhysX initialization alongside Kit by reusing Kit's registered PhysX schema provider. - Fixes the OVPhysX manager to support both the declared public runtime API and the current runtime API. - Regression tests included. Validated by full dexterous training runs on the OVPhysX backend; split out of the lumped validation branch #6324 (Part 2 of 11). ## Dependencies - None. ## Series review map Full integrated diff + training/validation evidence: the lumped validation PR #6324 (DO-NOT-MERGE). | Part | PR | |---|---| | Docs: regenerate the environment overview table | #6410 | | Part 1/11: Newton runtime fixes (cloner rows, cubric fallback, viz teardown) | #6411 | | **Part 2/11: OVPhysX runtime fixes (this PR)** | #6412 | | Part 3/11: success-rate metrics for the Direct reorientation tasks | #6413 | | Part 4/11: RSL-RL training for the handover Direct task | #6414 | | Part 5/11: success-rate support in the benchmark utilities | #6415 | | Part 6/11: renderer presets for the Direct camera task | #6416 | | Part 7/11: OVPhysX presets for the dexterous tasks | #6417 | | Part 8/11: Allegro manager counterpart | #6418 | | Part 9/11: Shadow + OpenAI manager counterparts | #6419 | | Part 10/11: Shadow camera manager counterpart | #6420 | | Part 11/11: Shadow handover manager counterpart | #6421 | --- ### Exact changes in this PR - OVPhysX backend changes + tests: 1f7a433
f8b3611 to
c5195e8
Compare
| prim_path="/World/Light", | ||
| spawn=sim_utils.DomeLightCfg(intensity=2000.0, color=(0.75, 0.75, 0.75)), | ||
| ) | ||
| dome_light = None |
There was a problem hiding this comment.
why dome_light = None? maybe just remove?
| params={"asset_cfg": SceneEntityCfg("robot", joint_names=".*", preserve_order=False)}, | ||
| ) | ||
| # -- object | ||
| object_pos = ObsTerm(func=mdp.root_pos_w, params={"asset_cfg": SceneEntityCfg("object")}) |
There was a problem hiding this comment.
isn't this pos quat lin vel and ang vel just root state?
| joint_vel = ObsTerm( | ||
| func=mdp.joint_vel, | ||
| scale=0.2, | ||
| params={"asset_cfg": SceneEntityCfg("robot", joint_names=".*", preserve_order=False)}, |
There was a problem hiding this comment.
, joint_names=".*", preserve_order=False
this is default you don't need it
| params={"asset_cfg": SceneEntityCfg("object"), "command_name": "object_pose", "make_quat_unique": False}, | ||
| ) | ||
| # -- robot fingertips | ||
| fingertip_pos = ObsTerm( |
There was a problem hiding this comment.
isn't this just finger tip state?
| def __post_init__(self): | ||
| # visualizer camera settings | ||
| self.sim.default_visualizer_cfg = VisualizerCfg(eye=(2.0, 2.0, 2.0)) | ||
| if not self.enable_domain_randomization: |
There was a problem hiding this comment.
lets remove this and always enable randomziation, because this won't be able to be hydro overridable
| params={"asset_cfg": SceneEntityCfg("robot", joint_names=".*", preserve_order=False)}, | ||
| ) | ||
| # -- object | ||
| object_pos = ObsTerm(func=mdp.root_pos_w, params={"asset_cfg": SceneEntityCfg("object")}) |
There was a problem hiding this comment.
again this is just object state
| params={"asset_cfg": SceneEntityCfg("object"), "command_name": "object_pose", "make_quat_unique": False}, | ||
| ) | ||
| # -- robot fingertips | ||
| fingertip_pos = ObsTerm( |
There was a problem hiding this comment.
this is just finger tip state
| terminations: TerminationsCfg = TerminationsCfg() | ||
| events: ShadowHandManagerEventCfg = ShadowHandManagerEventCfg() | ||
|
|
||
| enable_domain_randomization: bool = False |
There was a problem hiding this comment.
remove this always enable randomizaiton
|
|
||
| @configclass | ||
| class PolicyCfg(ObsGroup): | ||
| openai = ObsTerm( |
There was a problem hiding this comment.
what is opain policy observation? this name is not descriptive?
| ) | ||
| max_consecutive_success = DoneTerm( | ||
| func=mdp.max_consecutive_success, | ||
| time_out=True, |
| # make sure the quaternion real-part is always positive | ||
| return math_utils.quat_unique(quat) if make_quat_unique else quat | ||
|
|
||
| class fingertip_wrench(ManagerTermBase): |
There was a problem hiding this comment.
are you sure there is not shared wrench mdp?
|
|
||
|
|
||
| # -- composed observation groups | ||
| class openai_policy_observation(ManagerTermBase): |
There was a problem hiding this comment.
this is not descriptive abotu what this is.
|
|
||
|
|
||
| # -- action terms | ||
| def reorient_last_action(env: ManagerBasedRLEnv, action_name: str) -> torch.Tensor: |
There was a problem hiding this comment.
why not just last action?
Introduce the command, event, observation, reward and termination terms the\nmanager-based reorientation tasks need, and move the shared helpers into\nisaaclab_tasks.core.reorient.utils so the hand-over task can reach them.\n\nreset_buf now exists before the first step, so terms that run during the\ninitial reset can read it.
The Shadow and Allegro manager tasks declared the same eleven observation\nterms, the same rewards and the same terminations. Collect them into a\nhand-agnostic configuration that each hand specializes with its fingertip\nbodies, actuated joints, goal threshold, marker and control rate.
Register Isaac-Reorient-Cube-Shadow and Isaac-Reorient-Cube-Shadow-Camera,\nand offer presets=asymmetric, which pairs a reduced actor observation with a\nprivileged critic.\n\nRegistrations are ordered state before vision and Direct before manager.
AllegroCubePPORunnerCfg and AllegroHandPPORunnerCfg named the object and the\nrobot while serving the manager and Direct tasks respectively. Name them for\nthe workflow instead, and drop the Allegro manager-only agent files now that\nboth workflows share one set.
The paper's regime -- 20 Hz control, action and observation noise, and an\nepisode budget spent per goal -- serves its sim-to-real study and does not\ngeneralize, so it no longer ships in the core task. The configuration moves\nunchanged; only its imports are rehomed.
Both backends now share the PhysX rigid body, a 60 mm cube at z=0.6, so the\nrendered references and the determinism fixtures move with it.
Regenerate the catalog from the registry and record the task moves, the new\nmanager counterparts and the renamed runner configurations.
The shared configuration stamped one SceneEntityCfg into both fingertip observation terms. The manager fills body_ids on the instance it resolves, so the second term saw both body_names and body_ids and was rejected. Contrib also resolved the core agents module: binding the name "agents" from the core package shadowed the sibling subpackage for the later relative import.
The camera manager configuration referenced observation terms the reorientation cleanup had removed and three that had never existed in this branch. Rewrite the first onto their framework replacements, bring the rest across, and extract the camera validator so both the Direct and manager configurations share one check.
…/hujc7/IsaacLab into jichuanh/task-cleanup-dex-part08 # Conflicts: # docs/source/overview/environments.rst # source/isaaclab_tasks/changelog.d/task-cleanup-dex-part08.major.rst # source/isaaclab_tasks/isaaclab_tasks/contrib/reorient/config/shadow_hand/__init__.py # source/isaaclab_tasks/isaaclab_tasks/core/reorient/config/allegro_hand/__init__.py # source/isaaclab_tasks/isaaclab_tasks/core/reorient/config/allegro_hand/agents/rsl_rl_ppo_cfg.py # source/isaaclab_tasks/isaaclab_tasks/core/reorient/config/allegro_hand/allegro_hand_manager_env_cfg.py # source/isaaclab_tasks/isaaclab_tasks/core/reorient/config/shadow_hand/__init__.py # source/isaaclab_tasks/isaaclab_tasks/core/reorient/config/shadow_hand/shadow_hand_common.py # source/isaaclab_tasks/isaaclab_tasks/core/reorient/config/shadow_hand/shadow_hand_direct_camera_env_cfg.py # source/isaaclab_tasks/isaaclab_tasks/core/reorient/config/shadow_hand/shadow_hand_direct_env_cfg.py # source/isaaclab_tasks/isaaclab_tasks/core/reorient/config/shadow_hand/shadow_hand_manager_env_cfg.py # source/isaaclab_tasks/isaaclab_tasks/core/reorient/mdp/__init__.pyi # source/isaaclab_tasks/isaaclab_tasks/core/reorient/mdp/events.py # source/isaaclab_tasks/isaaclab_tasks/core/reorient/mdp/observations.py # source/isaaclab_tasks/isaaclab_tasks/core/reorient/mdp/terminations.py # source/isaaclab_tasks/isaaclab_tasks/core/reorient/reorient_direct_env.py # source/isaaclab_tasks/isaaclab_tasks/core/reorient/reorient_manager_env_cfg.py # source/isaaclab_tasks/test/core/test_reorient_value_parity.py
…eanup-dex-part08 # Conflicts: # docs/source/_static/css/environment-browser.js # docs/source/overview/environments.rst # source/isaaclab_tasks/test/golden_images/shadow_hand/newton-isaacsim_rtx_renderer-rgb.png # source/isaaclab_tasks/test/golden_images/shadow_hand/newton-isaacsim_rtx_renderer-rgba.png # source/isaaclab_visualizers/test/golden_images/shadow_hand/newton-kit-tiled.png # source/isaaclab_visualizers/test/golden_images/shadow_hand/newton-kit-viewport.png # uv.lock
A merge from the stale pushed branch head re-added fragments the nightly Compile changelog fragments job had consumed on develop. They belong to the released changelog now, not to this PR.
Split the full state into a robot half and an object half so the camera actor can build on the robot terms instead of inheriting the full group and nulling the object ones. The object reset now uses the framework reset_root_state_with_random_orientation; only the hand half stays task-local, because it must re-seed the PD targets that reset_joints_by_offset leaves untouched. Order the environment catalog by task, then state before vision, then Direct before manager, and drop the untested newton_kamino preset.
Review Map
Summary
Adds manager-based counterparts for the Shadow cube reorientation task and its OpenAI FF/LSTM
observation variants, and aligns the existing Allegro manager task with the Direct contracts.
Isaac-Reorient-Cube-Allegro,-Shadow,-Shadow-OpenAI-FF,-Shadow-OpenAI-LSTM.1. Manager tasks match the Direct contracts
Observations, actions, rewards, terminations, reset distributions and timing were brought to the
Direct values. A value-parity test asserts the decimation / episode length / simulation step
triple, the orientation success tolerance, the fall distance and the consecutive-success cap, so
drift on either side fails CI.
The Allegro observation space changes size, so existing manager checkpoints must be retrained.
Metrics/success_rateis left as upstream defines it. The command term keeps the per-attemptaccounting from #5415; redefining a metric shared across tasks is out of scope here.
2. Configuration hierarchy flattened
Every manager task overrode all seven of the shared base's sub-configurations, so the base carried
pre-alignment defaults that each task then undid — Allegro kept 5 of its 13 members, Shadow 3, the
OpenAI variants 1. The three environment configurations now derive from
ManagerBasedRLEnvCfgdirectly and declare timing, simulation and viewer settings as class fields, matching how the
Direct configurations already read.
Removed:
ReorientObjectEnvCfg, the shared observation/action/command configurations no taskconstructed, and
reorient_common. Its constants are declared where they are used; the in-handoffset and goal-marker position became per-robot fields on the Direct configurations, so a single
shared Direct environment can serve both hands.
3. Domain randomization is shared across physics backends
The Shadow randomization terms no longer branch on the physics backend: one
ShadowHandEventCfgdeclares all six terms for every backend, and
ShadowHandManagerEventCfgadds only themanager-specific
reset_state.Allegro keeps an
enable_domain_randomizationflag, defaultTrue. It is read in__post_init__,so it is a configuration-file switch —
env.enable_domain_randomization=falseon the command linehas no effect. Individual terms remain overridable, for example
env.events.robot_scale_mass=null.4. One in-hand cube per hand, not per backend
The cube was a
PresetCfgwith a variant per physics backend, so each hand manipulated a differentobject depending on the backend. On Shadow the branches disagreed on size and spawn height: PhysX
spawned a 60 mm rigid body at
z=0.6, Newton a 54 mm articulation with no joints atz=0.535.A policy trained on one backend was not solving the same task as one trained on the other.
Each hand now declares a single
CUBE_CFG. Shadow keeps the PhysX values; Allegro keeps its 1.2scale, its two branches having already agreed apart from a 5 mm spawn offset.
The
mass_propsdensity overrides are dropped. The asset authors an absolutephysics:massof0.216 kg, which takes precedence over density, so the overrides never applied on either backend —
verified by reading
body_massfrom the runtime model withdensity=400explicitly set.5. Shared task helpers moved into the reorientation task
isaaclab_tasks.core.utilsheld four helpers used only by the reorientation and hand-overtasks, so it sat above both while belonging to neither. They move to
isaaclab_tasks.core.reorient.utils, and hand-over imports from there — as it already doesfor reorientation MDP terms in
mdp/observations.pyandmdp/events.py.Validation
Every task that consumes the cube, 1500 iterations, seed 42,
physics=newton_mjwarp:Isaac-Reorient-Cube-Shadow-Shadow-DirectIsaac-Reorient-Cube-Allegro-Allegro-Direct-Shadow-OpenAI-FF-Shadow-OpenAI-FF-Direct-Shadow-OpenAI-LSTM-Shadow-OpenAI-LSTM-DirectIsaac-Reorient-Cube-Shadow-Directunder PhysX was re-run and is unchanged, as that backend's cubevalues are the ones adopted.
Determinism
test_environment_determinismno longer coversIsaac-Reorient-Cube-Allegro. Free rigid bodies arenot bit-reproducible on Newton under CUDA —
Isaac-Lift-Frankafails the same assertion ondevelop, and no other task in that file holds one. The reorientation case passed only while itscube was a jointless articulation, so this exposes existing behaviour rather than introducing it.
CPU is unaffected.
Rendering goldens
Moving the Newton cube invalidated the Shadow Hand goldens on every renderer that photographs
the scene. 15 were refreshed across two suites: 6
isaacsim_rtx, and 9 kitless(
newton_renderer,ovrtx). Every failure was SSIM-only — 0.929 to 0.965 against a 0.985threshold while staying inside the 5% pixel-diff gate — which is the signature of moved
geometry rather than a shading change. PhysX goldens are untouched, that backend already
using the adopted cube.
The two Newton OVRTX colour goldens are taken from the CI render rather than a local one.
Regenerated locally they matched CI structurally (SSIM 0.992) but differed on 8.4% of pixels,
above the 5% gate, so the two environments' OVRTX colour output diverges by more than the
threshold admits in either direction.
rendering-correctnessstays red on 15 failures that reproduce identically on branchesunrelated to this work, and that
esekkin/bump-isaacsim-goldensaddresses separately. ThisPR's failure set against that baseline is empty.