[Task Clean-up][Manager] Dexterous Part 5/9: Add the reorientation manager counterparts

[hujc7]

Review Map

• Exact changes: a stacked PR's page shows the cumulative diff of its dependency chain; the link pins the commit range that is the PR's own contribution.
• Links pin specific SHAs and go stale after a branch update. Parts 7–9 are pending a rebase onto the current stack, so their ranges are omitted rather than shown wrong.

PR	Status	Depends on	Exact changes
#6411 Part 1/9: Newton cloner/cubric/visualizer fixes		—	merged
#6412 Part 2/9: OVPhysX articulation + manager runtime		—	merged
#6413 Part 3/9: Reorient Direct, torch		—	merged
#6414 Part 4/9: MARL-to-single-agent fix + handover/camera Direct		—	merged
📌 #6418 Part 5/9: Reorient manager counterparts		—	—
#6421 Part 6/9: Handover + camera manager counterparts		#6418	changes
#6410 Part 7/9: Environment overview docs		#6421	pending rebase
#6415 Part 8/9: Benchmark success-rate utilities		#6421	pending rebase
#6582 Part 9/9: Warp variants → experimental (draft; merges last)		—	pending rebase
#6324 [DO-NOT-MERGE] Lumped validation reference		ALL	—

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.

• 4 manager tasks now mirror their Direct counterparts: Isaac-Reorient-Cube-Allegro,
  -Shadow, -Shadow-OpenAI-FF, -Shadow-OpenAI-LSTM.
• −380 lines of configuration: the shared base and its unconstructed sub-configs are gone.
• 4 commits, 30 files, +1378 / −554.

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_rate is left as upstream defines it. The command term keeps the per-attempt
accounting 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 ManagerBasedRLEnvCfg
directly 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 task
constructed, and reorient_common. Its constants are declared where they are used; the in-hand
offset 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 ShadowHandEventCfg
declares all six terms for every backend, and ShadowHandManagerEventCfg adds only the
manager-specific reset_state.

Allegro keeps an enable_domain_randomization flag, default True. It is read in __post_init__,
so it is a configuration-file switch — env.enable_domain_randomization=false on the command line
has no effect. Individual terms remain overridable, for example env.events.robot_scale_mass=null.

Validation

8 arms × 1500 iterations, seed 42, physics=newton_mjwarp. All four manager tasks train to
convergence; the Direct references reach success rates of 0.838 (Allegro), 0.929 (Shadow), 0.706
(OpenAI-FF) and 0.615 (OpenAI-LSTM).

Manager and Direct success rates are not printed side by side: the manager command term reports
Metrics/success_rate as a per-attempt ratio and the Direct environments report a per-episode
bit, so the two columns would not measure the same quantity.

A separate arm with domain randomization disabled on Isaac-Reorient-Cube-Shadow reaches 97% of
Direct's goals-per-step against 81% with it enabled.

[greptile-apps]

Greptile Summary

This 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.

• Success-rate fix: success_bonus is promoted to a ManagerTermBase that accumulates goals_reached per episode and logs Metrics/success_rate as a per-episode bit in reset(), matching the Direct definition. The previous per-attempt metric (which could not reach 1.0) is removed.
• New MDP terms: reorient_timeout (success-extended timer for the OpenAI variant), reset_reorient_state (Direct-matching reset distribution), NoisyEMAJointPositionToLimitsAction, fingertip_* observation helpers, and openai_policy_observation (stateful noise with shape-probe guard) round out the manager feature set.
• Config hierarchy flattened: all three environment configs derive from ManagerBasedRLEnvCfg directly; ReorientObjectEnvCfg and reorient_common are deleted; timing and physics are declared as class fields alongside a value-parity pytest that guards Direct/manager drift on timing, thresholds, and streak caps.

Confidence Score: 5/5

The 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

