Default episode_length_s to 20s for all tasks

[xyao-nv]

Summary

Default episode_length_s to 20s for all tasks

Detailed description

• Default episode_length_s to 20 s for all tasks via TaskBase instead of the per-env-cfg fallback (50s).
• Does not affect envs used in IL-interop and RL-interop example workflows

[greptile-apps]

Greptile Summary

This PR centralises the default episode length (20 s) in TaskBase instead of scattering it across per-environment YAML configs and the IsaacLabArenaManagerBasedRLEnvCfg class default (50 s). Mimic/data-generation environments are deliberately kept at 50 s via an explicit override on IsaacArenaManagerBasedMimicEnvCfg.

• TaskBase gains DEFAULT_EPISODE_LENGTH_S = 20.0 and coalesces None to that value in __init__, so all task subclasses that forward None automatically get the 20 s default; get_episode_length_s() return type is narrowed to float.
• arena_env_builder.py drops the None-guard and unconditionally assigns episode_length_s in the non-mimic branch; mimic envs remain unaffected and keep their 50 s class default.
• Six robolab YAML environments remove the now-redundant episode_length_s: 20.0 field, and a new test_task_base.py guards the None-coalescing logic.

Confidence Score: 5/5

Safe to merge — the change is a straightforward default consolidation with no behaviour change for the YAML-defined tasks (all had explicit 20 s) and an intentional, well-documented 50 s preservation for mimic envs.

All task subclasses forward None to TaskBase, which now coalesces it to 20 s — matching what the removed per-YAML overrides were already doing. The builder assignment is confined to the non-mimic branch, so data-generation pipelines are unaffected. The new unit tests directly guard the None-coalescing path. No logic regressions identified.

composite_task_base.py has a stale docstring claiming None disables the time limit, which is no longer true.

Important Files Changed

Filename	Overview
isaaclab_arena/tasks/task_base.py	Adds DEFAULT_EPISODE_LENGTH_S = 20.0 class constant and coalesces None → default in init; return type of get_episode_length_s narrowed from float
isaaclab_arena/environments/arena_env_builder.py	Removes the None-guard so episode_length_s is always applied to non-mimic envs; the assignment stays inside the non-mimic branch so mimic envs are unaffected. Change is correct.
isaaclab_arena/environments/isaaclab_arena_manager_based_env_cfg.py	Drops the 50 s override from IsaacLabArenaManagerBasedRLEnvCfg (now always overwritten by the builder) and adds an explicit 50 s default to IsaacArenaManagerBasedMimicEnvCfg to preserve long demo windows.
isaaclab_arena/tests/test_task_base.py	New data-only unit tests covering the None-coalescing regression case and an explicit-value preservation case. Clean and self-contained (no SimulationApp required).
isaaclab_arena_environments/robolab/bagel_plate_banana_bowl_linked.yaml	Removes explicit episode_length_s: 20.0 — now redundant with the new TaskBase default. Correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Task subclass __init__(episode_length_s=None)"] --> B["TaskBase.__init__"]
    B --> C{episode_length_s is None?}
    C -- Yes --> D["self.episode_length_s = 20.0 (DEFAULT_EPISODE_LENGTH_S)"]
    C -- No --> E["self.episode_length_s = episode_length_s"]
    D & E --> F["task.get_episode_length_s() → float"]
    F --> G["ArenaEnvBuilder.build_env()"]
    G --> H{mimic mode?}
    H -- No --> I["env_cfg = IsaacLabArenaManagerBasedRLEnvCfg(...)"]
    I --> J["env_cfg.episode_length_s = task value (always set)"]
    H -- Yes --> K["env_cfg = IsaacArenaManagerBasedMimicEnvCfg(...)"]
    K --> L["env_cfg.episode_length_s = 50.0 (class default, not overwritten)"]

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["Task subclass __init__(episode_length_s=None)"] --> B["TaskBase.__init__"]
    B --> C{episode_length_s is None?}
    C -- Yes --> D["self.episode_length_s = 20.0 (DEFAULT_EPISODE_LENGTH_S)"]
    C -- No --> E["self.episode_length_s = episode_length_s"]
    D & E --> F["task.get_episode_length_s() → float"]
    F --> G["ArenaEnvBuilder.build_env()"]
    G --> H{mimic mode?}
    H -- No --> I["env_cfg = IsaacLabArenaManagerBasedRLEnvCfg(...)"]
    I --> J["env_cfg.episode_length_s = task value (always set)"]
    H -- Yes --> K["env_cfg = IsaacArenaManagerBasedMimicEnvCfg(...)"]
    K --> L["env_cfg.episode_length_s = 50.0 (class default, not overwritten)"]

