Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 48 additions & 1 deletion isaaclab_arena/tests/test_osmo_experiment_workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@
from osmo.tasks.experiment_output_task import (
_REMOTE_BUILD_EXPERIMENT_OUTPUT_SCRIPT_PATH,
_REMOTE_EXPERIMENT_RUNNER_OUTPUT_DIRECTORIES_FILE_PATH,
ExperimentOutputTaskCfg,
experiment_output_swift_url,
experiment_report_https_url,
experiment_runner_output_directory_input_token,
)
from osmo.tasks.experiment_runner_task import REMOTE_EXPERIMENT_PATH, ExperimentRunnerTask, ExperimentRunnerTaskCfg
Expand All @@ -49,6 +52,7 @@
REPOSITORY_ROOT / "isaaclab_arena_environments/experiment_configs/droid_pnp_srl_openpi_experiment.yaml"
)
OPENPI_RUN_NAME = "droid_pnp_srl_openpi_billiard_hall"
TEMPORARY_REPORT_SWIFT_PATH = "AUTH_team-isaac/nvblox/reports"


def _pi0_experiment_cfg(first_variant: str = "pi05") -> ArenaExperimentCfg:
Expand Down Expand Up @@ -152,6 +156,7 @@ def test_explicit_experiment_and_policy_server_selector_compose_typed_defaults()
assert submission_cfg.osmo.platform == "ovx-l40s"
assert submission_cfg.experiment_runner == ExperimentRunnerTaskCfg()
assert submission_cfg.experiment_runner.image == "nvcr.io/nvstaging/isaac-amr/isaaclab_arena:latest"
assert submission_cfg.experiment_output == ExperimentOutputTaskCfg()
assert submission_cfg.policy_server == Pi0ServerTaskCfg()
assert submission_cfg.policy_server.client_ping_timeout_s == Pi0ServerTaskCfg.client_ping_timeout_s

Expand All @@ -174,7 +179,10 @@ def test_submitter_rejects_unregistered_policy_server_type():
submit_arena_experiment(submission_cfg)


@pytest.mark.parametrize("config_path", ["osmo.not_a_field", "experiment_runner.not_a_field"])
@pytest.mark.parametrize(
"config_path",
["osmo.not_a_field", "experiment_runner.not_a_field", "experiment_output.not_a_field"],
)
def test_hydra_rejects_unknown_typed_config_fields(config_path):
"""Let the structured Hydra root reject fields outside their owning config."""
with pytest.raises(ConfigCompositionException, match="not_a_field"):
Expand Down Expand Up @@ -337,6 +345,40 @@ def capture_submission(command, **kwargs):
assert not captured_workflow_path.exists()


def test_successful_submission_prints_direct_report_url(monkeypatch, capsys):
"""Print the browser URL corresponding to the submitted workflow's Swift output."""
submission_cfg = ArenaExperimentSubmissionCfg(
experiment_cfg=_pi0_experiment_cfg(),
policy_server=Pi0ServerTaskCfg(),
experiment_output=ExperimentOutputTaskCfg(swift_path=TEMPORARY_REPORT_SWIFT_PATH),
)

monkeypatch.setattr(
"osmo.workflows.workflow.subprocess.run",
lambda *args, **kwargs: SimpleNamespace(returncode=0, stdout="Workflow ID - arena-report-123\n"),
)

assert submit_arena_experiment(submission_cfg) == 0
output = capsys.readouterr().out
assert (
"Report (available when the workflow completes): "
"https://pdx.s8k.io/v1/AUTH_team-isaac/nvblox/reports/arena-report-123/index.html"
in output
)


def test_experiment_output_urls_append_workflow_identity_and_report_filename():
"""Derive collision-free upload and browser URLs from one configured Swift path."""
assert (
experiment_output_swift_url(f"/{TEMPORARY_REPORT_SWIFT_PATH}/")
== "swift://pdx.s8k.io/AUTH_team-isaac/nvblox/reports/{{workflow_id}}"
)
assert (
experiment_report_https_url(TEMPORARY_REPORT_SWIFT_PATH, "report workflow/1")
== "https://pdx.s8k.io/v1/AUTH_team-isaac/nvblox/reports/report%20workflow%2F1/index.html"
)


