fix: eliminate export-time episode truncation and camera-ahead-of-action violations - #1
Open
shamreen20 wants to merge 12 commits into
Open
fix: eliminate export-time episode truncation and camera-ahead-of-action violations#1shamreen20 wants to merge 12 commits into
shamreen20 wants to merge 12 commits into
Conversation
nova-data-cli has no incremental-append mode and is single-threaded per invocation, so exporting ~1000 episodes sequentially after collection wastes hours. pipeline.sh pulls (remote SSH/rsync or a local dir) and exports in parallel across workers sized from the machine's own RAM/cores, while collection is still running. Workers dynamically claim recordings via atomic mkdir, commit via a single atomic rename that doubles as the durable "done" record (closing a duplicate- export race found and fixed during testing), and bisect a failing batch instead of quarantining healthy batch-mates alongside a bad recording. Per-worker memory is capped via a systemd-run cgroup (MemorySwapMax=0) after an OOM-killed rerun subprocess was found to be the cause of an earlier corrupted export. merge_batches.py gains an atomic-output guarantee and a pre-merge video-encoder compatibility check that lerobot's own aggregate_datasets skips. Full design rationale in tools/AGENT.md. Test suite under tools/tests/ exercises both acquisition modes, live-feed and backlog-only, crash/restart, and a real end-to-end smoke test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
role_worker exited for good on a single empty candidate scan once COLLECTION_DONE existed, with no tolerance for a transient glitch in that one scan. Observed in production: worker w3 exited claiming no candidates while ~800 unclaimed recordings were still sitting right there, apparently under system load (4 CPU-heavy nova-data-cli decodes competing for cores) - a single false-empty scan permanently dropped a quarter of the throughput with no error signal. role_supervisor's drain loop had the same shape and a worse failure mode (would merge before everything was actually exported). Both now require repeated confirmation (3 consecutive empty scans for a worker, 2 clean passes for the supervisor) before trusting "nothing left", matching the pattern role_acquire's idle-detection already used. Also adds a final report (recordings found/exported/quarantined, episodes in the merged dataset) and a nonzero exit code when anything's quarantined, so a systemic failure doesn't silently look like a clean run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
top_up_workers was only ever called once at the start of the collection phase (the supervisor then blocked on `wait "$acquire_pid"` until collection fully finished), so the pool was permanently stuck at whatever system memory allowed at that single instant. Combined with the backlog cap only applying post-collection, a run could spawn its full memory-supported worker count immediately and leave most of it idle for the rest of a long collection if candidates matured slower than the pool could consume them. Now top_up_workers polls every 60s throughout collection too, and caps new spawns against the live unclaimed backlog unconditionally (safe now that a transient empty read just delays one poll instead of blocking forever). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… metadata Design spec (verified against the installed lerobot package, LeRobot policy configs, and real community datasets like DROID and AgiBot World 2026) plus the resulting task-by-task implementation plan for two optional exporter features: per-episode language instructions (config.task_field, sourced from meta.json, falling back to the existing dataset-wide task_description) and a fix generalizing episode_metadata to any JSON scalar type instead of float-only with a wrong 0.0 default for missing fields. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds two optional, backward-compatible exporter capabilities for VLA training datasets (LeRobot export head): - config.task_field: names a meta.json field (e.g. "task") holding each episode's own natural-language instruction. Resolved per episode and written via LeRobot's native frame["task"]/task_index mechanism, so different episodes can carry different instructions without inventing a parallel column. Falls back to the existing task_description when unset or missing for a given episode. - episode_metadata now accepts any JSON scalar (str/float/int/bool) instead of float-only, and defaults a missing field to null instead of the previously-wrong 0.0 default (e.g. a string field like cube_color). Both default to today's exact behavior when unset. Verified end-to-end against a real recording (task instruction and cube-position metadata round-tripped correctly into the exported dataset). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reuses the existing lerobot_export.json in pick_and_place_imitation_learning (no new config file) and this machine's paths, so the pipeline can be run with no PIPELINE_* env vars for this dataset. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
setsid detaches the supervisor from the controlling terminal, so Ctrl+C's SIGINT never reached it -- the shutdown trap was correctly written but never invoked. An outer wrapper now stays attached to the terminal and forwards INT/TERM into the detached process group. Also fixes a second bug this surfaced: the shutdown trap's own `kill -- -$$` re-signals itself (same pgid), re-entering the same trap forever instead of exiting. Trap now disarms itself before killing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ion violations Two real defects found and fixed in the export pipeline, verified against real recordings from pick_and_place_v4_1000: 1. signal_change trimming (_apply_trimming) compared its threshold against a per-sample joint-position delta, but the default threshold (0.01) sat above the 99th percentile of real per-sample motion for this robot/log rate. As a result, trimming matched almost nothing but the fastest samples (typically only the final retreat), collapsing episodes to a tiny fragment (as little as 1.6s of a true ~40s Leg B). Fixed the example configs to default to all_present trimming, lowered the threshold as a fallback default, and added a guard that warns loudly (with the observed signal statistics) whenever signal_change trimming keeps less than half the raw episode span, instead of silently shipping a fragment. 2. _query_action_state resampled action/state onto the export's fixed-rate grid via nearest-neighbor, which could select a sample from AFTER the grid timestamp. Because /actions_target has real dropouts (tens to hundreds of ms), this let some exported frames pair a camera timestamp with a numerically later action sample - i.e. camera-ahead-of-action in the exported data, undermining the action-lead invariant the collector otherwise guarantees at recording time. Replaced nearest-neighbor with strictly causal selection (latest sample at or before grid time), which cannot select a future sample. Verified on 5 real re-exported episodes: violations dropped from 425/9168 frame-camera pairs (4.6%, up to 331.7ms) to 3/9168 (0.03%, all attributable to genuine raw camera dropouts, not the export logic). Re-exporting 4-5 real episodes before/after confirms both fixes: exported duration now matches the raw HOME-to-HOME Leg B span (previously as low as 3.9% of it), and the exported action column exactly matches the raw /actions_target value at its causal grid selection. Test suite: 102 passed, 3 skipped.
get_frame_at/get_frame_index picked the nearest decoded frame to each grid timestamp, ties going to the earlier frame. That nearest-frame rule could still select a camera frame *before* the target time, re-opening the same camera-ahead/behind-of-action class of bug that 5fc15af just fixed on the action/state side. Both FrameCache.get_frame_index and VideoDecoder.decode_at now select the earliest frame at or after each target timestamp instead of the nearest one. Targets past the last decoded frame still clamp to it (now logged as a warning, since it means real target coverage was short). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the change here