Loading

_{Reviews (8): Last reviewed commit: "revert" | Re-trigger Greptile}

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

The core change — defaulting episode_length_s to 20s on TaskBase and having the builder resolve it unconditionally — is clean: NoTask calls super().__init__(), so get_episode_length_s() is now always a concrete float and the unconditional env_cfg.episode_length_s = episode_length_s can never assign None. However, the diff is much larger than the title/description suggest, and it carries a debug stub that disables the policy runner.

Design, Boundaries & Scope

The PR is described as a one-line default change ("Default episode_length_s to 20s for all tasks"), but 10 of the 13 changed files add an entire GR00T/OpenPI OSMO policy-runner workflow stack (new osmo/tasks/gr00t_*.py, osmo/workflows/gr00t_*.py, workflow_constants.py, the lead-flag plumbing, etc.). Combined with the wip/cleanup/self review commits and the leftover debug stub below, this looks like in-progress OSMO work bundled into an unrelated default-length PR. Could the GR00T OSMO workflow be split into its own PR (and the description updated)? That would let each change be reviewed on its own merits and keep the default-length fix small.

Findings

See inline comments.

Test Coverage

The episode_length_s default change is a behavior change with no accompanying test — a small unit test asserting TaskBase().get_episode_length_s() == 20.0 and that a built env's episode_length_s resolves to the task value would lock in the intent. The new GR00T OSMO tasks/workflows are untested here (the gr00t closed-loop E2E job is the closest coverage).

Verdict

Minor fixes needed — blocking on the hello world stub; the rest are scope/cleanup items.

[qianl-nv]

episode_length related changes LGTM. But some osmo changes got in unintentionally? suggest to remove

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

This PR unifies the episode-length default to 20 s by making TaskBase own the fallback (DEFAULT_EPISODE_LENGTH_S = 20.0), dropping the 50.0 class default on IsaacLabArenaManagerBasedRLEnvCfg, and having the builder always write the task's resolved length onto env_cfg. It also injects the default into generated task params in the intent compiler. The change reads correctly — task.get_episode_length_s() now always returns a concrete float (including NoTask, which calls super().__init__()), so the unconditional env_cfg.episode_length_s = episode_length_s assignment is safe. A nice side effect: the mimic branch now honors the task's episode length instead of silently always using 50 s, and get_episode_length_s()'s -> float annotation is now accurate.

Design, Boundaries & Scope

The value 20.0 is now defined twice as DEFAULT_EPISODE_LENGTH_S — once in default_params.py (injected into generated params) and once on TaskBase (the code-level fallback). They serve different paths (generated graph specs vs. hand-authored tasks) but represent the same concept, so if one is changed and the other isn't, generated envs and hand-authored tasks would silently get different defaults. Worth collapsing to a single source of truth (see inline).

Two behavior notes to confirm are intended (both look like the point of the PR, just flagging blast radius): any task/env that previously relied on the 50.0 cfg default now runs at 20 s, and the mimic/datagen path now uses the task's length rather than 50 s. If any first-party env was implicitly relying on 50 s, 20 s may truncate episodes.

Findings

🟡 Warning: isaaclab_arena/agentic_environment_generation/default_params.py:14 — duplicate default constant; see inline comment.

Test Coverage