def test_submission_composes_defaults_experiment_and_overrides(tmp_path, capsys):
"""Resolve typed defaults, Experiment values, then CLI overrides."""
experiment_path = tmp_path / "experiment.yaml"
Expand All @@ -358,6 +400,7 @@ def test_submission_composes_defaults_experiment_and_overrides(tmp_path, capsys)
"osmo.dry_run=true",
"osmo.workflow_name=overridden-experiment",
"experiment_runner.image=registry.example.com/evaluator:branch",
f"experiment_output.swift_path={TEMPORARY_REPORT_SWIFT_PATH}",
"policy_server.image=registry.example.com/openpi:overridden",
"policy_server.policy_config=overridden-pi0-config",
"policy_server.client_ping_timeout_s=600.0",
Expand All @@ -379,6 +422,10 @@ def test_submission_composes_defaults_experiment_and_overrides(tmp_path, capsys)
assert [task["name"] for task in tasks] == ["experiment-runner-0", "policy-server-0"]
assert tasks[0]["image"] == "registry.example.com/evaluator:branch"
assert tasks[1]["image"] == "registry.example.com/openpi:overridden"
experiment_output_task = _workflow_tasks(workflow, -1)[0]
assert experiment_output_task["outputs"] == [
{"url": "swift://pdx.s8k.io/AUTH_team-isaac/nvblox/reports/{{workflow_id}}"}
]

experiment = _embedded_experiment(tasks[0])
policy = experiment["runs"]["openpi_maple_table"]["policy"]
Expand Down
14 changes: 13 additions & 1 deletion osmo/submit_arena_experiment.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
from isaaclab_arena.evaluation.arena_experiment_config_loader import load_arena_experiment_from_config_file
from isaaclab_arena.utils.hydra_overrides import assert_hydra_overrides
from osmo.tasks.base_task import TaskCfg
from osmo.tasks.experiment_output_task import ExperimentOutputTaskCfg, experiment_report_https_url
from osmo.tasks.experiment_runner_task import ExperimentRunnerTaskCfg
from osmo.tasks.pi0_server_task import Pi0ServerTaskCfg
from osmo.workflows.arena_experiment_workflow import Pi0ArenaExperimentWorkflow
Expand Down Expand Up @@ -50,6 +51,9 @@ class ArenaExperimentSubmissionCfg:
experiment_runner: ExperimentRunnerTaskCfg = field(default_factory=ExperimentRunnerTaskCfg)
"""Configuration for the task that executes ``experiment_runner.py``."""

experiment_output: ExperimentOutputTaskCfg = field(default_factory=ExperimentOutputTaskCfg)
"""Configuration for publishing the complete Experiment output and report."""


def submit_arena_experiment(submission_cfg: ArenaExperimentSubmissionCfg) -> int:
"""Build and submit the OSMO workflow described by ``submission_cfg``.
Expand All @@ -72,8 +76,16 @@ def submit_arena_experiment(submission_cfg: ArenaExperimentSubmissionCfg) -> int
experiment_cfg=submission_cfg.experiment_cfg,
server_task_cfg=policy_server_task_cfg,
task_cfg=experiment_runner_task_cfg,
experiment_output_task_cfg=submission_cfg.experiment_output,
)
return workflow.submit_workflow().returncode
submission_result = workflow.submit_workflow()
if submission_result.returncode == 0 and submission_result.workflow_id is not None:
report_url = experiment_report_https_url(
submission_cfg.experiment_output.swift_path,
submission_result.workflow_id,
)
print(f"Report (available when the workflow completes): {report_url}")
return submission_result.returncode


def build_arena_experiment_submission_cfg(
Expand Down
45 changes: 41 additions & 4 deletions osmo/tasks/experiment_output_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,52 @@
import json
import shlex
from collections.abc import Mapping
from dataclasses import dataclass
from pathlib import Path
from typing import Any
from urllib.parse import quote

from osmo.tasks.base_task import BaseTask
from osmo.tasks.base_task import BaseTask, TaskCfg
from osmo.workflows.utils.yaml_utils import block_literal_str
from osmo.workflows.workflow_constants import DATASET_SWIFT_URL, OSMO_TASK_OUTPUT_DIR
from osmo.workflows.workflow_constants import DATASETS_PATH, HTTPS_URL_PREFIX, OSMO_TASK_OUTPUT_DIR, SWIFT_URL_PREFIX

_LOCAL_BUILD_EXPERIMENT_OUTPUT_SCRIPT_PATH = Path(__file__).parents[1] / "scripts" / "build_experiment_output.py"
_REMOTE_BUILD_EXPERIMENT_OUTPUT_SCRIPT_PATH = "/tmp/arena_build_experiment_output.py"
_REMOTE_EXPERIMENT_RUNNER_OUTPUT_DIRECTORIES_FILE_PATH = "/tmp/arena_experiment_runner_output_directories.json"
_OSMO_WORKFLOW_ID_TOKEN = "{{workflow_id}}"
_EXPERIMENT_REPORT_FILENAME = "index.html"


@dataclass
class ExperimentOutputTaskCfg(TaskCfg):
"""Configuration for publishing a complete Arena Experiment output."""

swift_path: str = DATASETS_PATH
"""Swift account, container, and prefix; OSMO appends the workflow ID."""


def _normalize_swift_path(swift_path: str) -> str:
"""Return a non-empty Swift account/container/prefix without surrounding slashes."""
normalized_swift_path = swift_path.strip("/")
assert normalized_swift_path, "Experiment output Swift path must not be empty"
assert (
"://" not in normalized_swift_path
), "Experiment output swift_path must contain only the account, container, and prefix, not a URL scheme"
return normalized_swift_path


def experiment_output_swift_url(swift_path: str) -> str:
"""Return the OSMO output URL for one workflow below ``swift_path``."""
normalized_swift_path = _normalize_swift_path(swift_path)
return f"{SWIFT_URL_PREFIX}/{normalized_swift_path}/{_OSMO_WORKFLOW_ID_TOKEN}"


def experiment_report_https_url(swift_path: str, workflow_id: str) -> str:
"""Return the browser URL for a workflow's uploaded Experiment report."""
normalized_swift_path = _normalize_swift_path(swift_path)
assert workflow_id, "Experiment report URL requires a workflow ID"
encoded_workflow_id = quote(workflow_id, safe="")
return f"{HTTPS_URL_PREFIX}/{normalized_swift_path}/{encoded_workflow_id}/{_EXPERIMENT_REPORT_FILENAME}"


