[Task Clean-up][Benchmark] Dexterous Part 8/9: Add success-rate support to the benchmark utilities

[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 can go stale after a branch update — the table on [DO-NOT-MERGE][Task Clean-up] Dexterous: lumped validation branch (series reference) #6324 is refreshed first.

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		—	changes
#6414 Part 4/9: MARL-to-single-agent fix + handover/camera Direct		#6413	changes
#6418 Part 5/9: Reorient manager counterparts		#6413	changes
#6421 Part 6/9: Handover + camera manager counterparts		#6413, #6414, #6418	changes
#6410 Part 7/9: Environment overview docs		#6421	changes
📌 #6415 Part 8/9: Benchmark success-rate utilities (this PR)		#6421	changes
#6582 Part 9/9: Warp variants → experimental (draft; merges last)		#6413	changes
#6324 [DO-NOT-MERGE] Lumped validation reference		ALL	—

Summary

• Reports the success-rate metric through the environment training benchmark utilities and excludes inference-only camera benchmark registrations from training-benchmark discovery (with unit tests for the discovery helpers).
• Regenerates the environment overview table and covers the install-order constants with a unit test.

Stacking

• Last mainline stack PR; based on [Task Clean-up][Manager] Dexterous Part 6/9: Add the handover and camera manager counterparts #6421's branch (own diff: Exact-changes link above).

Validation

• Kit-free suite green (72/72). Full evidence: [DO-NOT-MERGE][Task Clean-up] Dexterous: lumped validation branch (series reference) #6324.

[greptile-apps]

Greptile Summary

This PR extends the training benchmark utilities to record Metrics/success_rate alongside reward and episode length, using a new sustained-window threshold type (consecutive_samples) that requires N consecutive samples to meet a criterion. It also replaces the narrow endswith(\"Play-v0\") task filter with _is_training_task, which correctly excludes all Play and Benchmark variants.

• env_benchmark_test_utils.py: adds _is_training_task, _parse_threshold_spec, _extract_sustained_feature, and wires success_rate into _extract_log_val; evaluate_job now populates the KPI payload even for missing/non-finite metrics instead of silently continuing.
• test_env_benchmark_test_utils.py: new unit-test file with 9 parametrised scenarios covering the filter, scalar-threshold backward-compatibility, sustained-pass/fail paths, and unsupported-workflow fallback.
• test_environments_training.py: single-line swap to the new _is_training_task helper.

Confidence Score: 4/5

Safe to merge; the changes are additive and record-only for tasks that do not emit success_rate, with no impact on existing reward/episode_length benchmarks

The global non-finite check in _extract_sustained_feature could cause a valid training run to fail its benchmark if any NaN appears in the early success_rate history before the metric stabilises, and the undocumented aggregation switch for reward+consecutive_samples could mislead future config authors. Neither affects existing tasks today but they are latent correctness traps in new code paths.

env_benchmark_test_utils.py — specifically _extract_sustained_feature (global NaN check) and the _extract_log_val reward branch when consecutive_samples is set

Important Files Changed

Filename	Overview
source/isaaclab_tasks/test/benchmarking/env_benchmark_test_utils.py	Core utility extended to add success_rate metric support and sustained-window evaluation via _extract_sustained_feature and _parse_threshold_spec; two subtle design decisions worth attention
source/isaaclab_tasks/test/benchmarking/test_env_benchmark_test_utils.py	New unit-test file covering success_rate evaluation paths, filter logic, and scalar-threshold backward compatibility; missing one edge-case scenario (early NaN warm-up with later valid window)
source/isaaclab_tasks/test/benchmarking/test_environments_training.py	Single-line change replaces the narrow Play-v0 suffix guard with the new _is_training_task helper for broader Benchmark/Play exclusion; change is correct and safe
source/isaaclab_tasks/changelog.d/task-cleanup-dex-part05.rst	New changelog fragment noting the training benchmark discovery fix; content is accurate

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[evaluate_job] --> B[_retrieve_logs]
    B -->|no logs| C[fail: training did not finish]
    B -->|logs found| D[iterate thresholds]
    D --> E[_parse_threshold_spec]
    E -->|plain number| F[consecutive_samples = None]
    E -->|dict| G[consecutive_samples = N]
    F --> H{threshold_name}
    G --> H
    H -->|duration| I[val = duration arg]
    H -->|reward or episode_length or success_rate| J[_extract_log_val]
    J -->|reward, no consecutive| K[_extract_reward avg top-k]
    J -->|other, no consecutive| L[_extract_feature max or min]
    J -->|any metric, with consecutive| M[_extract_sustained_feature]
    M -->|len less than N| N[return None]
    M -->|any non-finite globally| O[return NaN]
    M -->|all finite| P[best window extremum]
    K --> Q{val None or non-finite?}
    L --> Q
    P --> Q
    N --> Q
    O --> Q
    I --> Q
    Q -->|yes| R[payload = None, success = False]
    Q -->|no| S{exceeds threshold?}
    S -->|yes| T[success = False with msg]
    S -->|no| U[payload = val, success unchanged]

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"}}}%%
flowchart TD
    A[evaluate_job] --> B[_retrieve_logs]
    B -->|no logs| C[fail: training did not finish]
    B -->|logs found| D[iterate thresholds]
    D --> E[_parse_threshold_spec]
    E -->|plain number| F[consecutive_samples = None]
    E -->|dict| G[consecutive_samples = N]
    F --> H{threshold_name}
    G --> H
    H -->|duration| I[val = duration arg]
    H -->|reward or episode_length or success_rate| J[_extract_log_val]
    J -->|reward, no consecutive| K[_extract_reward avg top-k]
    J -->|other, no consecutive| L[_extract_feature max or min]
    J -->|any metric, with consecutive| M[_extract_sustained_feature]
    M -->|len less than N| N[return None]
    M -->|any non-finite globally| O[return NaN]
    M -->|all finite| P[best window extremum]
    K --> Q{val None or non-finite?}
    L --> Q
    P --> Q
    N --> Q
    O --> Q
    I --> Q
    Q -->|yes| R[payload = None, success = False]
    Q -->|no| S{exceeds threshold?}
    S -->|yes| T[success = False with msg]
    S -->|no| U[payload = val, success unchanged]

Loading

_{Reviews (1): Last reviewed commit: "Add success-rate support to the benchmar..." | Re-trigger Greptile}

[greptile-apps]

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.

[AntoineRichard]

[AI Review] Confirmed on the current head: [NaN] + [0.3] * 20 returns NaN even though the suffix contains a valid contiguous 20-sample window. Please evaluate finiteness per candidate window without filtering across gaps, and add this exact regression case.

[greptile-apps]

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]

[AI Review] Confirmed on the current head: values 1 through 10 produce 6.5 with the scalar reward path and 8.0 with consecutive_samples=3. Because structured specs are accepted generically, this is a silent aggregation-contract switch. Restrict structured specs to success_rate, or define and test an explicit reducer for every supported metric.

[AntoineRichard]

I think we should store the success rate / metrics / expectations somewhere else.

[AntoineRichard]

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.

[AntoineRichard]

This is also hiding major changes under an incorrect PR scope

[AntoineRichard]

[AI Review] Confirmed at the current head. The exact b50ac890...f21a2124 contribution includes a camera physics-default change, rendering coverage, install-order coverage, manager/direct parity coverage, and a manager changelog fragment in this benchmark PR. Please move each item to its owning stack layer so the benchmark behavior can be reviewed and reverted independently.

[AntoineRichard]

This is a major change.

[AntoineRichard]

[AI Review] Agreed: this expands discovery from one Play-v0 suffix to every task containing a Play or Benchmark token, across versions. The unit test covers synthetic IDs only and does not prove that every excluded registered task is inference-only. Please split this into the separately discussed discovery-contract change and validate it against the actual registered task set.

[AntoineRichard]

[AI Review] Reviewed the exact stacked contribution from b50ac890 through f21a2124 (10 files). The inline comments identify correctness, scope, comment-quality, duplication, and repository-hygiene issues that should be addressed before merge. Local verification: 76 focused tests passed and ./isaaclab.sh -f passed; the GPU/Kit rendering test was not run. The PR is currently conflicting with its base.

[AntoineRichard]

[AI Review] Important — the manager camera default remains incompatible with RTX. The final fix changes only ShadowHandCameraEnvCfg, while this scene still resolves to newton_mjwarp (clone_in_fabric=False) and its camera resolves to IsaacRtxRendererCfg. The newly added rendering test uses four environments, so it exercises the exact no-render-products combination described in f21a2124; retrying it as flaky cannot fix a deterministic incompatibility. Make the manager training and play camera defaults PhysX too, or select a compatible renderer, and add a config-level assertion for the resolved pair.

[AntoineRichard]

[AI Review] Important — this changes every existing benchmark's failure semantics. Previously, a missing or non-finite reward/episode-length metric was omitted; now any configured metric fails the job. That is broader than success-rate support and can turn existing workflow runs red when a tag is absent or renamed. Please either scope the new failure behavior to success_rate, or split it into a separately discussed change with coverage for every supported workflow and an explicit changelog entry.

[AntoineRichard]

[AI Review] These calibration notes are transient review history rather than durable functional documentation: evidence-calibrated, interim, tighten ... once, and provisional describe past runs and future intentions. Move that evidence to the PR or a benchmark artifact. Keep only stable metric semantics next to the configuration; the detailed rationale for why a numeric threshold was chosen should not live here.

[AntoineRichard]

[AI Review] The constraint is useful, but this seven-line diagnostic/history comment is too long for the implementation. Keep the stable functional rule only, for example: Default to PhysX because RTX tiled cameras require Fabric cloning; Newton remains available for depth-only benchmarks. Put the failure text and investigation details in the regression test or PR.

[AntoineRichard]

[AI Review] New files must use the repository's current header exactly: Copyright (c) 2022-2026, .... Please replace the single-year 2026 header.

[AntoineRichard]

[AI Review] This is a second fragment for isaaclab_tasks and describes manager work owned by an earlier stack layer. The repository requires one fragment per touched package. Move this fragment to the owning PR and keep one isaaclab_tasks fragment here. Also add a source/isaaclab/changelog.d/*.skip fragment for the test-only install change, remove the internal with unit tests wording from the user-facing entry, and document the camera default change if it remains in this PR.

[AntoineRichard]

[AI Review] This constructs all four config pairs for every parameterized family, and failures report only an opaque case N. Make the cases lazy and named, such as (field_name, direct_value, manager_value), instantiate only the selected pair, and report the field name. That removes repeated construction/lookups and makes CI failures directly actionable.

[AntoineRichard]

[AI Review] Follow-up comment-quality pass: each remaining transient calibration comment and each redundant Verify ... test docstring now has its own inline remediation. The manager interim-threshold block and camera-default comment were already anchored in the previous review.

[AntoineRichard]

[AI Review] This records run-specific calibration evidence (iter ~2300, iter ~1000, and a measured tail rate) rather than a stable functional contract. Remove it from the configuration and keep the evidence in the PR or benchmark results. The threshold fields are self-describing.

[AntoineRichard]

[AI Review] This comment explains why 500 was chosen using one observed PhysX plateau. That calibration history will become stale and is not needed to understand the configuration. Remove it and retain the supporting run evidence outside the source file.

[AntoineRichard]

[AI Review] Provisional and matching the validation campaign are process/history notes, not functional documentation. Either define the stable semantics of this gate or remove the comment and track its provisional status in the PR or follow-up issue.

[AntoineRichard]

[AI Review] This docstring only restates the descriptive test name. Remove it; the test name and parameter cases already express the behavior.

[AntoineRichard]

[AI Review] This Verify ... docstring repeats the test name without adding a contract or non-obvious constraint. Remove it and keep the test self-documenting through its name and parameter IDs.

[AntoineRichard]

[AI Review] This docstring restates the test name exactly. Remove it.

[AntoineRichard]

[AI Review] This docstring repeats the already explicit unsupported-workflow test name and assertion. Remove it.

[AntoineRichard]

[AI Review] This docstring adds no information beyond the non-finite success-rate test name and input. Remove it.

[AntoineRichard]

[AI Review] This docstring repeats the sustained-streak reset behavior already stated by the test name. Remove it.

[AntoineRichard]

[AI Review] This docstring is redundant with test_scalar_threshold_behavior_is_preserved and the assertions below it. Remove it.