Skip to content

fix(simulation): replay_episode resolves robot_name the way its siblings do - #3624

Merged
cagataycali merged 2 commits into
strands-labs:mainfrom
cagataycali:replay-episode-shares-the-robot-resolver
Sep 14, 2026
Merged

cagataycali merged 2 commits into
strands-labs:mainfrom
cagataycali:replay-episode-shares-the-robot-resolver

Conversation

@cagataycali

@cagataycali cagataycali commented Sep 14, 2026

Copy link
Copy Markdown
Member

What

replay_episode resolves robot_name through the shared SimEngine._resolve_single_robot, the rule run_policy, eval_policy and evaluate_benchmark already apply: None picks the sole loaded robot, an ambiguous scene is refused listing the candidates, and a supplied name is never re-resolved. The private copy in PolicyRunner.replay (robot_name or <first robot>) goes, with its two unit tests.

Why

The copy read the selector by truthiness and picked a default where the siblings refuse. Two consequences, both reported as success:

call scene before after
replay_episode(ds, robot_name="") one robot substituted the only robot and replayed onto it Robot '' not found in sim. Available robots: [...]
replay_episode(ds, robot_name="") two robots substituted the first robot and replayed onto it same refusal, by name
replay_episode(ds) two robots replayed onto whichever robot list_robots() put first, naming it in the success text Multiple robots registered; specify robot_name. Available: ['arm_a', 'arm_b']
replay_episode(ds) one robot resolved the sole robot unchanged

Replay is the one policy surface that drives the actuators from a recording rather than a policy, so the substitution the siblings refuse is a robot the caller never chose being moved. Measured on the fake engine: two frames of recorded actions reached arm_a in an arm_a/arm_b scene with no name given. #3610 closed the same class on evaluate_benchmark; this is the fourth and last surface in SimEngine that takes a robot_name and drives a rollout.

"" now reaches the existing membership check unchanged, so it is reported by name with the loaded set, the same text an unknown name gets. The replay docstrings said "defaults to the first robot in the sim"; they and docs/recording.md now state the shared rule.

Tests

tests/simulation/test_simengine_facade_guardrails.py:

  • test_a_supplied_robot_name_the_scene_lacks_is_reported_by_name gains replay_episode as its fourth row in both scene shapes. The dataset loader is stubbed to fail loudly, so the cell also grades that the refusal precedes the download. Both new rows fail pre-fix (the loader was reached); the six sibling rows are controls.
  • test_replay_episode_ambiguous_multi_robot_refuses_before_commanding_anything: the loader is left reachable and send_action is recorded, so pre-fix this fails on the commanded list (['arm_a', 'arm_a']) rather than on a stubbed import.
  • test_replay_episode_resolves_the_sole_robot_when_name_omitted: control, two frames reach solo.
gate result
new cells, pre-fix policy_runner.py 3 failed / 7 passed
new cells, this head 10 passed
the six touched test files + describe suites 302 passed; the 4 failures are mujoco/pyarrow absent locally and reproduce identically on main
ruff check + ruff format --check clean, 2018 files
mypy on the five touched Python files clean
whole-tree graders (143 of the roster's 145; the 2 left out import lerobot, absent locally) 4623 passed, 70 failed - failure set byte-identical to main on the same 13 files, 0 attributable

Overlap: #3343 also edits simulation/policy_runner.py, in _resolve_success_fn (~line 4222); this change is in replay (~line 3088) and deletes a helper at ~4581, so it is a merge-order question with no shared hunk.

…ngs do

PolicyRunner.replay carried its own copy of the robot resolution that
run_policy, eval_policy and evaluate_benchmark share, and the copy read
`robot_name or <first robot>` where the shared resolver asks whether a
name was supplied at all and refuses an ambiguous scene.

Two consequences, both reported as success. An explicit robot_name="" -
the shape an unset config value arrives in - was read as omitted and a
robot substituted in both scene shapes. And robot_name=None in a
two-robot scene replayed the recorded actions onto whichever robot
list_robots() put first, naming it in the success text: replay is the one
policy surface that drives the actuators from a recording, so the
substitution the siblings refuse is a robot the caller never chose being
moved. Measured on the fake engine: two frames of actions reached arm_a
in an arm_a/arm_b scene with no name given.

replay now calls SimEngine._resolve_single_robot, so a supplied name is
never re-resolved and reaches the existing membership check (reported by
name, loaded set listed), and an ambiguous scene returns the shared
candidate-listing message. robot_name=None in a sole-robot scene is
unchanged. The private helper it replaces had no other caller and goes
with its two unit tests; the replay docstrings and docs/recording.md now
state the shared rule rather than "defaults to the first robot".

The pin adds replay_episode as the fourth row of the table-driven
presence-not-truthiness test (both rows fail pre-fix: the loader was
reached before any refusal) and a behavioural cell asserting the
ambiguous refusal commands nothing (fails pre-fix on the commanded list,
not on a stubbed import), beside a sole-robot control.
@cagataycali
cagataycali marked this pull request as ready for review September 14, 2026 09:42
@cagataycali
cagataycali enabled auto-merge (squash) September 14, 2026 09:42

@yinsong1986 yinsong1986 left a comment

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.

Summary

Removes PolicyRunner.replay's private robot-resolution copy (robot_name or <first robot>) and routes replay_episode through the shared SimEngine._resolve_single_robot, so the fourth and last robot_name-taking rollout surface now applies the same rule as run_policy / eval_policy / evaluate_benchmark: None resolves the sole robot, an ambiguous multi-robot scene is refused with the candidate list, and a supplied name (empty string included) is never re-resolved and hits the existing membership check. This replaces two silent-substitution behaviours that both reported success with explicit structured errors, which is the correct direction under AGENTS.md's "a subset selector is read by membership, never by truthiness" convention, and closes the class #3610 opened on evaluate_benchmark.

What's good

  • Verified locally at c0b97c8d: the three new test cells fail on the pre-fix policy_runner.py (exactly the 3 claimed, 7 controls green) and the touched suites pass at head (114 passed).
  • The ambiguous-scene test grades the actual hazard - it records send_action targets and asserts nothing was commanded - rather than only the error text, and the unknown-name rows stub the dataset loader to fail loudly so the refusal is proven to precede the download.
  • The deleted helper's two unit tests go with it, test_policy_runner_paths.py's no-robots test still passes against the shared resolver's message, and no remaining caller of _require_default_robot exists in the head tree.
  • Docstrings (base.py, policy_runner.py) and docs/recording.md updated in the same change; the new :meth: cross-reference roles resolve (xref grader green at head); changelog fragment named for the PR number is present.

@cagataycali
cagataycali merged commit fd58b40 into strands-labs:main Sep 14, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants