Skip to content

Delete no S3 key from a recording sync - #982

Draft
sasha-s wants to merge 3 commits into
mainfrom
sl/lane_pos_upload_safety/recording-sync-no-delete
Draft

sasha-s wants to merge 3 commits into
mainfrom
sl/lane_pos_upload_safety/recording-sync-no-delete

Conversation

@sasha-s

@sasha-s sasha-s commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes Positronic-Robotics/internal#264.

Problem

pos eval run and both data collection entry points (main, main_sim) call pos3.sync(output_dir, sync_on_error=True). pos3 defaults to delete_remote=True. So each sync (every 300 s and at exit) deletes every key under the S3 prefix that the local cache does not hold. That includes episodes that another run uploaded to the same prefix after this run started.

positronic-inference stats only reads a prefix, but it calls pos3.sync(output_dir). At exit it deletes every key that a live run uploaded after the stats download.

Change

  • sync_recording_dir in positronic/dataset/local_dataset.py owns the arguments of a recording sync: sync_on_error=True, delete_remote=False, upload_exclude=[UNFINISHED_MARKER, EDITS_FILE]. cli/eval/run.py and both data_collection.py entry points call it. The platform rollout console uses the same upload_exclude.
  • stats calls pos3.download, so it writes nothing to the prefix.
  • pos3>=0.5.0. pos3 0.5.0 adds upload_exclude (Refresh selected same-size files and configure upload exclusions pos3#16). The bump also brings in 0.4.0: an upload rate cap that is off by default, and a final sync that waits for a running background sync.

Why the two files stay out of the upload:

  • .unfinished: DiskEpisodeWriter writes it when an episode opens and removes it when the episode closes. A background sync during an episode uploads it. With no remote delete, the marker would stay in S3, and every reader would then skip the finished episode.
  • edits.jsonl: other tools append to it in S3. The run's copy from the startup download is stale and differs in size, so pos3 would upload it over the newer remote copy.

upload_exclude does not filter the startup download. An old .unfinished key in S3 downloads into the cache, and the run hides that episode, as every other reader does.

Draft until pos3 0.5.0 is on PyPI

pos3 main (ddebabd) carries version 0.5.0, but the latest release on PyPI is 0.4.0. Until the release, [tool.uv.sources] pins pos3 to git commit ddebabd, with a TODO. When 0.5.0 is on PyPI, the pin goes and uv.lock resolves pos3 from PyPI again. The pos3>=0.5.0 requirement stays as it is.

The three packaging checks fail until then: they install the built wheel from PyPI, where only pos3<=0.4.0 exists.

Known gaps

  • Two writers on one prefix still damage each other, by overwrite. LocalDatasetWriter picks the next episode id once, from its startup download, so two runs upload to the same keys. Telemetry sidecars under telemetry/ have a fixed name per process and are appended, so the later run's upload replaces the earlier run's spans the same way. A per-run subprefix or collision-free ids come later, as a separate design.
  • An aborted episode leaves its files in S3 when a background sync uploaded them before the abort. abort() removes the local directory only, and the mirror no longer deletes remote keys. Readers skip such a directory, because it has no meta.json, and the next run on the prefix numbers its episodes after it. The files stay as storage residue.
  • meta.json is now the only commit point in S3. Before, pos3 deleted the S3 marker only after all uploads in that sync succeeded. Now a reader can see a finished episode whose .mp4 upload failed, until the next sync. A hard kill in that window makes it permanent.
  • delete_local=True stays. If a run's final upload fails, its episodes exist only in the local cache, and the next run on the same machine and prefix deletes them at its startup download. On a clean exit a failed final upload raises; on an error exit pos3 logs Cleanup sync after error failed and the original exception fails the command. A hard kill skips the final sync.
  • Other pos3 call sites keep the default delete: the lerobot and lance converters, the lerobot training scripts, migrate_remote.py and fake_dataset_generator.py. They are out of scope here.
  • The fix holds for a prefix only when every writer to it runs this version.

Tests

An in-memory S3 client (InMemoryS3 in positronic/dataset/tests/utils.py) replaces pos3._Mirror._get_client. One test calls the private _Mirror._sync_uploads to run a background sync while an episode is open; the test says so.

  • test_a_recording_sync_keeps_a_key_that_another_writer_put_after_it_started
  • test_an_episode_that_a_sync_caught_open_lists_as_finished_in_s3
  • test_a_recording_sync_does_not_revert_edits_that_grew_in_s3
  • test_stats_writes_nothing_to_the_prefix_it_reads

Before and after, with the same tests:

Sync arguments keeps other key open episode lists as finished edits not reverted stats writes nothing
main (sync_on_error=True; stats uses pos3.sync) fail pass fail fail
delete_remote=False, no upload_exclude pass fail fail n/a
this PR pass pass pass pass

Local checks on 6920ec67: uv run --locked pytest 4807 passed, 31 skipped; ruff check . and ruff format --check . clean; pre-commit passed at commit.

Revert risk: low. A revert brings back the remote delete. The change touches no stored data format.

🤖 Generated with Claude Code

`pos eval run` and data collection now sync `--output_dir` through `sync_recording_dir`. It sets
`delete_remote=False` and keeps `.unfinished` and `edits.jsonl` out of the upload.
`positronic-inference stats` downloads the prefix and uploads nothing.

pos3 0.5.0 adds `upload_exclude`. Until it is on PyPI, `pos3` comes from a git pin of `ddebabd`.

Refs Positronic-Robotics/internal#264

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
sasha-s and others added 2 commits October 21, 2026 10:33
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…fety/recording-sync-no-delete

# Conflicts:
#	positronic/cli/eval/run.py
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.

1 participant