[david-tingdahl-nvidia]

Write the Experiment's KPIs as a flat list of InfluxDB-ready records.

Why this change?

We are building a KPI reporting framework. The results of an Experiment need to reach a time-series database so that
success rates and timings can be tracked per commit. The experiment result is a nested file that is not suited for
ingestion.

What changes?

• New arena_experiment_influxdb_kpi_records.py builds arena_experiment_influxdb_kpi_records.json next to the
  experiment result: a flat list of records with a timestamp, the Arena commit and branch, tags and numeric fields.
  There are three record types: per Experiment, per Run, and timing.
• The produced file can be directly ingested into an influxDB without any modification. Hence a stand-alone injector module does not have to make any assumptions on the format.
• The record classes check their own contents when they are created, so an invalid record cannot be written.
• The experiment runner and the OSMO output builder write the file after the report. It only formats data; it does
  not talk to any database.

Example KPI format

     {
       "type": "per_run",
       "run_name": "swap_objects",
       "environment": "pick_and_place_maple_table",
       "policy_variant": "zero_action",
       "success_rate": 0.0,
       "num_episodes": 1,
       "num_successes": 0,
       "mean_episode_length": 23.0,
       "mean_progress": 0.3333333432674408,
       "completed": 1
     },
     {
       "type": "timing",
       "name": "step",
       "run_name": "all",
       "mean_ms": 201.72688941491523,
       "total_ms": 18558.873826172203,
       "min_ms": 165.67344404757023,
       "max_ms": 238.26710926368833,
       "count": 92
     },

[david-tingdahl-nvidia]

/review

[greptile-apps]

Confidence Score: 5/5

[Medium impact] This PR appears safe to merge; no actionable issues were found.

Summary

Adds flat KPI records for experiments, Runs, and timers, then writes them after the report in both output paths.

• Experiment results produce flat records with success and timing measures.
• Direct and OSMO experiment outputs now include the KPI records file.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  R[Experiment result] --> K[Build KPI records]
  E[Episode results] --> S[Pool completed Run episodes]
  S --> K
  T[Optional timing file] --> K
  K --> V[Check values and unique tags]
  V --> J[Write KPI JSON]

Loading

_{Reviews (1) · Last reviewed commit: "evaluation: write the experiment KPIs as..." · Reviewed by Greptile}

[arena-review-bot]

🟡 A Run named all fails only after the whole Experiment has run

Nothing rejects a Run named all up front. experiment_runner.py (line 200) and build_experiment_output.py (line 187) always write the timings file before this step, so this assert fires after every Run has finished. On OSMO it is worse: the per-Run runner exits non-zero, ExperimentRunnerTask records the Run as failed, and the collector drops its episodes. Then the collector's own KPI step hits the same assert and fails the task under set -euo pipefail.

Could the totals get their own record type, for example type="timing_total" with no run_name tag? Then no Run name could collide with them. If you'd rather keep "all", could this check move next to assert_run_name_is_safe_path_component so it fails before any Run starts?

[arena-review-bot]

🟡 Should the runner write this by default?

Only the per-commit KPI pipeline needs this file, but every local and OSMO run now builds it. If any assert in it fires, the runner exits non-zero after the evaluation is already done.

On OSMO, that exit code turns a completed Run into a failed one (ExperimentRunnerTask._get_run_script), and the collector then drops its episodes. The file written here is thrown away anyway, because collect_run_outputs_into_experiment_output copies only <runner-output>/<run_name>/.

Could this be opt-in, for example a runner flag the KPI job sets, or left to build_experiment_output.py? build_experiment_influxdb_kpi_records only needs the output directory, so a small __main__ entry point would also let the KPI job build the file from any finished output.

[arena-review-bot]

🟡 How will a dashboard tell Experiments apart?

per_experiment records and the run_name="all" timing records only have type, git_sha and git_branch as tags. Neither ArenaExperimentCfg nor arena_experiment_result.json has an Experiment name. If two benchmarks run on the same commit, their records end up in the same series and differ only by timestamp, so a "success rate per commit" chart would mix them.

Is the ingester meant to pick a measurement per Experiment? If so, could the module docstring say that? If not, these records probably need an Experiment tag.

[arena-review-bot]

🔵 Second copy of the InfluxDB rules

This helper repeats MIN_TIMESTAMP_NS, TIMESTAMP_NS_KEY, _is_number, _is_finite_number and the tag-set uniqueness check from the production module. It also adds rules the production records don't check: no keys starting with _, and no time key. On top of that, it needs 17 tests of its own (test_validation_helper_rejects_invalid_records, test_validation_helper_rejects_non_list_contents).

Could those extra rules move into InfluxdbKpiRecord.__post_init__, so there is one set of rules? The integration tests could then compare the written file with the output of build_experiment_influxdb_kpi_records, as test_written_file_is_valid_influxdb_points already does. That would let you drop this helper and its tests.

