Repository navigation
Record Arena git provenance and each Run's end time in the experiment result - #1540
david-tingdahl-nvidia wants to merge 4 commits into
Conversation
|
/review |
 [Medium impact] The change reaches saved results across direct and OSMO experiment runs. The PR appears safe to merge, with a non-blocking concern about preserving the execution commit when collecting saved results. Findings
SummaryAdds the Arena git commit and branch to experiment results and records each run's end time.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Local run finishes or fails] --> B[Record time.time_ns]
B --> C[Write experiment result]
D[OSMO runner process exits] --> E[Record shell end time]
E --> F[Write runner result]
F --> G[Collect run outputs]
G --> C
H[Writer environment] --> I[Read git commit and branch]
I --> C
Reviews (1) · Last reviewed commit: "evaluation: record Arena git provenance ..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| """ | ||
| assert run_metadata_by_name, "run_metadata_by_name must not be empty" | ||
| self.experiment_output_directory = Path(experiment_output_directory) | ||
| self._provenance = provenance if provenance is not None else Provenance.from_environment() |
There was a problem hiding this comment.
ArenaExperimentResult reads the commit and branch from the machine collecting the results. build_experiment_output uses this default, while the OSMO runner result does not carry provenance. If saved outputs are collected in a newer image, old runs get labeled with the newer commit, making comparisons between versions unreliable. Preserve the runner's provenance in its output and pass it explicitly when collecting results.
![arena-review-bot[bot]](https://avatars.githubusercontent.com/u/195794629?s=60&v=4){kind=small}
| ) | ||
| write_experiment_runner_result_command = ( | ||
| 'printf \'{"execution_status":"%s","process_exit_code":%d,"runs":%s}\\n\' ' | ||
| 'printf \'{"execution_status":"%s","process_exit_code":%d,"end_timestamp_ns":%d,"runs":%s}\\n\' ' |
There was a problem hiding this comment.
🟡 Collector records its own commit, not the Runs'
build_experiment_output creates ArenaExperimentResult without a provenance, so the commit comes from the collector container. That container starts later, from the same :latest tag that ci.yml moves on every push to main. A merge during a long Experiment would label the results with a newer commit than the one that produced them. Separate Run groups could even run different commits.
Could this printf also write $ARENA_GIT_SHA and $ARENA_GIT_BRANCH? Then build_experiment_output could check that every Run reports the same values and pass a Provenance into ArenaExperimentResult. That would give the new provenance parameter its first production caller; today only a test passes it.
| ), f"provenance {provenance_field.name} must be a non-empty single-line string, got {value!r}" | ||
|
|
||
| @classmethod | ||
| def from_environment(cls) -> Provenance: |
There was a problem hiding this comment.
🟡 Stale commit in the dev container
run_docker.sh mounts the host checkout over the image's copy and only rebuilds the image with -r. So ARENA_GIT_SHA keeps the commit and -dirty state from the last image build while the mounted code moves on. Results from local runs, and the KPI records that #1541 writes from them, would then name a commit that didn't produce them.
Could run_docker.sh pass --env ARENA_GIT_SHA= --env ARENA_GIT_BRANCH=? This method already maps empty values to unknown, so local runs would record unknown. Or should this read the commit from the checkout with git when .git is present?
| metrics: MetricsDataCollection | None = None | ||
| """Metrics aggregated over all successful rebuilds, when provided by the task.""" | ||
|
|
||
| end_timestamp_ns: int | None = None |
There was a problem hiding this comment.
🔵 end_timestamp_ns could be required
The None state only exists between build_and_run returning and the replace(...) in execute_experiment. Because of it, _write_arena_experiment_result needs the nested _end_timestamp_ns assert.
If build_and_run stamped its own result with end_timestamp_ns=time.time_ns(), this could be a required end_timestamp_ns: int declared before metrics. The replace and the nested helper could then go. The fake build_and_run functions in test_run_execution.py would set the value themselves, and a build_and_run test would need to assert it for the success path.
|
|
||
|
|
||
| @pytest.mark.parametrize("status", ["completed", "failed"]) | ||
| def test_run_metadata_end_timestamp_is_written(tmp_path, status): |
There was a problem hiding this comment.
🔵 Duplicate test
test_collects_declared_runs_rebuilds_and_rank_files_without_changing_episode_objects already checks end_timestamp_ns for both a completed and a failed Run through its full to_dict() comparison. This test doesn't add coverage. Could we drop it? It also repeats the literal instead of using END_TIMESTAMP_NS.
| }], | ||
| }] | ||
| assert set(first_run_result) == {"environment", "policy_variant", "status", "rebuilds"} | ||
| assert first_run_result["end_timestamp_ns"] == END_TIMESTAMP_NS |
There was a problem hiding this comment.
🔵 Both Runs share one end time
Both runner results use the default END_TIMESTAMP_NS, so this test passes even if the collector gave every Run the same time.
Could the second runner pass end_timestamp_ns=END_TIMESTAMP_NS + 1, and this test also check experiment_result["runs"]["second"]["end_timestamp_ns"]? That would also give the new end_timestamp_ns parameter of _write_experiment_runner_result its first caller; no call passes it today.
🤖 Isaac Lab-Arena Review BotSummaryThis PR records the Arena git commit and branch ( Design, Boundaries & Scope
FindingsNew in this pass: 🔵 Improvement: osmo/scripts/build_experiment_output.py:57: Still open from the earlier inline threads: 🟡 Warning: osmo/tasks/experiment_runner_task.py:135: the collector records its own commit, not the commit that ran the Runs. Test Coverage and CI Cost
VerdictMinor fixes needed |
| }, f"Experiment Runner result for Run '{run_name}' must contain metadata for exactly that Run" | ||
| run_metadata = run_metadata_by_name[run_name] | ||
| assert isinstance(run_metadata, dict), f"Experiment Runner metadata for Run '{run_name}' must be a JSON object" | ||
| end_timestamp_ns = experiment_runner_result["end_timestamp_ns"] |
There was a problem hiding this comment.
🔵 end_timestamp_ns is checked twice
This value only goes into ArenaExperimentResult, and _collect_run (arena_experiment_result.py:144) runs the same int check on it. Could this just pass it through, and lines 57–60 go?
{**run_metadata, "end_timestamp_ns": experiment_runner_result["end_timestamp_ns"]},The Returns: line in the docstring also still says "environment and policy metadata". It now includes the end time too.
| git_branch: str | ||
| """Git branch name, or ``"unknown"``.""" | ||
|
|
||
| def __post_init__(self) -> None: |
There was a problem hiding this comment.
🔵 Is __post_init__ needed?
Git SHAs and branch names can't contain line breaks, and from_environment never returns an empty value. The single-line rule comes from InfluxDB tags, and #1541's InfluxdbKpiRecord.__post_init__ already checks every tag, git_sha and git_branch included.
Could we drop __post_init__ and keep the exact-key check in from_dict? The empty, line-break and None cases in test_provenance_rejects_invalid_values would go with it.
| assert '"runs":%s' in experiment_runner_command | ||
| assert '"end_timestamp_ns":%d' in experiment_runner_command | ||
| # The end time is taken once the Experiment Runner process has exited, before the result is written. | ||
| assert "fi\nexperiment_runner_end_timestamp_ns=$(date +%s%N)\n" in experiment_runner_command |
There was a problem hiding this comment.
🔵 Script output is never loaded
These asserts check pieces of the printf line, not the file it writes. If two arguments get swapped, they all still pass. For a failed Run, swapping the exit code and the end time would even load cleanly. test_osmo_build_experiment_output.py writes its runner results with a Python helper, so nothing checks the shell output against the loader.
Could one test run the rendered script with a stub isaaclab.sh and load the result? This file is already with_subprocess, so it adds one short bash process. It could also replace the asserts on lines 253, 255 and 256. A sketch (not run):
def test_experiment_runner_script_writes_a_result_the_collector_loads(tmp_path):
task = ExperimentRunnerTask(
task_cfg=ExperimentRunnerTaskCfg(),
experiment_cfg=_zero_action_experiment_cfg(),
lead=True,
task_name="experiment-runner",
)
stub_bin_directory = tmp_path / "bin"
stub_bin_directory.mkdir()
stub_isaaclab_script = stub_bin_directory / "isaaclab.sh"
stub_isaaclab_script.write_text("#!/bin/bash\nexit 3\n")
stub_isaaclab_script.chmod(0o755)
run_script = task._get_run_script().replace(OSMO_TASK_OUTPUT_DIR, str(tmp_path))
before_ns = time.time_ns()
subprocess.run(
["bash", "-c", run_script],
env={**os.environ, "PATH": f"{stub_bin_directory}:{os.environ['PATH']}"},
check=True,
)
after_ns = time.time_ns()
run_execution_report, run_metadata = load_experiment_runner_result(tmp_path, "baseline")
assert run_execution_report.status is RunStatus.FAILED
assert run_execution_report.process_exit_code == 3
assert before_ns <= run_metadata["end_timestamp_ns"] <= after_ns
9d093c4 to
199e51a
Compare
199e51a to
5aeeddd
Compare
| class ArenaExperimentResultData(TypedDict): | ||
| """Contents of an Arena Experiment result file.""" | ||
|
|
||
| git_stamp: dict[str, str] |
There was a problem hiding this comment.
Should be : GitStamp ?
| """Value recorded when the git commit or branch is not known.""" | ||
|
|
||
| ARENA_GIT_SHA_ENV_VAR = "ARENA_GIT_SHA" | ||
| """Environment variable holding the Arena git commit SHA; the Arena Docker image sets it.""" |
There was a problem hiding this comment.
we should not discuss the docker image here. it's a layer above us
|
|
||
| @classmethod | ||
| def from_environment(cls) -> GitStamp: | ||
| """Return the git stamp from ``ARENA_GIT_SHA`` and ``ARENA_GIT_BRANCH``, ``"unknown"`` when unset or empty.""" |
There was a problem hiding this comment.
enough to say "from environment"
5aeeddd to
ab19c3a
Compare
| environment: dict[str, str] | ||
| policy_variant: str | ||
| status: Literal["completed", "failed"] | ||
| end_timestamp_ns: int |
There was a problem hiding this comment.
Shall we add start_timestamp_ns for symmetry?
|
|
||
| ArenaExperimentResultData = dict[Literal["runs"], dict[str, ArenaExperimentRunData]] | ||
| class ArenaExperimentResultData(TypedDict): | ||
| """Contents of an Arena Experiment result file.""" |
There was a problem hiding this comment.
Suggestion to lengthen to say what the maps contain. I guess they map from run name to run data?
| process_exit_code=process_exit_code, | ||
| ), | ||
| run_metadata, | ||
| {**run_metadata, "end_timestamp_ns": experiment_runner_result["end_timestamp_ns"]}, |
There was a problem hiding this comment.
nit: this is a bit hard to read. Consider adding the timestamp to the metadata outside of this big return statement.
Add a GitStamp (git commit and branch of the Arena source, read from ARENA_GIT_SHA and ARENA_GIT_BRANCH) and each Run's end time to arena_experiment_result.json, and forward both to the OSMO experiment output. Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
…mments Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
682d1a7 to
9849580
Compare
Record the Arena git commit and each Run's end time in the experiment result.
Why this change?
We are building a KPI reporting framework. To track results over time, each result must say which Arena commit and
branch produced it and when each Run ended. The experiment result had neither. The commit and branch are now
available inside the image.
What changes?
GitStampdataclass reads the Arena commit and branch from the environment.end_timestamp_nsfor every Run.