def experiment_runner_output_directory_input_token(experiment_runner_task_name: str) -> str:
Expand All @@ -38,13 +74,14 @@ class ExperimentOutputTask(BaseTask):

def __init__(
self,
task_cfg: ExperimentOutputTaskCfg,
image: str,
experiment_runner_task_names_by_run_name: Mapping[str, str],
lead: bool | None = None,
resource: str | None = None,
) -> None:
assert experiment_runner_task_names_by_run_name, "Experiment output requires at least one Run task"
super().__init__(lead=lead, resource=resource)
super().__init__(task_cfg=task_cfg, lead=lead, resource=resource)
self.image = image
self.experiment_runner_task_names_by_run_name = dict(experiment_runner_task_names_by_run_name)

Expand All @@ -64,7 +101,7 @@ def _get_inputs(self) -> list[dict[str, Any]]:

def _get_outputs(self) -> list[dict[str, Any]]:
"""Publish the final Experiment directory, including all Runs and ``index.html``."""
return [{"url": DATASET_SWIFT_URL}]
return [{"url": experiment_output_swift_url(self.task_cfg.swift_path)}]
Comment on lines 102 to +104

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 self.task_cfg not narrowed to ExperimentOutputTaskCfg

BaseTask declares self.task_cfg as TaskCfg | None, so a static type-checker will flag the .swift_path access on line 104 as an unknown attribute and a potential None dereference. At runtime it's always safe because ExperimentOutputTask.__init__ requires a non-optional ExperimentOutputTaskCfg, but the declared type doesn't reflect that. Adding a typed attribute annotation (e.g. task_cfg: ExperimentOutputTaskCfg on the class body) would let type checkers verify this without changing behaviour.


def _get_files_to_create(self) -> list[dict[str, Any]]:
"""Embed the output-building script and its ``run-name -> Experiment Runner output`` JSON input."""
Expand Down
5 changes: 4 additions & 1 deletion osmo/workflows/arena_experiment_workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from isaaclab_arena.evaluation.arena_run import ArenaRunCfg
from isaaclab_arena_openpi.policy.pi0_remote_config import Pi0RemotePolicyCfg
from osmo.tasks.base_task import BaseTask
from osmo.tasks.experiment_output_task import ExperimentOutputTask
from osmo.tasks.experiment_output_task import ExperimentOutputTask, ExperimentOutputTaskCfg
from osmo.tasks.experiment_runner_task import ExperimentRunnerTask, ExperimentRunnerTaskCfg
from osmo.tasks.pi0_server_task import Pi0ServerTask, Pi0ServerTaskCfg
from osmo.workflows.workflow import Workflow, WorkflowCfg
Expand All @@ -40,10 +40,12 @@ def __init__(
server_task_cfg: Pi0ServerTaskCfg,
group_name: str = "arena",
task_cfg: ExperimentRunnerTaskCfg | None = None,
experiment_output_task_cfg: ExperimentOutputTaskCfg | None = None,
) -> None:
assert isinstance(experiment_cfg, ArenaExperimentCfg)
self.experiment_cfg = deepcopy(experiment_cfg)
self.pi0_server_task_cfg = server_task_cfg
self.experiment_output_task_cfg = experiment_output_task_cfg or ExperimentOutputTaskCfg()
super().__init__(
workflow_cfg=workflow_cfg,
task_cfg=task_cfg or ExperimentRunnerTaskCfg(),
Expand Down Expand Up @@ -118,6 +120,7 @@ def _create_experiment_output_group_dict(
) -> dict[str, Any]:
"""Collect every Experiment Runner task output into one published Experiment output."""
experiment_output_task = ExperimentOutputTask(
task_cfg=self.experiment_output_task_cfg,
image=self.task_cfg.image,
experiment_runner_task_names_by_run_name=experiment_runner_task_names_by_run_name,
lead=True,
Expand Down
Loading