Repository navigation
[Task Clean-up][Manager] Dexterous Part 5/10: Add the reorientation manager counterparts - #6418
Merged
hujc7 merged 36 commits intoAug 11, 2026
Merged
Conversation
Contributor
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+100
to
+131
| 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 |
Contributor
There was a problem hiding this comment.
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.
Comment on lines
+226
to
+235
| 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 |
Contributor
There was a problem hiding this comment.
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.
Comment on lines
+103
to
+108
|
|
||
|
|
||
| @configclass | ||
| class ObservationsCfg: | ||
| """Full 124-dimensional state observation in Direct order.""" | ||
|
|
Contributor
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.
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!
This was referenced Jul 14, 2026
Merged
[Task Clean-up] Dexterous Part 3/9: Add success-rate metrics to the reorientation Direct tasks
#6413
Merged
Closed
hujc7
force-pushed
the
jichuanh/task-cleanup-dex-part08
branch
from
July 17, 2026 00:56
3b4f0cc to
d4d1290
Compare
hujc7
changed the title
hujc7
changed the title
hujc7
added a commit
that referenced
this pull request
Jul 17, 2026
…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
hujc7
changed the title
6 of 8 tasks
hujc7
force-pushed
the
jichuanh/task-cleanup-dex-part08
branch
from
July 18, 2026 11:10
f8b3611 to
c5195e8
Compare
hujc7
requested review from
AntoineRichard,
ClemensSchwarke,
Mayankm96,
StafaH,
kellyguo11,
ooctipus and
pascal-roth
as code owners
July 18, 2026 11:10
…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.
…eanup-dex-part08 # Conflicts: # docs/source/_static/css/environment-browser.js # docs/source/overview/environments.rst
The contributed OpenAI Shadow Hand task had been collapsed into a single identifier that exposed the recurrent policy through an "rsl_rl_lstm_cfg_entry_point" keyword argument. Nothing reads that key: the training entry point selects an agent configuration by workflow, not by name, so the recurrent policy could not be reached at all. Register the two identifiers the released tasks used instead, each pointing at its own runner configuration, and repoint the benchmarking configuration and the environment catalog that still named the retired core identifiers.
"Metrics/success_rate" named two different quantities: the manager-based tasks reported goals reached over goals presented, while the Direct tasks reported a per-episode success bit drawn at a configured goal count. The two curves were therefore not comparable even though they shared a name and measured the same task. Report the per-attempt rate from both, and drop the success count threshold that only the retired bit consumed. Move the goal accounting into a SuccessTracker beside EpisodeErrorRecorder so the command term keeps one member instead of open-coded buffers. The tracker also suppresses success detection for the single step after an automatic reset, where the goal has been drawn but no physics has run against it yet, so a reset pose landing inside the success threshold no longer banks a goal the policy never earned.
Inline the two noise configurations that the OpenAI task alone consumes, and remove scene entity arguments that only restate their defaults.
The manager reset samples orientations with the framework's reset_root_state_with_random_orientation, which left random_xy_rotation without callers. The Direct and hand-over tasks compose their rotations with randomize_rotation directly, so one composition helper remains rather than two. Also name the contributed OpenAI tasks by their current identifiers in the stage-creation guide, which still listed the retired core ones.
The shared configuration classes were named generically enough to collide with identically named classes elsewhere in the repository -- SceneCfg, EventCfg and ObservationsCfg are each defined in dozens of files -- so a reader could not tell which base a task was specializing. Apply one convention. Classes shared across files carry the task prefix, and the Base suffix marks only those a subclass must complete by supplying fields the parent leaves unset; the observation halves are assembled rather than completed, so they are named as parts. Classes nested inside another configuration drop the prefix, since the enclosing class already supplies it. Registered and preset-selectable configurations always name their task. Environment configurations that a task registers or a preset selects now hold fields only, with their helper classes declared beside them, so the configuration reads as the list of what it configures. Also name the event configurations after what they hold, rather than the generic EventCfg, and drop the redundant robot prefix where an enclosing class supplies it.
The Shadow Hand goldens were regenerated when the in-hand cube became one rigid body per hand, but the Isaac RTX colour pair was missed while its albedo, normals, segmentation and shading siblings were updated. Those two still showed the previous Newton cube, so the colour comparison failed while every other modality passed.
The shared in-hand cube changes the Newton shadow-hand render, so the newton-isaacsim_rtx rgb and rgba goldens no longer match. The previous refresh regenerated them on a workstation GPU, which renders the same scene differently from the CI runner: CI measured 8.80% differing pixels against those images, worse than the 6.25% it measured against the originals, both over the 5% gate. Take the images from CI's own rendering-correctness comparison-images artifact instead, so the goldens encode the renderer that grades them.
AntoineRichard
approved these changes
Aug 11, 2026
Collaborator
There was a problem hiding this comment.
Do we really need this test?
…eanup-dex-part08 Resolved the six shadow_hand Newton golden conflicts in favour of develop. Develop refreshed 99 goldens repo-wide in isaac-sim#6932 (synchronous texture streaming for the IsaacSim RTX renderer); those images carry the current renderer, so they are the correct baseline to carry forward.
The test asserted that two configuration classes hold equal literals for timing, success tolerance, fall distance and the streak cap. That checks how the configs are wired rather than any behaviour a user can observe, and no other task in the repository carries an equivalent test. Remove it, along with the changelog entry that announced it and the parenthetical the three Direct cfgs used to cite it; those comments keep stating that the manager cfg mirrors the values.
The shared in-hand cube changes the rendered Shadow Hand scene, so these goldens do need to move. The earlier refresh generated them on a workstation GPU, whose renderer differs from the CI runner's by more than the comparison gate allows, and develop has since refreshed 99 goldens of its own for the synchronous texture streaming change in isaac-sim#6932. Reset every Shadow Hand golden to develop's current image so the comparison starts from the renderer CI actually runs, and let CI report which ones the cube change moves.
The shared cube spawns as a rigid body at the Direct PhysX scale and drop height rather than Newton's smaller articulated variant, so the Newton IsaacSim RTX views of the Shadow Hand scene move. The semantic segmentation view moves too, which is geometry rather than shading. Take all eight images from CI's rendering-correctness comparison-images artifact so they encode the renderer that grades them. The PhysX views stay on develop's goldens: CI measured them within tolerance.
Moving the module under the reorientation package left the hand-over task importing across into a sibling task's namespace for helpers it shares, and removed a public module without a deprecation. Move it back and restore the test's name. The module keeps SuccessTracker, so the diff against develop is now additive rather than a relocation.
The kitless job grades the Newton and OVRTX views of the same scene from their own golden set, so the cube change moves them as well. Semantic segmentation moves too, which is geometry rather than shading. Take the nine images from the kitless job's comparison-images artifact, matching how the IsaacSim RTX goldens were updated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review Map
Summary
Adds manager-based counterparts for the Shadow cube reorientation task and aligns the existing
Allegro manager task with the Direct contracts.
Isaac-Reorient-Cube-Shadow,-Shadow-Cameraand-Allegro.training regime, so they get no manager counterpart.
Metrics/success_ratenow means the same thing in both workflows.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 and the fall distance, so drift on either side fails CI.
The Allegro observation space changes size, so existing manager checkpoints must be retrained.
Domain randomization becomes opt-in through
presets=randomized, replacing a configuration-fileflag that the command line could not override.
2. OpenAI variants move to the contributed tasks
Isaac-Reorient-Cube-Shadow-OpenAI-{FF,LSTM}-DirectbecomeIsaacContrib-Reorient-Cube-Shadow-OpenAI-{FF,LSTM}-Direct. Both identifiers are kept: they shareone environment configuration and differ only in their agent configuration, so collapsing them
would leave the recurrent policy unreachable — the training entry point selects an agent
configuration by workflow, not by name.
3. One success rate, defined once
Metrics/success_ratenamed two quantities. The manager tasks reported goals reached over goalspresented; the Direct tasks reported a per-episode success bit drawn at
success_count_threshold.The curves were not comparable despite sharing a name and measuring the same task.
Both now report the per-attempt rate, and
success_count_thresholdis removed as unused. The goalaccounting moves into a
SuccessTrackerbesideEpisodeErrorRecorder, which also suppressessuccess detection for the single step after an automatic reset — where the goal has been drawn but
no physics has run against it, so a reset pose inside the success threshold would otherwise bank a
goal the policy never earned.
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. The
mass_propsdensity overrides are dropped: the asset authors an absolutephysics:massof 0.216 kg, which takes precedence over density, so they never applied on either backend.
5. Configuration hierarchy flattened, classes named by role
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. The environment configurations now derive from
ManagerBasedRLEnvCfgdirectly. Removed:ReorientObjectEnvCfg, the shared sub-configurations notask constructed, and
reorient_common.isaaclab_tasks.core.utilsmoves toisaaclab_tasks.core.reorient.utils, which the hand-over task already imports from.One naming convention now applies throughout, since
SceneCfg,EventCfgandObservationsCfgare each defined in dozens of files and gave no clue which base a taskwas specializing. Classes shared across files carry the task prefix; the
Basesuffixmarks only those a subclass must complete by supplying fields the parent leaves unset, so
the observation halves are named as parts rather than templates. Classes nested inside
another configuration drop the prefix. Registered and preset-selectable configurations
always name their task and hold fields only, with their helpers declared beside them.
Validation
Eight arms, 1500 iterations, 8192 environments,
physics=newton_mjwarp, one L40 each. All exitedcleanly with no regression in reward.
Isaac-Reorient-Cube-Shadow-Shadowpresets=asymmetric-Shadow-Direct-Allegro-Allegropresets=randomized-Allegro-DirectIsaacContrib-…-OpenAI-FF-DirectIsaacContrib-…-OpenAI-LSTM-DirectThe LSTM arm resolves
ShadowHandAsymLSTMPPORunnerCfg, confirming the recurrent policy is reachablethrough its own identifier.
Known failure
test_rendering_shadow_hand[newton-isaacsim_rtx-rgb]fails; it passes ondevelop. Only thergbmodality is affected — depth, normals and all three segmentations pass on the same backend, so the
cube move is not an obvious explanation. Root cause is not yet established.