Filename	Overview
source/isaaclab_tasks/isaaclab_tasks/core/reorient/mdp/commands.py	Refactored command term: pre-allocated _command_buf replaces per-call torch.cat, success rate moved to success_bonus reward term, _skip_success_update logic added to suppress spurious resample on autoreset.
source/isaaclab_tasks/isaaclab_tasks/core/reorient/mdp/rewards.py	success_bonus converted to stateful ManagerTermBase that accumulates goals_reached and logs Metrics/success_rate in reset(). Partial-reset logging bias was flagged in previous review; no new issues beyond that.
source/isaaclab_tasks/isaaclab_tasks/core/reorient/mdp/terminations.py	Adds reorient_timeout class that maintains a per-env timer restarted on each goal reach. Uses _steps_since_success >= max_episode_length - 1, which terminates 1 step earlier than standard time_out.
source/isaaclab_tasks/isaaclab_tasks/core/reorient/mdp/observations.py	Adds fingertip_pos/quat/vel helpers, fingertip_wrench ManagerTermBase with zero-fallback, reorient_last_action for autoreset-compatible last-action observation, and openai_policy_observation with stateful noise.
source/isaaclab_tasks/isaaclab_tasks/core/reorient/config/allegro_hand/allegro_hand_manager_env_cfg.py	Full rewrite of Allegro manager config. enable_domain_randomization defaults to True despite PR description claiming False; docstring erroneously states Hydra CLI overrides have no effect.
source/isaaclab_tasks/isaaclab_tasks/core/reorient/config/shadow_hand/shadow_hand_manager_env_cfg.py	New 241-line Shadow Hand state manager config. Clean direct derivation from ManagerBasedRLEnvCfg with backend-preset events. No issues found.
source/isaaclab_tasks/isaaclab_tasks/core/reorient/config/shadow_hand/shadow_hand_openai_manager_env_cfg.py	New OpenAI FF/LSTM Shadow manager config with asymmetric observations, stateful action noise, and reorient_timeout.
source/isaaclab_tasks/isaaclab_tasks/core/reorient/mdp/init.pyi	Updated stub exports; ReorientCommand and ReorientCommandCfg are listed twice in all, which is a copy-paste artefact.
source/isaaclab_tasks/test/core/test_reorient_value_parity.py	New pytest module validating timing, success threshold, fall distance, streak cap, and success_count_threshold parity. Does not cover timeout termination logic.
source/isaaclab_tasks/isaaclab_tasks/core/reorient/mdp/events.py	New reset_reorient_state event matching Direct reset distributions; saves terminal raw action and step counter for reorient_last_action autoreset handling.
source/isaaclab_tasks/isaaclab_tasks/core/reorient/mdp/noisy_actions.py	New NoisyEMAJointPositionToLimitsAction applying stateful noise before EMA processing. Reset propagates to noise model correctly.

Sequence Diagram

sequenceDiagram
    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

Loading

_{Reviews (3): Last reviewed commit: "Keep Allegro manager domain randomizatio..." | Re-trigger Greptile}

[greptile-apps]

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.

[greptile-apps]

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.

[greptile-apps]

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.

Suggested change

	@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!

[AntoineRichard]

[AI Review]

Requesting changes for two merge blockers: this removes public APIs without the required prior deprecation, and the lazy-export stub advertises camera symbols that do not exist. Inline comments also cover the unenforced Direct/manager parity, source-comment hygiene, and duplicated quaternion construction.

Verification: ./isaaclab.sh -f passed; the added math tests passed (8 tests); Reorient config-loading tests passed (18 tests); a runtime probe of mdp.ShadowHandCameraFeatures failed with the expected AttributeError.

[AntoineRichard]

[AI Review] — Important

These symbols are public today (success_bonus, track_*, and the termination terms are exported from mdp.__all__), and ReorientObjectEnvCfg is an importable configuration. Removing them in this PR violates the repository rule that breaking changes require a prior deprecation release. The existing Allegro task ID also changes contract in place, making released checkpoints incompatible.

Please retain the old configuration/task behavior and exported terms with deprecation warnings for at least one release, introduce the replacement under new names/IDs, and record this under Deprecated rather than Removed.

