Add Osmo Arena Experiment output downloader

[cvolkcvolk]

Summary

Reliably download Arena Experiment outputs

Detailed description

• Arena Experiment workflows publish reports and per-Run JSONL outcomes to object storage, but users previously had to know and invoke the underlying OSMO/Swift download command.
• OSMO 6.3.1 directory downloads can report success after retrieving only a subset of the workflow output; in the reproduced case, four of ten objects were downloaded, omitting five Run JSONLs and index.html.
• Add one workflow-ID-based command that inventories remote objects, performs the bulk download, retrieves omissions individually, and verifies that the local output exactly matches the inventory.
• Store downloads under /eval/<workflow-id> by default, allow local and remote base overrides, validate workflow IDs, and reject non-empty destinations to avoid mixing output sets.

[greptile-apps]

Greptile Summary

This PR adds osmo/scripts/download_experiment_output.py, a new CLI script that wraps osmo data download to fetch the complete output of an Arena Experiment workflow by ID into a local directory, along with a matching __init__.py and a thorough unit-test suite.

• New download_experiment_output script: validates the workflow ID with is_valid_workflow_id (plus an explicit ./.. guard), creates the destination directory, asserts it is empty, then delegates to subprocess.run(["osmo", "data", "download", ...]) as a list command (no shell injection surface). The default destination is arena_experiment_outputs/<workflow-id>.
• Tests: cover default-directory creation, explicit (space-containing) paths, a parametrized set of invalid IDs, non-empty destination rejection, and failure-code propagation via monkeypatching.

Confidence Score: 4/5

Safe to merge; the new script is additive and isolated, with no effect on existing paths.

The core download logic is straightforward and well-tested. Two non-blocking issues are worth addressing before wider use: the validation guards in download_experiment_output use assert (disabled by -O, which would also silently break the related tests), and failed downloads leave behind a created output directory that blocks retry if partial files were written. Neither affects the happy path or existing code.

osmo/scripts/download_experiment_output.py — specifically the assert-based guards and the missing cleanup on download failure.

Important Files Changed

Filename	Overview
osmo/scripts/download_experiment_output.py	New script wrapping osmo data download for fetching Experiment outputs; validates workflow ID and destination, then delegates to subprocess. Two P2 concerns: assert-based guards are disabled under -O, and partial-download failures orphan the output directory.
isaaclab_arena/tests/test_osmo_download_experiment_output.py	Good coverage of the happy path, explicit destination, invalid IDs, non-empty destination guard, and failure propagation; tests rely on AssertionError which won't fire under -O.
osmo/scripts/init.py	Trivial package init with only license header; no logic to review.

_{Reviews (1): Last reviewed commit: "Add Arena Experiment output downloader" | Re-trigger Greptile}

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

Adds a small standalone script (osmo/scripts/download_experiment_output.py) that downloads one Arena Experiment output from OSMO/Swift by workflow ID, plus a solid set of unit tests. It is generic, lives outside the core package, reuses the existing is_valid_workflow_id / DATASETS_SWIFT_URL helpers, passes the command to subprocess.run as a list (no shell splitting), and validates the ID as a safe path component. Clean overall — just two minor notes.

Findings

🟡 download_experiment_output.py:44 — The empty-directory check guards against writing over an existing experiment output. Its condition is external filesystem state (user-recoverable), and as an assert it is stripped under python -O, silently removing the guard. Consider raising a real error instead.

🔵 download_experiment_output.py:38 — The "valid ID and not ./.." check is duplicated here and in _workflow_id_argument. A small shared predicate would keep the ./.. rule in one place.

Test Coverage

Good. Pure unit tests (no SimulationApp, so the inner/outer pattern isn't needed) covering the default and explicit destinations, no-shell-splitting on a path with spaces, invalid-ID rejection ("", ., .., /, space), non-empty-directory rejection, and non-zero return-code propagation. They land correctly in Phase 1. If the :44 guard becomes a raised error, test_rejects_nonempty_output_directory_before_download would move from AssertionError to that exception.

Verdict

Minor fixes needed (both optional — neither blocks merge).