test_episode_length_s_default_injected_when_omitted covers the injection path well, and the two happy-path tests were updated for the new param. The setdefault "an agent-provided value wins" branch isn't exercised, though — consider a small test that passes episode_length_s in the TaskSpec params and asserts the compiler leaves it untouched (e.g. params={..., "episode_length_s": 45.0} → spec.tasks[0].params["episode_length_s"] == 45.0). No sim tests are needed for this change.

Verdict

Minor fixes needed

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

Moves the episode-length fallback off the env-cfg (50 s) and onto TaskBase (new DEFAULT_EPISODE_LENGTH_S = 20 s), applies it to RL/eval envs while keeping mimic/data-gen at 50 s, and has the intent compiler inject the same 20 s into generated task params. The mechanics are correct and the mimic carve-out is clean. Two things worth a look: the 20 s value is now duplicated as a hand-synced literal across two modules, and this changes the effective default for every eval task that relied on the old 50 s fallback.

Design, Boundaries & Scope

• Duplicated default / single source of truth. The 20 s value now lives as a literal in both task_base.py and default_params.py, reconciled only by cross-referencing NOTE comments — exactly the kind of hand-synced constant that drifts. default_params.py and its package __init__ import nothing Isaac Lab, so task_base can import the constant from there (keeping the pre-sim compiler layer import-free) and drop the second literal. See the inline notes.
• Default-behavior change (blast radius). Previously an omitted episode_length_s fell back to the 50 s cfg default; now it resolves to 20 s for every RL/eval env. This is the PR's stated intent, but it silently shortens any existing eval task that relied on the old 50 s fallback. Can we confirm no long-horizon eval task depended on 50 s — i.e. that 20 s won't truncate legitimate episodes and depress benchmark success rates?

Findings

See inline comments.

Test Coverage

test_episode_length_s_default_injected_when_omitted covers the compiler-injection path (pure-Python, Phase 1 — no sim / inner-outer needed), and the two happy-path tests are updated for the new param. The new TaskBase runtime fallback (20 s when omitted) isn't exercised directly, but it's a one-line default and low-risk. No sim-marker concerns.

Verdict

Minor fixes needed

[qianl-nv]

approved with two nits

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

This makes TaskBase default episode_length_s to 20s (removing the shared 50s
env-cfg default, kept only for the mimic/data-gen path), and additionally teaches
the intent compiler to inject that default into generated task params. The runtime
default change is clean and the mimic carve-out is handled correctly. My main
question is about the compiler-injection half of the PR: it pulls in the opposite
direction from the robolab YAML cleanup and looks redundant with the new default.

Design, Boundaries & Scope

The PR does two things that seem to contradict each other:

• It strips the explicit episode_length_s: 20.0 from the six robolab
  *_linked.yaml graphs as "redundant" now that TaskBase defaults to 20s — i.e.
  hand-authored graphs are made to rely on the implicit default.
• It simultaneously adds compiler code (resolved_params.setdefault(...) +
  DEFAULT_EPISODE_LENGTH_S) so that generated graphs carry the value
  explicitly — the comment even says "matching the robolab env graphs", which is
  exactly what this PR removes from those graphs.

Since episode_length_s flows through build_task_from_specs as a normal task
kwarg, an omitted value already resolves to the TaskBase 20s default — so the
injection doesn't change behavior; it only changes what the serialized artifact
records. That leaves one policy to pick, applied consistently:

• If the TaskBase default is the single source of truth (the direction the YAML
  cleanup takes), then the compiler injection, DEFAULT_EPISODE_LENGTH_S, and the
  new test_episode_length_s_default_injected_when_omitted test can all be dropped
  — leaner, and no second literal to keep in sync.