[AntoineRichard]

[AI Review] — Important

This export is broken: ShadowHandCameraFeatures, shadow_hand_camera_cached_features, and shadow_hand_goal_keypoints are imported from observations.py, but that module defines none of them. A runtime access to mdp.ShadowHandCameraFeatures raises AttributeError, and from ...mdp import * consequently fails.

Please remove these three stale exports (and the orphaned camera-section header in observations.py) until the implementation lands, or add the missing implementation. A small test that resolves every explicit symbol in this stub would catch this regression.

[AntoineRichard]

[AI Review] — Moderate

There is no value-parity test in this PR or the existing Reorient tests, so this comment makes a false maintenance guarantee. The simulation, reset, reward, and observation contracts are duplicated across the Direct and manager configurations; future edits can silently break the parity promised by this PR.

Please centralize these tunables in a private shared parameter bundle and add an explicit Direct/manager contract test. Once enforced, keep only a concise functional comment.

[AntoineRichard]

[AI Review] — Moderate

This comment narrates the backend investigation, failure mode, CLI overrides, and benchmark history merely to justify one default. That is review/validation context rather than a durable source-code contract.

Please reduce it to the functional constraint, for example: # Isaac RTX requires Fabric cloning, so the camera task defaults to PhysX. Keep the benchmark and alternative-command details in the PR description or user documentation.

[AntoineRichard]

[AI Review] — Moderate

random_xy_rotation and randomize_rotation below implement the same X/Y quaternion composition. The Direct path uses one and the manager reset path uses the other, creating two sources of truth for quaternion order; randomize_rotation also lacks the specific type annotations required for new interfaces.

Please extract one typed composition helper and reuse it from both sampling paths, with a known-input regression test.

[greptile-apps]

DR enabled by default contradicts PR description and class contract

enable_domain_randomization is True out of the box, but the PR description states it is "disabled by default" and the changelog repeats this. More importantly, the class docstring says "Direct-compatible semantics" and explicitly notes the Direct Allegro task has no randomization — so a default-constructed AllegroCubeEnvCfg() will silently apply physics-material, mass, and stiffness perturbations that the Direct reference task never applies. Anyone comparing the two workflows without overriding this flag will see a training-curve difference that is entirely DR, not workflow.

Set the default to False so the no-DR path is the safe default and DR must be deliberately opted in.

[hujc7]

@greptileai review

[StafaH]

Why didi we remove the manager base class?

[hujc7]

The base class was mostly used for allegro task and inheriting into the shadow task causes a lot of overrides, so I decided to make the base class for allegro and shadow but not shared.

[StafaH]

Move to contrib

[StafaH]

Odd naming, diverges from rest of isaaclab

[hujc7]

updated

[StafaH]

I don't see a difference between this and the newton one, why do we need preset cfg?

[StafaH]

Im not a fan of the openai naming scheme. The MDP configs should keep very generic names like the rest of isaaclab.

For openai, keep a comment at the top, link to the paper and give them a reference and acknowledgement. We don't need to use the name everywhere.

[hujc7]

renamed.

[StafaH]

Strip away these stray comments. The code itself is readable and doesnt need a commen title.

[hujc7]

removed

[StafaH]

We shouldn't use getattr inside the MDP like this. Either this needs to exist (so we can write a test), or it doesn't and we need to modify code elsewhere to ensure there is a reset_buf? Is it intended for it to be optional like this? When would it be None?

[hujc7]

it was not available at init and only get assigned during step. Created a reset buffer at init time to avoid it.

[StafaH]

Stray coment

[StafaH]

Some of these MDP functions/terms look very generic. Is there nothing in the global MDP for this? Maybe we need to promote this to the global MDP so multiple tasks can take advantage?

[hujc7]

With the SR setup, I think some mdps are quite specific. I don't have a clear picture yet how to make it cleaner.

[StafaH]

I think this base class was good (and matched the convenetion across all isaaclab)

[hujc7]

It's kind of combined into the allegro one.