fix(lerobot): read episode metadata from every v3 shard (fixes #293) - #383
Conversation
…n-Robotics#293) Dataset v3 splits meta/episodes into chunk-XXX/file-YYY.parquet shards by size. The importer cached each shard under its basename, so a second chunk directory reusing file-000.parquet looked already downloaded and its episodes vanished while the first shard was read twice. It also discovered video columns and video windows from the first shard only, so every episode in a later shard converted without its window and fell back to the whole chunk video. Keep the tree path below meta/episodes in the cache, refusing entries that would escape it, and read episode rows, video columns, and video windows from one relation over every shard. Bump CONVERTER_VERSION to v5 so outputs prepared under the old behavior do not share an identity with corrected ones. Measured on lerobot/droid_1.0.1 (v3.0, 95,658 episodes, seven metadata shards) with its first two shards: 14,152 of 28,305 episodes had no video window before, 0 after.
|
👋 Hi @akshatpatel64 — thank you so much for your first contribution to HFlow! A maintainer will review your pull request as soon as possible. In the meantime:
💡 Tip: one open pull request per contributor at a time. Issues with an assignee are taken; everything else is fair game. We are excited to have you here and appreciate your help making the project better! 🙌 |
|
CI note: both checks jobs fail on one pre-existing test, test_preview_stats_render_timestamps_in_utc_on_a_non_utc_host. It asserts "-04" not in the bound value, and the runner's date is 2026-09-04, so the date string itself trips it. It fails the same way on main today and passes on any day that is not the 4th. Everything else is green: 1458 passed on 3.11 and 3.14. I kept this PR to #293; if you would rather I fold a one-line fix for that assertion in here instead of a follow-up, say so and I will push it. |
kstonekuan
left a comment
There was a problem hiding this comment.
LGTM, merging. Welcome, and measuring the defect on droid_1.0.1 before fixing it is the right way to open a first PR.
The parametrization is doing real work rather than decorating the test. Each defect fails on its own shard layout:
mutation fails
cache keyed by basename again [chunk-001/file-000.parquet] only
episode rows from the first shard only both layouts
video windows from the first shard only both layouts
traversal refusal removed all four refusal cases
The first line is the point: the collision only exists when a later chunk reuses a basename, so a test built solely on droid's shape (distinct names in one chunk) would have missed it entirely. Keeping the tree path below meta/episodes instead of flattening is the fix for the cause rather than the symptom.
Keep v5. You asked whether to bump or treat the old multi-shard outputs as wrong under v4. Bump. A version constant cannot be conditional on how many shards a corpus happens to have, so the choice is between re-minting single-shard IDs that did not need it and letting a corrected output share an identity with a wrong one. The first costs a re-conversion; the second means a cache hit can serve footage from the wrong episode, which is the bug you are fixing. TRANSFORM_BEHAVIOR_VERSION staying put is right for the same reason: the transform's bytes did not change.
One observation, not a request. Moving column discovery onto the union relation is untested: pointing it back at the first shard leaves the whole suite green, because both shards in the fixture carry the same cameras. Real v3 shards are size-splits of one table and share a schema, so this is defensive rather than load-bearing, and I would not add a fixture asymmetry just to pin it. Worth knowing it is not covered.
Separately, converter_version is not asserted anywhere in the suite, so nothing catches an accidental change to a string that is part of episode identity. That is ours, not yours, and predates this.
Your date-sensitive failure report was right and it was ours: "-04" not in bound_value also matches the day in 2026-09-04 and the month in any April date. Fixed in #385, which credits you and @victorwon2001 for catching it independently. No need to send the one-liner.
Gate on the merged result: ruff check, ruff format --check, ty check clean, 1469 passed / 6 skipped, with the same #385 failure and nothing else.
Closes #293. If you want another, #191 is a good fit for what you have just been through: a reproducible real-camera LeRobot import through quality, curate, and export, and nobody is on it. The other thing worth your time, given you already have droid_1.0.1 in hand, is pointing HFlow at a real corpus and reporting what breaks, what is slow, or what is awkward. #287 came out of exactly that and it is more useful to us than any issue we could write ourselves.
…hes the date (#385) test_preview_stats_render_timestamps_in_utc_on_a_non_utc_host asserted "-04" not in the rendered bound to catch the host's -04:00 offset. The substring also matches the day in 2026-09-04 and the month in any April date, so the test failed on the 4th of every month and through all of April regardless of the code under test. It is failing on main today. The bounds are now compared to the row timestamps and to the wall clock instead. That drops the date collision and covers more: a render that shifted the clock into the host zone while still stamping +00:00 passed both the old substring check and a rows-to-stats comparison on its own, because rows and stats go through the same projection and shift together. Reported independently by @akshatpatel64 (#383) and @victorwon2001 (#384).
Summary
Multi-shard LeRobot Dataset v3 repositories now import every episode with its own video window and its own data. Before this change, two things went wrong once
meta/episodeshad more than one parquet file:lerobot.py:459and:482onmain). Every episode in a later shard converted with no window, so the whole chunk video was transcoded in place of its slice. With real frame counts that shows up as aframe count mismatcherror; when the counts happen to agree, the wrong footage is published.:413). A second chunk directory that reusesfile-000.parquetlooked already downloaded and was skipped, and the first shard was read twice. Two episodes became four published files, one of them stampedsource_episode_index=1with episode 0's task and data, andprepared-manifest.jsoncounted four.Measured on
lerobot/droid_1.0.1(v3.0, 95,658 episodes, seven metadata shards, revision0eabc778), running the first two shards through_ensure_source_archive: 14,152 of 28,305 episodes had no video window onmain, 0 with this change. I measured two shards, not seven; every episode outside shard 0 is affected, so the full corpus would lose roughly 81,500 windows.Why
Both defects come from the same assumption, that
meta/episodesis one file. The fix removes the assumption in the two places it lived:_episode_metadata_cache_pathkeeps the tree path belowmeta/episodesin the cache instead of flattening to the basename. The tree listing is remote input, so an entry that is absolute, contains.., or sits outsidemeta/episodes/is refused with an error that names the repository, matching the other Hugging Face boundary errors in this module.read_parquet([...], union_by_name=true)relation over every shard. That also drops the per-file loop and the post-hoc sort.CONVERTER_VERSIONmoves tolerobot-converter-v5, following fix(lerobot): separate video cache files by index #353. Multi-shard corpora previously published episodes with the wrong window or the wrong source episode under the same output names, so those outputs should not share an identity with corrected ones.TRANSFORM_BEHAVIOR_VERSIONdoes not move: the transform still writes identical bytes for identical input. This does re-mint content IDs for single-shard re-imports such as pusht; if you would rather keep v4 and treat the old multi-shard outputs as simply wrong under it, I will change that.docs/how-to/import-lerobot-v3.md.Tests:
test_index_discovery_reads_every_metadata_shardis parametrized over both shard layouts (distinct basenames in one chunk, which is droid's shape, and the same basename in the next chunk). It asserts each shard downloads once to its own cache entry and is reused on a second discovery, that episode indexes come back as[0, 1, 2, 3], and that every episode carries its own per-camera window. Onmainthe first layout fails on the missing window and the second on the skipped download, so each defect is pinned on its own.test_episode_metadata_cache_path_refuses_entries_outside_the_metadata_treecovers the trust boundary.Alternatives I did not take: running the video-window query per shard works but leaves two places that must agree on the shard list. Deduplicating
episode_rowsbyepisode_indexwould hide the collision instead of fixing it and would mask a genuinely malformed corpus. Reusinghflow.storage._validated_relative_keyfor the path check would work, but its message talks about storage keys. Stale flattened cache files from earlier runs are left alone; they are small and no longer read.Validation
The one failure is pre-existing and unrelated:
test_preview_stats_render_timestamps_in_utc_on_a_non_utc_hostasserts"-04" not in bound_valueand fails on the fourth day of any month because the date itself contains-04. It fails the same way onmaintoday (1 failed, 1447 passed, 11 skipped). Happy to send a one-line fix separately once this PR is closed out, given the one-open-PR rule.Checklist
uv run ruff check --fix,uv run ruff format, anduv run ty check.CONVERTER_VERSIONv4 to v5; canonical format andTRANSFORM_BEHAVIOR_VERSIONunchanged).