Move RoboLab episode_length_s to composite task root

[qianl-nv]

Summary

Set RoboLab task episode limits on the composite root and wire conversion.

Detailed description

• Add episode_length_s to CompositeTaskSpec and pass it into composite/atomic task constructors during graph conversion
• Move episode_length_s out of RoboLab subtask params into each task YAML root, using upstream RoboLab benchmark durations
• Update graph-spec tests and fixtures to match the new schema

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

This PR moves episode_length_s from RoboLab subtask params to the composite task root: it adds an optional episode_length_s field to CompositeTaskSpec, threads it into the atomic/composite/sequential constructors during graph conversion, and updates the RoboLab task YAMLs to carry per-task benchmark durations at the root. This is a real correctness fix — the env builder sets env_cfg.episode_length_s from the root task’s get_episode_length_s(), so previously every parallel/sequential RoboLab task silently ran for the 20 s default no matter how many subtasks it had; multi-object tasks now get their intended (longer) budgets. The change is well-scoped and backward-compatible: episode_length_s defaults to None, so any spec that doesn’t set it keeps the existing 20 s default.

Test Coverage

Good. The unit tests cover the atomic, sequential, and (new) parallel conversion paths and assert the root value is threaded through; the sim-level graph-conversion test uses the inner/outer pattern and now asserts arena_env.task.get_episode_length_s() == 20.0 against the root-level YAML value. Nothing missing.

Verdict

Ship it — clean, well-tested, and it fixes a real episode-length bug for composite RoboLab tasks.

[greptile-apps]

Greptile Summary

This PR moves episode_length_s from individual subtask params to the CompositeTaskSpec root, and updates all 37 RoboLab task YAML files to use per-task benchmark durations (replacing a universal 20 s default).

• CompositeTaskSpec gains an episode_length_s: float | None field; the conversion utils forward it to atomic tasks and composite task constructors, with subtask-level params still taking precedence.
• All RoboLab task YAMLs are migrated from episode_length_s: 20.0 in each subtask's params block to a single per-task root value matching upstream benchmark durations (ranging 30 s–240 s).
• Graph-spec unit tests and the integration fixture are updated to cover the new field for atomic, sequential, and parallel composition types.

Confidence Score: 4/5

Safe to merge. The structural move of episode_length_s to the composite root is consistent across all 37 YAML files and the conversion logic correctly guards against double-application.

The core plumbing change is correct and well-tested for the happy path. The only gaps are a stale docstring example pointing at the old subtask-params convention and a missing test for the episode_length_s=None default and the subtask-params-over-root precedence path.

arena_env_graph_task_conversion_utils.py (stale docstring) and test_arena_env_graph_task_conversion.py (missing default and precedence tests for episode_length_s).

Important Files Changed

Filename	Overview
isaaclab_arena/environment_spec/arena_env_graph_task_conversion_utils.py	Core conversion logic correctly threads root episode_length_s to atomic/composite constructors; docstring example in _resolve_node_refs_in_task_args still shows the old subtask-params pattern.
isaaclab_arena/environment_spec/arena_env_graph_types.py	Adds episode_length_s: float
isaaclab_arena/tests/test_arena_env_graph_task_conversion.py	New tests cover parallel and sequential episode_length_s propagation; missing a test for the episode_length_s=None default and for the subtask-params-over-root precedence rule.
isaaclab_arena/tests/test_arena_env_graph_conversion.py	Integration assertion added to verify episode_length_s=20.0 on the sequential task; straightforward and correct.
isaaclab_arena/tests/test_data/pick_and_place_maple_table_env_graph.yaml	episode_length_s moved from both subtask params to composite root at 20.0; consistent with new schema.
isaaclab_arena_environments/robolab/tasks/banana_in_bowl.yaml	Representative of all 37 task YAML changes: episode_length_s lifted to root with benchmark-calibrated value (50.0 s vs old 20.0 s default).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["CompositeTaskSpec\nepisode_length_s"] --> B{composition type}
    B -- ATOMIC --> C["_build_atomic_task_from_spec\nsubtask + episode_length_s"]
    C --> D{episode_length_s\nin subtask params?}
    D -- "yes, subtask wins" --> E["atomic task class\nepisode_length_s from params"]
    D -- "no, use root" --> F["atomic task class\nepisode_length_s from root"]
    B -- PARALLEL --> G["subtasks built\nno episode_length_s"]
    G --> H["CompositeTaskBase\nepisode_length_s from root"]
    B -- SEQUENTIAL --> I["subtasks built\nno episode_length_s"]
    I --> J["SequentialTaskBase\nepisode_length_s from root"]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["CompositeTaskSpec\nepisode_length_s"] --> B{composition type}
    B -- ATOMIC --> C["_build_atomic_task_from_spec\nsubtask + episode_length_s"]
    C --> D{episode_length_s\nin subtask params?}
    D -- "yes, subtask wins" --> E["atomic task class\nepisode_length_s from params"]
    D -- "no, use root" --> F["atomic task class\nepisode_length_s from root"]
    B -- PARALLEL --> G["subtasks built\nno episode_length_s"]
    G --> H["CompositeTaskBase\nepisode_length_s from root"]
    B -- SEQUENTIAL --> I["subtasks built\nno episode_length_s"]
    I --> J["SequentialTaskBase\nepisode_length_s from root"]

Loading

Comments Outside Diff (2)

1. isaaclab_arena/environment_spec/arena_env_graph_task_conversion_utils.py, line 81-85 (link)

   The docstring example for _resolve_node_refs_in_task_args still shows episode_length_s living in raw_task_args (subtask params), which is the old pattern this PR is moving away from. A reader following this example as a template would put episode_length_s back in the wrong place.

   Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

2. isaaclab_arena/tests/test_arena_env_graph_task_conversion.py, line 163-188 (link)

   Missing precedence and default-value tests for episode_length_s

   There is a parallel test for task_description precedence (test_build_atomic_task_from_spec_params_task_description_takes_precedence) but no equivalent for episode_length_s. Two cases are untested: (1) when episode_length_s=None (the default), the key should be absent from task_init_kwargs; (2) when a subtask's params already contains episode_length_s, the subtask value should take precedence over the root value. If the guard at line 71 were accidentally changed from is not None to a truthiness check, a 0.0 root value would be silently dropped, and no existing test would catch it.

_{Reviews (1): Last reviewed commit: "Fix test data" | Re-trigger Greptile}