fix(sim): a single-rollout run_policy says how many frames sit in the open episode and that it closed none - #3799
Conversation
… open episode and that it closed none
Through the agent tool, start_recording -> run_policy -> run_policy ->
stop_recording saves ONE 20-frame episode where the caller meant two.
run_policy closes no episode: frames buffer into the open one until
save_episode, reset or stop_recording. That merge is documented in the
docstring and in a logger.info the agent never sees, and the result text said
only "Policy complete on 'so101' … 10 steps". start_recording's advice said
"run_policy (one call per episode)", and the remedy it named - save_episode -
is Python-only: "Action 'save_episode' is not available to an agent … deliberately
not published in tool_spec". So was verify_dataset_episodes, the verb the
docstring sends the caller to for confirming the count.
The single-rollout path now appends a Recorder line to the result while a
dataset recording is active:
Recorder: +10 frames buffered in the open episode (10 unsaved). run_policy
closes no episode: the next rollout appends to this one unless reset runs
first (reset saves the open episode, then starts a new one), or pass
n_episodes=N in ONE run_policy call for N distinct episodes. stop_recording
saves whatever is open. (From Python, save_episode also closes it.)
and, when frames from an earlier rollout were already open:
Recorder: +10 frames APPENDED to the open episode, which already held 10
from an earlier rollout - now 20 unsaved frames in ONE episode. …
The json block gains episode_open_frames and episode_merged_prior_frames next
to the existing n_episodes_* / episode_flush_deferred fields. Not recording
adds nothing. reset and n_episodes are named because they are the two remedies
in the published action enum / parameter set; save_episode is named as the
Python route it is.
start_recording's advice names the boundary (reset between rollouts or
n_episodes=N in one call, else consecutive rollouts merge into one episode)
instead of "one call per episode" and the unpublished verb.
tests/simulation/test_run_policy_names_the_open_episode_while_recording.py
(5, fake recorder so no lerobot is needed): first-rollout line + fields; second
rollout says APPENDED and names the prior count; a closed episode restarts the
count; not recording adds no line and no fields; the remedies the line names
are in _PUBLISHED_ACTIONS / _PUBLISHED_PARAMS. tests/simulation selection
(episode/record/run_policy/contract/advice) 2,872 pass; mypy clean.
…r graded The file's docstring claimed "start_recording's advice names the boundary rather than 'one call per episode'", but no cell asserted it: reverting recording.py to main's wording left all 5 cells green, so the advice change was shipped unpinned. Adds test_start_recording_advice_names_a_boundary_an_agent_can_dial, which reads the success text out of the source with ast (as the sibling grader test_recording_advice_names_every_route_that_captures does, so no dataset and no lerobot are needed) and asserts the advice no longer promises "one call per episode", names the consequence and the two published boundaries, and names neither save_episode nor verify_dataset_episodes - the two verbs absent from _PUBLISHED_ACTIONS that an agent would be refused for calling. Also corrects the module docstring: the defect in the advice is that "one call per episode" is false, not that the advice named save_episode - main's advice text names it zero times. save_episode is reached from run_policy's docstring and a logger.info, neither of which an agent sees.
The fragment was numbered 600 for the harness issue that requested the change, which points nowhere in strands-labs/robots.
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
When a single-rollout run_policy runs during a dataset recording, the result now tells the caller how many frames sit in the open (unsaved) episode, says APPENDED and names the prior count when an earlier rollout already left frames there, and points at the two boundaries an agent can actually reach (reset between rollouts, n_episodes=N in one call) while labelling save_episode as Python-only. Two new additive JSON fields (episode_open_frames, episode_merged_prior_frames) carry the same signal machine-readably, and start_recording's advice drops the false "one call per episode" promise. Recorder behaviour is unchanged - only what the caller is told. The implementation is defensive where it needs to be: _open_episode_frames() re-checks _is_recording() and reads episode_frame_count via getattr(..., 0), so the base _active_recorder() -> None path and backends whose recorders lack the counter degrade to a zero count instead of crashing; the new fields and text line are gated on recording, so non-recording callers see byte-identical results.
What's good
- The remedies named in the new text are cross-checked against
_PUBLISHED_ACTIONS/_PUBLISHED_PARAMSin the tests, so the advice can't drift into naming a verb the tool refuses. - The previously-ungraded
recording.pyadvice edit is now pinned by anast-based source-reading test, matching the existingtest_recording_advice_names_every_route_that_capturespattern (no dataset or lerobot needed). - All new user-facing strings are plain ASCII, tests use
monkeypatch.setattr, and the changelog fragment ships in the same PR - consistent with AGENTS.md conventions. - Mutation-tested (6/6 mutants caught) with a clean two-way
commagainst the main-worktree failure set.
…b the stop_policy resolution-cell fix (strands-labs#3798)
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
When a single-rollout run_policy (n_episodes=1) executes during a dataset recording, the result now carries a Recorder: text line and two additive JSON fields (episode_open_frames, episode_merged_prior_frames) telling the caller how many frames sit in the open unsaved episode, flagging APPENDED when a prior rollout already left frames there, and naming only remedies the agent can actually invoke (reset between rollouts, n_episodes=N in one call) while labelling save_episode as Python-only. start_recording's advice drops the false "one call per episode" claim. Recorder behaviour is unchanged - only what the caller is told - and the new fields are additive to the result payload, so nothing here is a one-way door.
What's good
- All 6 new tests pass on the head SHA; the AST-based advice test pins the
recording.pywording change that was previously ungraded, and the_PUBLISHED_ACTIONS/_PUBLISHED_PARAMScross-checks keep the advice honest against the actual tool spec. _open_episode_framesdegrades safely (getattr(..., 0)on aNonebase-class recorder) and_append_texthandles a missing/emptycontentlist, so the non-recording and error paths cannot crash.- Product strings are plain ASCII, no host paths in the test file, changelog fragment included - AGENTS.md hygiene throughout.
- Scope discipline: 55 product lines, the rest is the pinning test and the fragment; the mutant table in the description shows every line is graded.
Pull request was closed
Through the agent tool,
start_recording->run_policy->run_policy->stop_recordingsaves ONE 20-frame episode where the caller meant two.run_policycloses no episode; frames buffer into the open one untilsave_episode,resetorstop_recording. The merge was documented in the docstring and in alogger.infothe agent never sees, and the result text said only "Policy complete ... 10 steps".Measured on Thor (MuJoCo
so101, mock policy, 10 steps/rollout @ 10 Hz), same probe run in agit worktreeatupstream/mainand on this branch:episode_open_framesepisode_merged_prior_framesPolicy complete ... 10 steps+10 frames buffered in the open episode (10 unsaved)+10 frames APPENDED to the open episode, which already held 10 from an earlier rollout - now 20 unsaved frames in ONE episode2010Recorder truth in both trees: 10 open frames after rollout 1, 20 after rollout 2, 0 episodes saved. Behaviour is unchanged - only what the caller is told.
The remedies have to be ones an agent can dial
_PUBLISHED_ACTIONS?resetn_episodes=N_PUBLISHED_PARAMS)save_episodeverify_dataset_episodessave_episodeandverify_dataset_episodesare absent from the published enum ("deliberately not published in tool_spec"), so advice naming them as the remedy sends an agent into a refusal.start_recording's advice now names the boundary instead of the false "run_policy (one call per episode)" - a call is not an episode.Correction to the original report
The finding said the advice named
save_episode. It does not - main'sstart_recordingtext names it zero times (grep -con the success literal). Thesave_episodedead-end is reached fromrun_policy's docstring and thelogger.info, neither of which an agent sees; the advice's own defect is simply that "one call per episode" is false. The line this PR adds is therefore the first agent-visible mention ofsave_episode, and it labels it Python-only.An unpinned change, now pinned
The
recording.pyadvice edit was shipped ungraded: reverting it to main's wording left all 5 original cells green. Added a 6th cell that reads the success text out of the source withast- the shapetest_recording_advice_names_every_route_that_capturesalready uses, so no dataset and no lerobot are needed.6 cells, 6/6 mutants caught (pre-fix control: 4 F / 1 P - the survivor is the not-recording cell, trivially true when no line exists):
recording.pyadvice wordingopen_before = 0(never says APPENDED)_open_episode_framesreturns 0_append_textno-opopen_beforeafter the rollout (ordering)Gate
tests/simulation+tests/simulation/mujoco-k "episode or record or run_policy or contract or advice": 2758 P / 140 F vs 2752 P / 140 F on a cleanupstream/mainworktree. Two-waycommon the FAILED-id sets is empty both ways - delta is exactly my 6 cells. All 140 are pre-existing lerobot drift (No module named 'lerobot.utils.feature_utils', lerobot 0.5.1 on this node); clean main fails identically, and no failing id names anything this PR touches.ruff check+ruff format --check strands_robots tests tests_integ: clean, 2133 files.mypy strands_robots tests tests_integ: 28 errors in 12 files = the clean-main baseline; 0 in the three files touched.check_changelog_fragment.pyexit 0,assemble_changelog.py --checkOK.check_merge_base_overlap.pynames fix(simulation/isaac): Isaac Sim backend #3343, fix(hardware_robot): the approval prompt says how long the arm may move, and whether the words matter #3766, fix(sim): evaluate_benchmark names a benchmark written for another robot first, by id, with the robot's name #3795 onsimulation/base.py- all different changes (Isaac backend, approval prompt, benchmark mismatch); none touch the single-rollout recorder path, so this is merge-order only.LOC delta +248 / -2, of which 181 are the test file and 8 the fragment; 55 lines of product (a check, a describer and a text-append helper).
Closes #600