Add env perf/timing config to teleop_replay_agent stats output

[rwiltz]

Description

• Embed a new top-level env_cfg block in teleop_replay_agent.py's stats JSON capturing the perf- and frame-timing-relevant env config inputs (sim.dt, sim.render_interval, decimation, episode_length_s, scene.num_envs, sim.device, sim.use_fabric, sim.render.antialiasing_mode) plus precomputed policy_dt_s / render_dt_s / renders_per_step / target_policy_hz / target_render_hz rates, so the measured numbers are self-interpreting across machines and configs without cross-referencing the env definition. Echoed in a compact Env timing: line in the stdout summary.
• Report cpu_frame_time_ms and fps (per-run and aggregate) on a per-render basis instead of per-env.step: each env.step CPU sample is divided by decimation / render_interval (renders per env.step) before stats are computed. cpu_frame_time_ms.mean now reads as the wall time between rendered frames and fps.mean reads as the render rate -- matching the simulator HUD and the rate the headset wearer / spectator actually perceives during real-time teleop. Falls back to raw per-env.step units when decimation or render_interval are unavailable. Field shapes and schema_version are unchanged (v1 unadopted; no bump needed).
• Replace narrative references to "Kit" throughout the script's docstrings and comments with neutral terms ("the simulator", "the OpenXR", "simulator-side", etc.) so the documentation reads agnostic of the underlying runtime brand. The two literal identifiers required to invoke XR mode (the --kit_args CLI flag and isaacsim.kit.xr.teleop.bridge extension name) are preserved in the usage example.
• Address PR review feedback: drop a dead env_cfg = None init whose comment misleadingly described a finally-side report (_build_report actually runs after the try/finally), and normalize None-as-"n/a" rendering across the Env timing: stdout fields.

Fixes # (issue)

Type of change

• New feature (non-breaking change which adds functionality)

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and the corresponding version in the extension's config/extension.toml file
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

PR: #5716 — Add env perf/timing config to teleop_replay_agent stats output
Commit: 74d5c3a8

---

Summary

This PR enhances teleop_replay_agent.py by embedding environment performance/timing configuration into the stats JSON output and printing a summary line to stdout. The changes are additive and non-breaking.

---

✅ Strengths

1. Well-designed defensive accessor: The _safe() helper function properly handles missing attributes and None values with appropriate type casting, preventing crashes on non-standard env_cfg subclasses.

2. Comprehensive documentation: Excellent docstring in _extract_env_perf_cfg() explaining the rationale for capturing these fields and how derived values are computed.

3. Self-documenting output: Precomputing derived values (policy_dt_s, render_dt_s, renders_per_step, target_policy_hz, target_render_hz) eliminates manual calculation for cross-machine comparisons.

4. Non-breaking schema: Maintaining schema_version=1 and allowing fields to degrade to null ensures backward compatibility.

5. Good separation of concerns: Extracting the perf config into a dedicated function (_extract_env_perf_cfg) keeps _build_report clean.

---

💬 Suggestions (Non-blocking)

1. Division safety in renders_per_step: The check render_interval not in (None, 0) is good, but consider unifying the pattern with target_policy_hz/target_render_hz for consistency:

   # Current pattern mixes "not in (None, 0)" with checking separately
   renders_per_step = (
       decimation / render_interval if decimation is not None and render_interval not in (None, 0) else None
   )

2. Type annotation opportunity: _safe() could benefit from type hints for better IDE support:

   def _safe(obj: Any, attr: str, cast: Callable | None = None) -> Any:

3. Consider antialiasing_mode stringification: Unlike other fields that use explicit casts (str, int, float), antialiasing_mode is passed without a cast. If this is an enum, consider str() wrapping for JSON serialization robustness:

   "antialiasing_mode": _safe(render, "antialiasing_mode", str),

4. Stdout formatting edge case: When sim.dt is None, the stdout output shows n/a, but other None values would print as None. Consider consistent formatting for all nullable fields.

---

📝 Notes

• CI Status: Pre-commit and initial checks passing; some jobs still pending.
• Test coverage: The PR mentions tests were added per the checklist. Since teleop_replay_agent.py is a script without a dedicated test file visible in the test directory, consider documenting how the new env_cfg extraction is validated (manual testing, integration test, etc.).
• Changelog: Properly added in changelog.d/rwiltz-replay-stats-env-cfg.rst ✓

---

Verdict

Clean, well-thought-out enhancement that improves debugging and cross-machine comparisons without breaking existing consumers. The defensive coding patterns are appropriate for handling varying env_cfg implementations.

---

_{🔍 Reviewed by Isaac Lab Review Bot | 📋 Review Guidelines}

[greptile-apps]

Greptile Summary

This PR extends teleop_replay_agent.py's stats JSON output with a new env_cfg block capturing timing-relevant env config fields (sim.dt, render_interval, decimation, etc.) plus precomputed derived rates, and changes the cpu_frame_time_ms / fps metrics from per-env.step to per-render basis so the reported numbers match the simulator HUD.

