Skip to content

[Task Clean-up] Dexterous Part 4/11: Enable RSL-RL training for the handover Direct task#6414

Open
hujc7 wants to merge 9 commits into
isaac-sim:developfrom
hujc7:jichuanh/task-cleanup-dex-part04
Open

[Task Clean-up] Dexterous Part 4/11: Enable RSL-RL training for the handover Direct task#6414
hujc7 wants to merge 9 commits into
isaac-sim:developfrom
hujc7:jichuanh/task-cleanup-dex-part04

Conversation

@hujc7

@hujc7 hujc7 commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixes the multi-agent-to-single-agent adapter to expose current observations through the Direct RL interface used by RSL-RL (it previously returned stale buffers), with unit tests.
  • Adds an RSL-RL PPO runner configuration for the handover task and registers it on Isaac-Shadow-Handover-Direct, plus the same success-rate metrics as the reorientation tasks.
  • Fixes handover on the Newton backend end to end: per-backend actuated-joint mapping for the renamed asset, hand rotations composed with the asset's baked base rotation, and raised contact substeps for sustained ball contact. Validated by a full training run: success rate 0 → ~0.95 on the previously flatlined handover-Newton row.
  • The shared reward functions are implemented in Warp with parity tests, and the handover task constants live in a shared module consumed by the Direct cfg and the manager counterpart.
  • Validated by full handover training on PhysX (success streaks met); split out of the lumped validation branch [DO-NOT-MERGE][Task Clean-up] Dexterous: lumped validation branch (split into Parts 1-11) #6324 (Part 4 of 11).

Dependencies

Review updates (2026-07-09)

  • Review response: the MARL adapter no longer implements the private Direct-env observation hook. RslRlVecEnvWrapper.get_observations() now reads the public environment-owned observation buffer (obs_buf); DirectRLEnv.reset() stores the buffer like step() already does (matching the manager-based and multi-agent environments), and the adapter exposes the wrapped environment's latest observations through the same public attribute. The private path also skipped observation-noise corruption, so initial observations now match what training receives.
  • Completed the handover constants module: scalar task parameters move to handover_task_constants and the Direct cfg consumes them; Warp reward wrappers take caller-owned buffers; the Newton hand rotation composes in float64 via wp.quatd; stale J0 comment fixed.

The multi-agent-to-single-agent adapter now exposes current
observations through the Direct RL interface used by RSL-RL (it
previously returned stale buffers). Adds an RSL-RL PPO runner
configuration and registers it on the Direct handover task, and adds
the same success-rate metrics as the reorientation tasks.

Also fixes the Newton distal-joint override to match the renamed
distal joints (J0 -> J1) in the current Shadow Hand Newton asset,
without which the handover environments fail to construct on Newton.

Validated by full handover training on PhysX (success streaks met).
@greptile-apps

greptile-apps Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR enables RSL-RL PPO training on the Shadow Hand handover Direct task by fixing the MARL-to-single-agent adapter to expose live observations via _get_observations, adding a HandoverPPORunnerCfg, registering it on Isaac-Shadow-Handover-Direct, and fixing the Newton distal-joint actuator override from J0 to J1 to match the renamed asset joints.

  • MARL adapter fix: adds _convert_observations/_get_observations helpers plus an episode_length_buf property with setter and __getattr__ delegation so RSL-RL sees current rather than stale step-return observations; covered by three new unit tests.
  • Handover env additions: success-rate metrics, EpisodeErrorRecorder for per-episode goal-distance diagnostics, and sample_joint_positions_within_limits for cleaner reset sampling are wired in; these depend on isaaclab_tasks.core.utils from PR [Task Clean-up] Dexterous Part 3/11: Add success-rate metrics to the reorientation Direct tasks #6413 (declared dependency).
  • Missing module: handover_env.py also imports evaluate_handover_success and handover_reward from isaaclab_tasks.core.handover.mdp.rewards, a module that is absent from the repository and not named as coming from any dependency PR — this will cause an ImportError at task load time.

Confidence Score: 3/5

Not safe to merge as-is: handover_env.py will raise ModuleNotFoundError on import due to a missing mdp/rewards.py module that is neither present in the repo nor named as a dependency.