[arena-review-bot]

🔵 record_tags also holds the timestamp

The module says timestamp_ns is not a tag (TIMESTAMP_NS_KEY, the skip in __post_init__), but it's in this dict. A name like shared_values would match what the dict holds. Both **record_tags uses below would need the same rename.

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

This PR adds arena_experiment_influxdb_kpi_records.py, which turns a finished Experiment output into flat per-Experiment, per-Run and timing records. Both the local Experiment Runner and the OSMO collector now write that file after the report. The earlier review of this commit still applies. This pass adds one startup-order problem in the runner and two gaps in the new tests.

Design, Boundaries & Scope

• Still open from the earlier review of this commit:
  • The runner writes the file by default, and a failure while building it fails an evaluation that already finished (experiment_runner.py:207).
  • A Run named all only fails at the very end (arena_experiment_influxdb_kpi_records.py:425).
  • per_experiment records and the run-total timing records have no Experiment identifier (:195).
• Chunked runs add to the first and third points. With --chunk_size, each chunk subprocess writes its own KPI file. Each file has a per_experiment record that covers only that chunk, so one sweep shows up as several partial Experiments. If the KPI step fails in one chunk, run_legacy_json_in_chunks calls sys.exit and the remaining chunks never run.
• Episode counts. The earlier review asked whether unpaired mp4s could inflate num_episodes. That doesn't happen for Experiment runs: CameraObsVideoRecorder.close() deletes partial-episode videos, and only the standalone policy runner writes per-rank results. There is a smaller gap, though. The report scan skips everything under <experiment>/report/, so a Run named report would get num_episodes=0 while arena_experiment_result.json lists its episodes. Reading episodes from the result file, as suggested earlier, would fix that and remove the second directory scan.

Findings

🟡 Warning: isaaclab_arena/evaluation/experiment_runner.py:15 — This new top-level import now loads torch, gymnasium, isaaclab.utils and isaaclab_rl before the app starts, through arena_run and isaaclab_arena/policy/__init__.py. Before this PR, the runner kept all of these out of its startup path.
🔵 Improvement: isaaclab_arena/tests/test_arena_experiment_influxdb_kpi_records.py:118 — Both Runs have 50 episodes, so the test can't tell the documented episode-weighted success rate from a plain average of the per-Run rates.
🔵 Improvement: isaaclab_arena/tests/test_arena_experiment_influxdb_kpi_records.py:184 — The latest Run is listed first, so code that used the first Run's end time would also pass.

Earlier inline findings on this commit are still open: experiment_runner.py:207, arena_experiment_influxdb_kpi_records.py:425, :195 and :300, and tests/utils/influxdb_kpi_records_validation.py:13.

Test Coverage and CI Cost

• Not yet run in CI. Pre-commit passed on this head commit, but the test jobs were skipped and the image build is still queued. test_experiment_runner_from_typed_yaml is the only test that would catch a startup problem from the import above, and it has not run in CI yet.

Verdict

Minor fixes needed

[arena-review-bot]

🟡 Loads torch and Isaac Lab before the app starts

This top-level import loads arena_run, which the KPI module imports for RunStatus. arena_run in turn imports isaaclab_arena.policy, whose __init__.py star-imports rsl_rl_action_policy and replay_action_policy. So the runner now loads torch, gymnasium, isaaclab.utils and isaaclab_rl before SimulationAppContext starts the app on line 157. Before this PR, only isaaclab.app, hydra and the standard library were loaded at that point. timer.py:147 notes that torch must not load before Isaac Sim starts.

Could this import move into the with SimulationAppContext(args_cli): block next to the build_report import, where the runner already imports arena_run? Another option: the KPI module could compare against COMPLETED_STATUS from report_data and drop its arena_run import. That would keep it as light as report_data.py.

[arena-review-bot]

🔵 Can't tell the episode-weighted rate from a plain average

Both Runs have 50 episodes, so averaging the two per-Run rates also gives 81 / 100, and this test would still pass if the code did that. If one Run has a different episode count, the test checks the episode-weighted rate that PerExperimentRecord.from_jobs documents:

Suggested change

	"pnp_pi0": {"episodes": _episode_records(50, 40, episode_length=20), "policy_variant": "pi0"},
	"pnp_pi0": {"episodes": _episode_records(10, 2, episode_length=20), "policy_variant": "pi0"},

The expected values below would then change to num_successes == 2 for this Run, and to success_rate == 43 / 60 and num_episodes == 60 in the per-Experiment record.

[arena-review-bot]

🔵 Latest Run is listed first

late comes before early, so code that used the first Run's end_timestamp_ns instead of the latest would also pass. Listing early first would make the test check what its name says.