• _extract_env_perf_cfg is added to defensively read env config fields and populate the env_cfg block; _build_report signature gains an env_cfg parameter and uses the extracted config to scale per-run CPU samples before stats are computed.
• _RunStats.to_dict is updated to accept decimation / render_interval and scale active_frame_times_ms by render_interval / decimation before computing stats, falling back to raw env.step units when either value is unavailable.
• Several docstrings and comments replace "Kit" references with runtime-neutral language; the changelog file inconsistently retains "Kit" terminology.

Confidence Score: 5/5

Safe to merge; all new code paths are well-guarded and do not affect existing functionality when env config fields are absent.

The per-render scaling in _RunStats.to_dict guards against None and zero for both decimation and render_interval before dividing, and _extract_env_perf_cfg uses a _safe helper for every attribute access so non-standard env configs degrade gracefully to None. The env_cfg block is additive to the JSON output and the schema_version is not bumped. No new code paths affect env lifecycle, CloudXR, or the replay control loop itself.

The changelog fragment rwiltz-replay-stats-env-cfg.rst still uses Kit terminology inconsistent with the PR documentation goal, but this has no runtime impact.

Important Files Changed

Filename	Overview
scripts/environments/teleoperation/teleop_replay_agent.py	Adds env_cfg extraction, per-render sample scaling in _RunStats.to_dict, and stdout Env timing line; logic is sound with defensive None/zero guards throughout.
source/isaaclab_teleop/changelog.d/rwiltz-replay-stats-env-cfg.rst	New changelog fragment describing the env_cfg and per-render changes; still references Kit and Kit's HUD contrary to the PR's stated documentation goal.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[main] --> B[_prepare_env_cfg]
    B --> C[env_cfg object]
    A --> D[_run_single_replay loop]
    D --> E[_RunStats with active_frame_times_ms]
    A --> F[_build_report]
    C --> F
    E --> F

    F --> G[_extract_env_perf_cfg]
    G --> H[env_perf_cfg dict]
    H --> I[Pull decimation and render_interval]
    I --> J[_RunStats.to_dict]
    J --> K{decimation and render_interval valid?}
    K -- Yes --> L[scale samples by render_interval / decimation]
    K -- No --> M[use raw per-env.step samples]
    L --> N[_compute_frame_stats + _compute_fps_stats]
    M --> N
    N --> O[run_dict]
    O --> P[_aggregate_runs]

    F --> Q[report dict with env_cfg block]
    Q --> R[_print_stdout_summary]
    Q --> S[_write_json_report optional]

Loading

_{Reviews (2): Last reviewed commit: "Addressing feedback" | Re-trigger Greptile}

[rwiltz]

@greptile

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

PR: #5716 — Add env perf/timing config to teleop_replay_agent stats output
Commit: ccc938f5 (updated from 74d5c3a8)

---

Summary

This PR enhances teleop_replay_agent.py with two significant improvements:

1. Embeds environment performance/timing configuration into the stats JSON output
2. Reports cpu_frame_time_ms and fps on a per-render basis instead of per-env.step, matching the simulator HUD

The new commit addresses feedback from the initial review with clean, well-documented fixes.

---

✅ Review Feedback Addressed

The author has addressed several points raised in the previous review:

1. ✓ Stdout formatting consistency: Added _or_na() helper function to normalize None as "n/a" for integer fields (num_envs, decimation, render_interval), matching the behavior of _fmt() for floats. The Env timing line now displays consistently.

2. ✓ Dead code removal: Removed the misleading env_cfg = None init whose comment incorrectly described a finally-side report path that does not exist (since _build_report runs after the try/finally).

3. ✓ Code efficiency: _extract_env_perf_cfg() result is now stored once in env_perf_cfg and reused for both the decimation/render_interval extraction and the report dict, avoiding a redundant function call.

---

✅ Strengths

1. Well-designed per-render conversion: The to_dict() method now correctly converts env.step samples to per-render basis using samples_ms = [t / renders_per_env_step for t in samples_ms], with proper guards for None/zero values.

2. Comprehensive docstrings: The extended comment block in to_dict() clearly explains the math: per-render ms = env.step ms × render_interval / decimation.

3. Defensive graceful degradation: Falls back to raw env.step units when decimation or render_interval is unavailable, rather than dropping the report entirely.

4. Brand-neutral documentation: Systematic replacement of "Kit" with "the simulator" throughout docstrings and comments maintains documentation quality while making it renderer-agnostic.

5. Complete changelog: The .rst changelog properly documents both the Added (env_cfg block) and Changed (per-render basis) modifications.

---

💬 Remaining Minor Notes (Non-blocking)

1. Changelog still references "Kit": The changelog file (rwiltz-replay-stats-env-cfg.rst) contains two remaining references to "Kit" ("the number of Kit renders" and "Kit's HUD shows") while the main script now uses "the simulator". Consider updating for consistency with the script's terminology changes.

2. Type annotations: _safe() and _or_na() helper functions could benefit from type hints for better IDE support, but this is stylistic and the functions work correctly as-is.

---

📋 CI Status

All pre-commit checks passed. Most CI jobs are still pending for the new commit.

---

Verdict

Clean iteration that addresses review feedback thoroughly. The per-render normalization is a meaningful improvement that makes the output match what users actually see in the simulator HUD. Ready for maintainer review.

---

_{🔍 Reviewed by Isaac Lab Review Bot | 📋 Review Guidelines}