The handover_env.py change adds an import from isaaclab_tasks.core.handover.mdp.rewards (evaluate_handover_success, handover_reward). That package path does not exist anywhere in the repository, and neither this PR nor its declared dependency (#6413) explicitly accounts for it. Every code path that constructs or imports HandoverEnv will fail immediately with an unresolved import. The MARL adapter changes and the RSL-RL config are clean, and the Newton joint-name fix is correct, but the broken import makes the core task class unusable until the missing module is supplied or the import is corrected.

source/isaaclab_tasks/isaaclab_tasks/core/handover/handover_env.py — the import from isaaclab_tasks.core.handover.mdp.rewards references a module that does not exist.

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/envs/utils/marl.py Adds __getattr__ delegation, episode_length_buf property+setter, and _get_observations/_convert_observations helpers to the MARL-to-single-agent adapter so RSL-RL gets fresh observations instead of stale step/reset buffers. Logic is correct; minor inefficiency when state_as_observation=True calls the inner env's _get_observations unnecessarily.
source/isaaclab/test/envs/test_marl_utils.py New unit tests covering concatenation, state-as-observation mode, and episode-length forwarding for the MARL adapter. Correctly uses torch.testing.assert_close and validates both getter and setter paths.
source/isaaclab_tasks/isaaclab_tasks/core/handover/handover_env.py Imports evaluate_handover_success and handover_reward from isaaclab_tasks.core.handover.mdp.rewards, a module that does not exist in the repository; will raise ModuleNotFoundError at import time. Also imports from isaaclab_tasks.core.utils (declared dependency from #6413).
source/isaaclab_tasks/isaaclab_tasks/core/handover/handover_env_cfg.py Renames distal-joint regex from J0 to J1 in the Newton actuator override to match the renamed Shadow Hand Newton asset joints; docstring updated to match. One inline comment still says "J0" (stale).
source/isaaclab_tasks/isaaclab_tasks/core/handover/agents/rsl_rl_ppo_cfg.py New RSL-RL PPO runner config (HandoverPPORunnerCfg) with a 4-layer MLP actor/critic and standard PPO hyperparameters; consistent with other dexterous task configs in the repo.
source/isaaclab_tasks/isaaclab_tasks/core/handover/init.py Adds rsl_rl_cfg_entry_point to the Isaac-Shadow-Handover-Direct Gym registration, pointing at the new HandoverPPORunnerCfg.
source/isaaclab_tasks/test/core/test_handover_rsl_rl_cfg.py Tests that the Gym entry point resolves to the correct config class and that combined observation/action dimensions (314/40) match expectations for the two-hand setup.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant RSL as RSL-RL Runner
    participant Adapter as multi_agent_to_single_agent (Env)
    participant MARL as HandoverEnv (DirectMARLEnv)

    RSL->>Adapter: reset(seed, options)
    Adapter->>MARL: reset(seed, options)
    MARL-->>Adapter: obs (per-agent dict), extras
    Adapter->>Adapter: _convert_observations(obs)
    Adapter-->>RSL: "{"policy": concat_obs}, extras"

    RSL->>Adapter: _get_observations()
    Note over Adapter: NEW: exposes live obs for RSL-RL direct access
    Adapter->>MARL: _get_observations()
    MARL-->>Adapter: obs (per-agent dict)
    Adapter->>Adapter: _convert_observations(obs)
    Adapter-->>RSL: "{"policy": concat_obs}"

    RSL->>Adapter: step(action)
    Adapter->>Adapter: split action by agent
    Adapter->>MARL: "step({right_hand: a0, left_hand: a1})"
    MARL-->>Adapter: obs, rewards, terminated, time_outs, extras
    Adapter->>Adapter: _convert_observations(obs)
    Adapter-->>RSL: obs, sum(rewards), AND(terminated), AND(time_outs), extras

    RSL->>Adapter: "episode_length_buf = value"
    Note over Adapter: Property setter forwards write to MARL env
    Adapter->>MARL: "episode_length_buf = value"
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant RSL as RSL-RL Runner
    participant Adapter as multi_agent_to_single_agent (Env)
    participant MARL as HandoverEnv (DirectMARLEnv)

    RSL->>Adapter: reset(seed, options)
    Adapter->>MARL: reset(seed, options)
    MARL-->>Adapter: obs (per-agent dict), extras
    Adapter->>Adapter: _convert_observations(obs)
    Adapter-->>RSL: "{"policy": concat_obs}, extras"

    RSL->>Adapter: _get_observations()
    Note over Adapter: NEW: exposes live obs for RSL-RL direct access
    Adapter->>MARL: _get_observations()
    MARL-->>Adapter: obs (per-agent dict)
    Adapter->>Adapter: _convert_observations(obs)
    Adapter-->>RSL: "{"policy": concat_obs}"

    RSL->>Adapter: step(action)
    Adapter->>Adapter: split action by agent
    Adapter->>MARL: "step({right_hand: a0, left_hand: a1})"
    MARL-->>Adapter: obs, rewards, terminated, time_outs, extras
    Adapter->>Adapter: _convert_observations(obs)
    Adapter-->>RSL: obs, sum(rewards), AND(terminated), AND(time_outs), extras

    RSL->>Adapter: "episode_length_buf = value"
    Note over Adapter: Property setter forwards write to MARL env
    Adapter->>MARL: "episode_length_buf = value"
Loading

Comments Outside Diff (1)

  1. source/isaaclab_tasks/isaaclab_tasks/core/handover/handover_env_cfg.py, line 130 (link)

    P2 The inline comment above _SHADOW_HAND_NEWTON_CFG still references "J0" after the joint rename to "J1" was applied to both the actuator config and the function docstring, leaving this brief description stale.

    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!

Reviews (1): Last reviewed commit: "Enable RSL-RL training for the handover ..." | Re-trigger Greptile

)

from isaaclab_tasks.core.handover.handover_env_cfg import HandoverEnvCfg
from isaaclab_tasks.core.handover.mdp.rewards import evaluate_handover_success, handover_reward

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 Missing module: isaaclab_tasks.core.handover.mdp.rewards

handover_env.py imports evaluate_handover_success and handover_reward from isaaclab_tasks.core.handover.mdp.rewards, but no such module exists anywhere in the repository — there is no mdp/ subdirectory under core/handover/. The PR description names PR #6413 as the only declared code dependency (for isaaclab_tasks.core.utils), but that PR's description only mentions "core.utils helpers" and does not describe adding a handover-specific mdp/rewards.py. Without this file the task class will raise ModuleNotFoundError on import, blocking all training runs.

Comment on lines +106 to +108
def _get_observations(self) -> VecEnvObs:
"""Return current observations through the single-agent interface."""
return self._convert_observations(self.env._get_observations())

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 Unnecessary _get_observations() call in state-as-observation mode

When _state_as_observation=True, _get_observations still calls self.env._get_observations() (which may trigger sensor data collection) and then immediately discards the result inside _convert_observations, which pivots to self.env.state(). The call is harmless but wasteful — consider short-circuiting before the inner call, e.g. returning {"policy": self.env.state()} directly when in state-as-observation mode.

hujc7 added 2 commits July 8, 2026 12:56
The Direct handover environment imports handover_reward and
evaluate_handover_success from the handover MDP package, so the
multi-agent environment test could not construct the task without it.
The manager-only remainder of the package lands with the manager
counterpart PR.
@Toni-SM

Toni-SM commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Fixes the multi-agent-to-single-agent adapter to expose current observations through the Direct RL interface...

Such implementation should not be accepted as it is trying to fix a problem a RL library has (require a private method, that is not part of the standard API Gymnasium / Petting-Zoo Parallel API to initialize its logic) from Isaac Lab side.

There is nothing to fix here. The fix must be implemented in the RL library to do not depend on the private _get_observations method that is not part of the standard API, but instead to rely on public properties and method of the standard environment API to initialize the library.

@kellyguo11 for viz

hujc7 added 2 commits July 9, 2026 01:05
Three stacked fixes validated by full training (success rate 0.74 by
iteration 206 from a permanent flatline): per-backend actuated-joint
names via the physics preset key, hand root rotations composed with
the asset's baked base rotation instead of replacing it, and four
solver substeps for sustained ball-palm contact. The shared handover
reward functions move to Warp kernels with parity tests, and the
multi-agent adapter skips computing observations it discards in
state-as-observation mode.
Per-backend actuated-joint mapping for the renamed Newton asset, hand
rotations composed with the asset's baked base rotation, raised contact
substeps, Warp reward functions with parity tests, the skipped
discarded-observation computation in the MARL adapter, and the shared
handover constants module. Validated: success rate 0 -> ~0.95 on the
previously flatlined handover-Newton row.
Comment on lines +96 to +111
def _convert_observations(self, obs: dict[AgentID, ObsType]) -> VecEnvObs:
"""Convert multi-agent observations to the single-agent policy observation."""
if self._state_as_observation:
obs = {"policy": self.env.state()}
# concatenate agents' observations
# FIXME: This implementation assumes the spaces are fundamental ones. Fix it to support composite spaces
else:
obs = {
"policy": torch.cat(
[obs[agent].reshape(self.num_envs, -1) for agent in self.env.possible_agents], dim=-1
)
}
return {"policy": self.env.state()}
return {
"policy": torch.cat(
[obs[agent].reshape(self.num_envs, -1) for agent in self.env.possible_agents], dim=-1
)
}

return obs, extras
def _get_observations(self) -> VecEnvObs:
"""Return current observations through the single-agent interface."""
if self._state_as_observation:
# the state replaces the observations entirely; skip computing them
return {"policy": self.env.state()}
return self._convert_observations(self.env._get_observations())

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.

The scalar task parameters (decimation, episode length, reset noise,
reward scales, thresholds) move into the constants module and the
Direct cfg consumes them, so the manager counterpart can read the same
values without instantiating the Direct cfg. The Warp reward wrappers
take caller-owned buffers, the Newton hand rotation composes in float64
via wp.quatd, and the restored MARL FIXME marks the fundamental-space
assumption at its consolidated site.

Per review feedback, the MARL adapter no longer implements the private
Direct-env observation hook: RslRlVecEnvWrapper caches the reset/step
observations and serves get_observations() from the standard API.
hujc7 added 2 commits July 9, 2026 08:52
The scalar task parameters (decimation, episode length, reset noise,
reward scales, thresholds) move into the constants module and the
Direct cfg consumes them, so the manager counterpart can read the same
values without instantiating the Direct cfg. The Warp reward wrappers
take caller-owned buffers, the Newton hand rotation composes in float64
via wp.quatd, and the restored MARL FIXME marks the fundamental-space
assumption at its consolidated site.

Per review feedback, the MARL adapter no longer implements the private
Direct-env observation hook: RslRlVecEnvWrapper caches the reset/step
observations and serves get_observations() from the standard API.
The scalar task parameters (decimation, episode length, reset noise,
reward scales, thresholds) move into the constants module and the
Direct cfg consumes them, so the manager counterpart can read the same
values without instantiating the Direct cfg. The Warp reward wrappers
take caller-owned buffers, the Newton hand rotation composes in float64
via wp.quatd, and the restored MARL FIXME marks the fundamental-space
assumption at its consolidated site.

Per review feedback, the MARL adapter no longer implements the private
Direct-env observation hook: RslRlVecEnvWrapper reads the public
observation buffer that every environment maintains, DirectRLEnv.reset
stores that buffer like step already does, and the adapter exposes the
wrapped environment's latest observations through the same attribute.
@hujc7 hujc7 requested a review from kellyguo11 as a code owner July 9, 2026 09:20
The scalar task parameters (decimation, episode length, reset noise,
reward scales, thresholds) move into the constants module and the
Direct cfg consumes them, so the manager counterpart can read the same
values without instantiating the Direct cfg. The reward kernels launch directly on Warp-native environment state
with caller-owned, view-cached buffers, the Newton hand rotation
composes in float64 via wp.quatd, and the restored MARL FIXME marks the fundamental-space
assumption at its consolidated site.

Per review feedback, the MARL adapter no longer implements the private
Direct-env observation hook: RslRlVecEnvWrapper reads the public
observation buffer that every environment maintains, DirectRLEnv.reset
stores that buffer like step already does, and the adapter exposes the
wrapped environment's latest observations through the same attribute.
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.

2 participants