[Task Clean-up][Benchmark] Dexterous Part 7/8: Add success-rate support to the benchmark utilities#6415
[Task Clean-up][Benchmark] Dexterous Part 7/8: Add success-rate support to the benchmark utilities#6415hujc7 wants to merge 57 commits into
Conversation
| def _extract_sustained_feature(log_data, feature, uses_lower_threshold, consecutive_samples): | ||
| """Extract the best threshold-facing value sustained over a sample window.""" | ||
| values = np.asarray(log_data[feature], dtype=float)[:, 1] | ||
| if len(values) < consecutive_samples: | ||
| return None | ||
| if not np.all(np.isfinite(values)): | ||
| return math.nan |
There was a problem hiding this comment.
Global NaN check rejects series with any early non-finite value
np.all(np.isfinite(values)) is applied to the full historical series before any windowing, so a single NaN anywhere — including in early warm-up steps before the metric stabilises — causes the function to return math.nan and fail the threshold even when a valid consecutive_samples-length window exists entirely within the finite portion of the data. For success_rate in particular, early episodes may not record any successes and could produce NaN values before converging; those early samples would silently disqualify an otherwise passing run. Restricting the finiteness check to each candidate window (or filtering non-finite entries before slicing) would make the "best sustained window" semantics consistent with the function's name.
| @@ -230,7 +271,7 @@ def _extract_log_val(name, log_data, uses_lower_threshold, workflow): | |||
| "skrl": "Reward / Total reward (mean)", | |||
| } | |||
| tag = reward_tags.get(workflow) | |||
| if tag: | |||
| if tag and consecutive_samples is None: | |||
| return _extract_reward(log_data, tag) | |||
|
|
|||
| elif name == "episode_length": | |||
| @@ -241,8 +282,20 @@ def _extract_log_val(name, log_data, uses_lower_threshold, workflow): | |||
| "skrl": "Episode / Total timesteps (mean)", | |||
| } | |||
| tag = episode_tags.get(workflow) | |||
| if tag: | |||
| return _extract_feature(log_data, tag, uses_lower_threshold) | |||
| elif name == "success_rate": | |||
| success_rate_tags = { | |||
| "rl_games": "Episode/Metrics/success_rate", | |||
| "rsl_rl": "Metrics/success_rate", | |||
| "skrl": "Metrics/success_rate", | |||
| } | |||
| tag = success_rate_tags.get(workflow) | |||
|
|
|||
| if tag: | |||
| if consecutive_samples is not None: | |||
| return _extract_sustained_feature(log_data, tag, uses_lower_threshold, consecutive_samples) | |||
| return _extract_feature(log_data, tag, uses_lower_threshold) | |||
There was a problem hiding this comment.
reward bypasses _extract_reward when consecutive_samples is set
When name == "reward" and consecutive_samples is not None, the early return _extract_reward(...) branch is skipped (lines 274–275) and the code falls through to the generic if tag: block, where _extract_sustained_feature is called instead. _extract_reward uses an "average of the top-k" aggregation that is quite different from the sliding-window min/max in _extract_sustained_feature. Any future YAML config that specifies a structured threshold for the reward metric would silently receive a semantically different aggregation than the scalar-threshold path. There are no tests covering this combined path, and the behaviour change is not documented.
AntoineRichard
left a comment
There was a problem hiding this comment.
I think we should store the success rate / metrics / expectations somewhere else.
AntoineRichard
left a comment
There was a problem hiding this comment.
I think we should discuss how to go about this. It's not clear if we should do any training tests on our regular CI. If we make such changes to this API they should not belong in that PR series. I would rather discuss this first.
There was a problem hiding this comment.
This is also hiding major changes under an incorrect PR scope
| def _is_training_task(task_id: str) -> bool: | ||
| """Return whether a registered task is intended for training benchmarks.""" | ||
| stem, separator, version = task_id.rpartition("-v") | ||
| if separator and version.isdigit(): | ||
| task_id = stem | ||
| return not {"Play", "Benchmark"}.intersection(task_id.split("-")) | ||
|
|
||
|
|
There was a problem hiding this comment.
This is a major change.
Add a behavioral Metrics/success_rate signal (goal-reach streaks per episode) and threshold-independent episode orientation-error diagnostics to the Direct reorientation environments, with the shared helpers in isaaclab_tasks.core.utils and torch math tests. The task logic is torch-first per the mainline convention; success gates task health while reward stays diagnostic. Also fix hand resets that could initialize joints below their lower position limits.
Add the RSL-RL runner configuration for the Shadow handover Direct task and success-rate metrics on the torch-first path, fix handover construction on Newton (renamed distal joints), and land the camera Direct renderer presets with configuration validation. RSL-RL observations now read from the public environment-owned obs_buf on all Direct env bases (reset stores the buffer like step), replacing the adapter-side private hook.
Add manager-based training environments for the Allegro and Shadow cube reorientation tasks (state and OpenAI FF/LSTM variants). The manager cfgs share the Direct tasks' scalar parameters through per-family task constants, and the shared MDP terms reuse the Direct success evaluation so both variants report the same boolean success metric.
Add manager-based training environments for the Shadow handover and Shadow camera reorientation tasks, completing manager coverage of the dexterous families. Direct-vs-manager scalar parity across all families is enforced by a new value-parity test.
Report the success-rate metric through the environment training benchmark utilities, exclude inference-only camera benchmark registrations from training-benchmark discovery, regenerate the environment overview table, and cover the install-order constants with a unit test.
381c55b to
df070af
Compare
Part 4 share of the lump readability round (cdaadac): the camera Direct files carry the workflow marker in their names and read the camera geometry from reorient_common; handover_task_base is renamed handover_common and the handover files take the Shadow identity from shadow_hand_common. Sim settings are declared inline per file.
Rebuilt from the reviewed lump so the handover and camera manager counterparts land with the review rounds folded in: inline section values per file, identity from the common modules, camera manager overriding only the fields that differ, and no sim mixins. Lump review commits folded: 792e400, 42675b6, 1f05f85, 173e9dc, 2727616, 1732493, 3659a33, ee272c9, cdaadac.
# Conflicts: # source/isaaclab_tasks/isaaclab_tasks/core/handover/handover_manager_env_cfg.py # source/isaaclab_tasks/isaaclab_tasks/core/handover/mdp/commands.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_camera_manager_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/reorient_task_base.py # source/isaaclab_tasks/test/core/test_dexterous_value_parity.py
…eanup-dex-part05 # Conflicts: # docs/source/overview/environments.rst
The shadow_hand_env_cfg -> shadow_hand_direct_env_cfg rename landed at this layer while two consumers kept importing the removed module name, so the part tree no longer stood alone (caught by arm-ci kitless rendering collection). Pin the camera cfg import to shadow_hand_direct_env_cfg and the handover robot cfg import to shadow_hand_common, matching the final tree.
The shadow_hand_env_cfg -> shadow_hand_direct_env_cfg rename landed at this layer while two consumers kept importing the removed module name, so the part tree no longer stood alone (caught by arm-ci kitless rendering collection). Pin the camera cfg import to shadow_hand_direct_env_cfg and the handover robot cfg import to shadow_hand_common, matching the final tree.
…eanup-dex-part04 # Conflicts: # source/isaaclab_tasks/isaaclab_tasks/core/handover/handover_env_cfg.py # source/isaaclab_tasks/isaaclab_tasks/core/reorient/config/shadow_hand/shadow_hand_camera_env_cfg.py
Pin the shadow-hand rendering test helper to the final tree state: the camera cfg refactor nested _ShadowHandBaseTiledCameraCfg as ShadowHandTiledCameraCfg.BaseTiledCameraCfg and the helper kept the removed top-level name, failing shadow-hand rendering CI at import.
The rename round updated the camera gym-registration entry-point strings to shadow_hand_direct_camera_env* at this layer, but the camera modules here still carry their pre-rename names, so every registry-driven cfg load failed at import (isaaclab_tasks suites, forbidden-imports test, registered-tasks rendering). Point the registrations back at the modules that exist at this layer.
The lower layer's registration revert merged forward and un-renamed the camera entry points here, where the camera modules do carry the renamed filenames. Pin the registration block back to the final state.
…eanup-dex-part11 # Conflicts: # source/isaaclab_tasks/isaaclab_tasks/core/reorient/config/shadow_hand/__init__.py
…isualizer teardown (#6411) ## 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 can go stale after a branch update — the table on #6324 is refreshed first. | PR | Status | Depends on | Exact changes | |---|---|---|---| | #6410 [Docs] Environment overview regen |  | — | — | | 📌 #6411 Part 1/8: Newton cloner/cubric/visualizer fixes (this PR) |  | — | — | | #6412 Part 2/8: OVPhysX articulation + manager runtime |  | — | — | | #6413 Part 3/8: Reorient Direct, torch |  | — | — | | #6414 Part 4/8: MARL-to-single-agent fix + handover/camera Direct |  | #6413 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6414/changes/6e8a63e4e028b2d43676ea30c446b9dc9068c7b5..5cb00e7cb5cc813b202521272e043007cd255194) | | #6418 Part 5/8: Reorient manager counterparts |  | #6413 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6418/changes/79f87501ac4c81de93a71dab00dc443da62113aa..e7c9a9a3fae3a7972b0c5165ae683abffb7d0e0f) | | #6421 Part 6/8: Handover + camera manager counterparts |  | #6413, #6414, #6418 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6421/changes/01c9f4d8c5c35a5688b2a5bb90209e16b8f81b99..835a5815ec49b11aada1d20a76c177054505e6e7) | | #6415 Part 7/8: Benchmark success-rate utilities + docs |  | #6413, #6414, #6418, #6421 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6415/changes/e1abb6b1790ccc57af42551eebccf743633f1f13..d6348539aa8032d9668c20a8fea462c5d88d3af9) | | #6582 Part 8/8: Warp variants → experimental (draft; merges last) |  | #6413 | [changes](https://github.com/isaac-sim/IsaacLab/pull/6582/changes/21dbb1769c4e30c8e9e5b0f563c2dae24c230349..83e1587cadd9712a60615ed2a3cb2d177c2ac24d) | | #6324 [DO-NOT-MERGE] Lumped validation reference |  | ALL | — | ## Summary - Fixes Newton cloner label rows, the cubric IAdapter version audit (exact-match fallback to the CPU hierarchy path), and visualizer teardown. - Retains an in-tree `ignore_paths` workaround for custom-frequency USD traversal; it becomes redundant once the Newton pin advance (#6584) merges — this PR then only needs a rebase. ## Stacking - Independent; based on `develop`. ## Review history - Approved. The Newton pin + MuJoCo overrides were split out to #6584 via revert commits (2026-07-17) so this PR's CI runs against develop's pins.
Review Map
Summary
Stacking
Validation