• If explicit-in-serialized-graph matters (reasonable for reproducibility, since a
  saved eval graph shouldn't silently shift if the code default changes later),
  then keep the injection but don't strip the robolab overrides — otherwise
  generated and hand-authored graphs disagree on the convention.

As written there are also two 20.0 literals (TaskBase.__init__ and
DEFAULT_EPISODE_LENGTH_S) that must be kept equal by hand; if the injection
stays, worth collapsing them to one source of truth (see inline note).

Separately, a blast-radius check worth a sentence in the PR: this shortens the
default eval horizon 50s→20s for every non-mimic task that didn't set it
explicitly. Most first-party envs pass an explicit value, so the practical impact
looks small, but longer-horizon tasks (door/assembly/sequential) that leaned on
the 50s default would now truncate at 20s and could show lower success rates.
Worth confirming none of the eval baselines depend on the old default.

Findings

See inline comments.

Test Coverage

The compiler-injection path gets a dedicated test and the two existing
happy-path assertions are updated — good. If the injection is kept, a test that an
agent-provided episode_length_s overrides the default (the setdefault
semantics) would be worth adding; if it's dropped, the new test goes with it.

Verdict

Minor fixes needed

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

This PR moves the episode_length_s default (50s → 20s) off the env cfg and onto the task layer, injects a 20s default into compiler-generated task params, and drops the now-"redundant" explicit episode_length_s: 20.0 from the robolab linked env graphs. The compiler-injection path is sound, but the runtime default in TaskBase does not actually take effect — so the robolab graphs (and any directly-built task) regress to episode_length_s = None. See the critical finding below.

Design, Boundaries & Scope

Beyond the bug, note the blast radius: this changes the default episode budget for every non-mimic task from 50s to 20s. That is the PR's stated intent, but it more than halves the time limit for any eval/task that relied on the old 50s default — policies that needed >20s to finish will now time out and benchmark numbers will shift. Worth confirming this is intended for all existing tasks, not just the robolab set. (Mimic/data-gen correctly retains 50s.)

Findings

🔴 Critical: isaaclab_arena/tasks/task_base.py — the new 20.0 parameter default is dead code; every task subclass forwards an explicit None, so tasks built without an explicit value end up with episode_length_s = None. This directly breaks the 6 robolab graphs whose explicit 20.0 this PR removed. Details + fix inline.

🟡 Warning: default_params.py:15 — the 20.0 default is now duplicated (compiler constant + inline literal in TaskBase) and synced only by hand. Details inline.

Test Coverage

The new test_episode_length_s_default_injected_when_omitted correctly covers the compiler-injection path. But the actual behavior change — a task built without episode_length_s resolving to 20s — is untested, which is exactly why the regression above slips through. A small test asserting SomeTask(...).get_episode_length_s() == 20.0 (or that the builder path yields a concrete float) would have caught it and guards the fix.

Verdict

Significant concerns — the runtime default doesn't take effect, so the robolab env graphs regress to episode_length_s = None.

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

This PR moves the episode-length default (50s → 20s) into TaskBase and the intent compiler, and strips the now-redundant episode_length_s: 20.0 overrides from the robolab linked env graphs. The compiler-side injection works, but the runtime default does not take effect: every real TaskBase subclass forwards episode_length_s=None, so the new 20.0 default is dead — and with the env-builder is not None guard removed plus the yaml overrides gone, the affected envs now resolve to episode_length_s = None instead of 20s.

Findings

See inline comments. The core issue is the TaskBase default (🔴) and its two downstream consequences.

Test Coverage

The added test_episode_length_s_default_injected_when_omitted covers the compiler-injection path — which works. But there is no test for the runtime default, which is exactly where the bug lives. A test that constructs any subclass without episode_length_s (or builds an env from a robolab *_linked.yaml via build_arena_env_from_graph_spec) and asserts get_episode_length_s() == 20.0 would have caught this regression — worth adding.

Minor: NoTask calls super().__init__() with no args, so scene-only non-mimic envs now default to 20s instead of 50s. Intended?

Verdict

Significant concerns

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

The PR aims to default episode_length_s to 20s via two mechanisms — a new TaskBase.__init__ default and a compiler that injects the value into generated task params — while dropping the 50s env-cfg fallback and the redundant per-task overrides in the robolab linked graphs. As written, neither mechanism actually takes effect, so tasks that don't carry an explicit episode_length_s (including the six robolab envs whose override this PR just deleted) end up resolving to None, not 20s.

Design, Boundaries & Scope

Two independent sources of truth for the same default (TaskBase's inline literal and default_params.DEFAULT_EPISODE_LENGTH_S) that must be "kept in sync by hand" is fragile. default_params.py has no Isaac Lab imports, so once the injection is actually wired up, consider having the single default live there and letting both the compiler and (indirectly) the runtime reference one constant — or, more simply, rely on the TaskBase coalescing default alone and drop the compiler-side injection entirely, since the runtime default already covers the load path. Right now there are three places (20s task, 20s compiler constant, 50s mimic) and none of the 20s paths are connected.

Findings

🔴 Critical: the TaskBase 20s default is dead — every subclass forwards None. All task subclasses (PickAndPlaceTask, SortingTask, OpenDoorTask, GoalPoseTask, PlaceUprightTask, PressButtonTask, AssemblyTask, CloseDoorTask, RotateRevoluteJointTask) declare episode_length_s: float | None = None and call super().__init__(episode_length_s=episode_length_s). TaskBase now stores that value verbatim (self.episode_length_s = episode_length_s) with no coalescing, so the = 20.0 parameter default is never reached — self.episode_length_s becomes None. Combined with removing the episode_length_s: float = 50.0 fallback from IsaacLabArenaManagerBasedRLEnvCfg and the now-unconditional env_cfg.episode_length_s = episode_length_s in the builder, any non-mimic env whose task wasn't given an explicit value gets episode_length_s = None, which breaks env construction. This is exactly the "subclasses forward None, so the param default alone would not suffice" case noted in commit f938d69 — the coalescing default it describes is missing from the code. See the inline suggestion on task_base.py.

🔴 Critical: the six robolab *_linked.yaml files now resolve episode_length_s = None. These linked graphs are loaded directly through build_task_from_specs → _build_task_from_spec → task_class(**params) (arena_env_graph_task_conversion_utils.py); they do not pass through IntentCompiler.compile(). With episode_length_s: 20.0 deleted from the YAML and no runtime coalescing default, the constructor receives no value → None → env fails to build. The YAML removals are only safe once the TaskBase coalescing fix above lands.

🔴 Critical: the compiler never injects episode_length_s, so DEFAULT_EPISODE_LENGTH_S is dead and the new tests fail. IntentCompiler.compile() / _resolve_task_params_to_node_ids() contain no reference to episode_length_s, and DEFAULT_EPISODE_LENGTH_S is imported by nothing (intent_compiler.py imports only INITIAL_STATE_SPEC_ID). TaskSpec has no default for it either. The PR body and commit 45360fe describe an injection step that isn't present in the final diff — it looks like it was dropped during the later self-review commits but the constant and the tests asserting it were left behind. See inline notes on default_params.py and test_intent_compiler.py.

Test Coverage

The three test changes in test_intent_compiler.py (two amended expected-param dicts plus the new test_episode_length_s_default_injected_when_omitted) assert that compile() populates params["episode_length_s"] == 20.0, but no code performs that injection — these will fail (Run tests is still pending). Separately, nothing tests the path the YAML removals now depend on: a task instantiated without episode_length_s should report 20s. A direct regression test — construct e.g. PickAndPlaceTask(...) with no episode_length_s and assert get_episode_length_s() == 20.0 — would have caught the dead-default bug and should be added.

Verdict

Needs rework — the core default mechanism doesn't take effect, and the YAML/env-cfg removals turn the intended 20s into None for the affected envs.

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

The PR aims to make 20s the default episode_length_s for all tasks by moving the default onto TaskBase (keeping 50s for mimic/data-gen) and dropping the now-redundant episode_length_s: 20.0 overrides from the robolab linked env graphs. The intent is sound and the mimic split is reasonable, but as written the new default never takes effect — the common construction path resolves episode_length_s to None, which then lands on env_cfg. See the critical below.

Findings

🔴 Critical — the new TaskBase default of 20.0 is bypassed by every task subclass. PickAndPlaceTask, SortingTask, GoalPoseTask, OpenDoorTask, CloseDoorTask, PlaceUprightTask, PressButtonTask, AssemblyTask, and RotateRevoluteJointTask all declare episode_length_s: float | None = None and forward it verbatim to super().__init__(episode_length_s=episode_length_s). When a task is built without an explicit value — exactly what this PR now does for the six robolab graphs, since the conversion util only passes params present in the YAML — the subclass default None is forwarded, so TaskBase sets self.episode_length_s = None. get_episode_length_s() then returns None, and arena_env_builder writes env_cfg.episode_length_s = None, breaking episode-length computation instead of yielding 20s. An earlier commit in this branch even had a coalescing default for this reason; it was dropped. Fix inline on task_base.py.

Test Coverage

No test exercises the new default. A small regression test that constructs a task with no explicit episode_length_s and asserts get_episode_length_s() == 20.0 (and that a robolab linked graph resolves to 20s) would have caught the None propagation above. Given this changes the default path for all non-mimic envs, that coverage seems worth adding.

Verdict

Significant concerns — the headline default doesn't take effect and the changed YAMLs regress to episode_length_s = None. Worth a small fix + regression test before merge.

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

Moves the default episode_length_s from the env-cfg fallback (50s) into TaskBase (20s), keeps data-gen/mimic at 50s, and drops the now-redundant episode_length_s: 20.0 overrides from the robolab linked graphs. The coalescing lives in TaskBase.__init__ (rather than a param default) precisely because subclasses declare episode_length_s: float | None = None and forward it verbatim — a good catch, and the added data-only regression test covers exactly that path. The refactor is consistent: get_episode_length_s() now always returns a concrete float, the builder unconditionally sets it for non-mimic envs, and mimic retains 50s via its own cfg default. Nice, focused change.

The one thing worth flagging for reviewers is blast radius (by design, but note it): any non-mimic task that previously relied on the implicit 50s fallback now runs 20s episodes. That's the PR's stated intent and mimic/data-gen is unaffected, but tasks that need a longer horizon must now set episode_length_s explicitly.

Findings

🔵 Improvement: isaaclab_arena/tasks/task_base.py:24 — the 20s default is now the framework-wide single source of truth but lives as a bare inline literal; a named module constant would make it greppable and easier to discover.

Test Coverage

test_task_base.py is a data-only unit test (no SimulationApp), so the inner/outer pattern isn't needed and no marker is correct — it lands in Phase 1. It covers both the subclass-forwards-None regression and the explicit-value passthrough. Good coverage for the change.

Verdict

Ship it

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

Moves the default episode_length_s off the RL env-cfg (previously 50s) and onto TaskBase (20s), so any task that doesn't set it explicitly now resolves to a concrete 20s, while the mimic/data-gen cfg keeps 50s. The implementation is correct and tight — the None→20s coalescing in __init__ is genuinely needed because every task subclass declares episode_length_s: float | None = None and forwards it verbatim, and the new regression test guards exactly that path.

Design, Boundaries & Scope

The one thing worth confirming before merge is blast radius (see inline on arena_env_builder.py): this is a default-behavior change for every RL/eval env that relied on the implicit 50s — they now run for 20s. Since episode length gates success in evaluation, this can silently truncate tasks that legitimately needed longer and lower their measured success rate. It's the PR's stated intent, so this is a request to confirm the affected envs were audited, not an objection.

One structural note (not a defect): with the RL cfg's own 50s default removed, IsaacLabArenaManagerBasedRLEnvCfg.episode_length_s now inherits MISSING from ManagerBasedRLEnvCfg, so the builder is the sole thing that fills it for RL/eval envs (the mimic cfg re-declares 50s precisely because the builder never sets it in the mimic branch). Fine as long as the builder stays the only construction path — worth being aware of.

Test Coverage

Good. test_task_base.py is a data-only unit test (no SimulationApp, so the inner/outer pattern correctly isn't needed) covering both the subclass-forwards-None→20s regression and the explicit-value path. New-file copyright year (2026) is correct.

Verdict

Ship it — pending confirmation that eval envs relying on the old implicit 50s default are fine at 20s.