From e4e062a458e9078bd525d07ee742a6681ab9df70 Mon Sep 17 00:00:00 2001 From: Chirag Gupta <103719146+chiruu12@users.noreply.github.com> Date: Mon, 7 Sep 2026 16:56:44 +0530 Subject: [PATCH 1/3] fix(lerobot): stop flooring a fractional source fps --- src/hflow/importers/lerobot.py | 7 ++-- tests/test_lerobot_converter.py | 58 +++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/hflow/importers/lerobot.py b/src/hflow/importers/lerobot.py index 1b3a0d0..6ae8661 100644 --- a/src/hflow/importers/lerobot.py +++ b/src/hflow/importers/lerobot.py @@ -1053,7 +1053,7 @@ def import_lerobot_dataset( episode_index=selected_episode_index, camera_keys=resolved_camera_keys, numeric_schemas=numeric_schemas, - frames_per_second=int(source_archive.fps), + frames_per_second=source_archive.fps, ) ) episodes_converted += 1 @@ -1089,7 +1089,7 @@ def _convert_single_episode( episode_index: int, camera_keys: tuple[str, ...], numeric_schemas: dict[str, _NumericSchema], - frames_per_second: int, + frames_per_second: int | float, ) -> _PublishedEpisode: """Convert a single episode to canonical MCAP and publish it. @@ -1312,6 +1312,9 @@ def _feature_rows(feature_name: str) -> list | None: ) for frame_index in range(frame_count): + # Divided at the source rate, not a truncated one. meta/info.json + # is allowed a fractional fps, and 29.97 floored to 29 stretches + # the time axis by about a second every thirty. log_time_ns = EPISODE_START_TIME_NS + round( frame_index * NANOSECONDS_PER_SECOND / frames_per_second ) diff --git a/tests/test_lerobot_converter.py b/tests/test_lerobot_converter.py index fa1741a..5c38ad2 100755 --- a/tests/test_lerobot_converter.py +++ b/tests/test_lerobot_converter.py @@ -1158,6 +1158,64 @@ def fake_convert( return published_keys +def test_fractional_fps_reaches_conversion_untruncated( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A fractional source fps must not be floored on the way to conversion. + + meta/info.json is allowed a non-integer fps, and NTSC corpora ship 29.97. + Truncating it to 29 stretches the canonical time axis by about a second + every thirty and moves the keyframe interval off the GOP the provenance + record claims. + """ + received_fps: list[object] = [] + + def capture_convert(*, frames_per_second: object, **_kwargs: object) -> prep._PublishedEpisode: + received_fps.append(frames_per_second) + staged = tmp_path / "staged.mcap" + staged.write_bytes(b"episode") + return { + "uri": "file:///landing/lerobot_episode_0001.mcap", + "content_id": prep.content_episode_id(staged), + "size_bytes": staged.stat().st_size, + } + + info = _stub_single_episode_info() + info["fps"] = 29.97 + + monkeypatch.setattr(prep, "_convert_single_episode", capture_convert) + monkeypatch.setattr( + prep, "_hf_repo_info", lambda repo, rev: {"sha": "abc", "license": "apache-2.0"} + ) + monkeypatch.setattr( + prep, + "_ensure_source_archive", + lambda dataset_source, cache_dir: _source_archive( + dataset_source, + cache_dir, + info=info, + episodes=[ + prep._EpisodeRow( + episode_index=0, + task="push", + length=1, + data_chunk="000", + data_file="000", + data_from=0, + data_to=1, + ) + ], + video_keys=[prep.DEFAULT_CAMERA_KEY], + ), + ) + + prep.import_lerobot_dataset( + dataset_repo="fake/repo", revision="abc", output_dir=tmp_path / "out" + ) + + assert received_fps == [29.97] + + @pytest.mark.parametrize( "depth_metadata", [ From a1082f27b5c6a69f98bccf8451e6ac0c66c56883 Mon Sep 17 00:00:00 2001 From: Chirag Gupta <103719146+chiruu12@users.noreply.github.com> Date: Mon, 7 Sep 2026 17:04:58 +0530 Subject: [PATCH 2/3] bump the converter version with the fps change --- src/hflow/importers/lerobot.py | 6 +++++- tests/test_lerobot_converter.py | 11 ++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/hflow/importers/lerobot.py b/src/hflow/importers/lerobot.py index 6ae8661..2885fb5 100644 --- a/src/hflow/importers/lerobot.py +++ b/src/hflow/importers/lerobot.py @@ -55,7 +55,11 @@ # can prove a landing file belongs to this exact selection (#303). Those # fields change the canonical bytes that content_episode_id hashes, so v5 # and v6 outputs must not share a converter identity. -CONVERTER_VERSION = "lerobot-converter-v7" +# "v8": a fractional source fps is no longer floored, so every message log +# time on a corpus declaring one (29.97, say) moves. A v7 file of such a +# corpus carries the stretched time axis, and resume would otherwise accept +# it as completed work. +CONVERTER_VERSION = "lerobot-converter-v8" # Canonical transform knobs that affect published bytes for this importer. IMPORT_GOP_SECONDS = 1.0 # The v3 per-episode aggregate of the collector's frame-level next.success diff --git a/tests/test_lerobot_converter.py b/tests/test_lerobot_converter.py index 5c38ad2..2cd9bd4 100755 --- a/tests/test_lerobot_converter.py +++ b/tests/test_lerobot_converter.py @@ -2215,9 +2215,14 @@ def test_success_label_omitted_when_source_has_no_outcome_feature( def test_converter_version_bumped_with_the_label_support() -> None: - """The label changes episode/v1 bytes, which content_episode_id hashes: - the converter version moves with the change, not after it.""" - assert prep.CONVERTER_VERSION == "lerobot-converter-v7" + """The converter version moves with any change to the published bytes. + + The label changed episode/v1, which content_episode_id hashes. Reading a + fractional fps as declared moves every message log time. Reuse keys on + this stamp, so a version that lags a byte change makes stale output look + like completed work. + """ + assert prep.CONVERTER_VERSION == "lerobot-converter-v8" def test_reuse_refuses_a_landing_episode_with_damaged_payload( From 392857d5d51884838021029e95022a2d9bc21954 Mon Sep 17 00:00:00 2001 From: Chirag Gupta <103719146+chiruu12@users.noreply.github.com> Date: Mon, 7 Sep 2026 19:46:53 +0530 Subject: [PATCH 3/3] pin the log times, the keyframe interval, and the resume refusal --- tests/test_lerobot_converter.py | 122 ++++++++++++++++++++++++++++---- 1 file changed, 107 insertions(+), 15 deletions(-) diff --git a/tests/test_lerobot_converter.py b/tests/test_lerobot_converter.py index 2cd9bd4..2500e7e 100755 --- a/tests/test_lerobot_converter.py +++ b/tests/test_lerobot_converter.py @@ -1979,14 +1979,16 @@ def should_not_convert(**_kwargs: object) -> prep._PublishedEpisode: # --- success label: read the collector's outcome, never invent it (#395) ----- -def _build_success_label_corpus(root: Path, outcome_mode: str) -> dict: +def _build_success_label_corpus( + root: Path, outcome_mode: str, frames_per_second: int | float = 30 +) -> dict: """One two-frame episode. outcome_mode: 'transition', 'all-false', 'empty-aggregate', or 'none'. """ has_outcome = outcome_mode != "none" info = { - "fps": 30, + "fps": frames_per_second, "data_path": "data/chunk-{chunk_index:03d}/file-{file_index:03d}.parquet", "video_path": "videos/{video_key}/chunk-{chunk_index:03d}/file-{file_index:03d}.mp4", "features": { @@ -2076,10 +2078,14 @@ def _build_success_label_corpus(root: Path, outcome_mode: str) -> dict: def _import_success_label_corpus( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch, outcome_mode: str + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + outcome_mode: str, + frames_per_second: int | float = 30, + transcode_calls: list[float] | None = None, ) -> Path: root = tmp_path / "corpus" - corpus = _build_success_label_corpus(root, outcome_mode) + corpus = _build_success_label_corpus(root, outcome_mode, frames_per_second) output_dir = tmp_path / "out" monkeypatch.setattr( @@ -2106,17 +2112,16 @@ def fake_download(url: str, dest: Path, **_kwargs: object) -> None: shutil.copy(root / "data" / "chunk-000" / "file-000.parquet", dest) monkeypatch.setattr(prep, "_download_file", fake_download) - monkeypatch.setattr( - prep, - "_transcode_mp4_to_h264", - lambda mp4_path, gop, fps: ( - [ - b"\x00\x00\x00\x01\x09\x10\x00\x00\x00\x01\x67\x42\x00" - b"\x00\x00\x00\x01\x68\x88\x80\x00\x00\x00\x01\x65\x88" - ] - * 2 - ), - ) + + def fake_transcode(mp4_path: Path, gop: float, fps: float) -> list[bytes]: + if transcode_calls is not None: + transcode_calls.append(fps) + return [ + b"\x00\x00\x00\x01\x09\x10\x00\x00\x00\x01\x67\x42\x00" + b"\x00\x00\x00\x01\x68\x88\x80\x00\x00\x00\x01\x65\x88" + ] * 2 + + monkeypatch.setattr(prep, "_transcode_mp4_to_h264", fake_transcode) monkeypatch.setattr(prep, "_get_video_pts_times", lambda path: [0, 0]) monkeypatch.setattr(prep, "ffmpeg_version", lambda: "test-ffmpeg") @@ -2310,3 +2315,90 @@ def test_reuse_accepts_an_intact_episode_after_the_crc_pass( camera_keys=_MATCHING_CAMERA_KEYS, ) assert damaged is None + + +def test_fractional_fps_sets_the_log_times_from_the_declared_rate( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """29.97 is the rate NTSC-derived capture writes, and it must reach the + time axis unfloored. + + Frame n sits at n / fps seconds. At 29.97 the second frame is 33.3667 ms + in; read as 29 it lands at 34.4828 ms, and the error grows with the frame + index: about a second by frame 900, six seconds by frame 5400. + """ + from hflow.episode import Episode + + output_dir = _import_success_label_corpus( + tmp_path, monkeypatch, "none", frames_per_second=29.97 + ) + landing = sorted((output_dir / "landing").glob("*.mcap")) + with Episode(landing[0]) as episode: + log_times = list(episode.channel("/action").timestamps) + + expected_second_frame = prep.EPISODE_START_TIME_NS + round(prep.NANOSECONDS_PER_SECOND / 29.97) + floored = prep.EPISODE_START_TIME_NS + round(prep.NANOSECONDS_PER_SECOND / 29) + assert log_times[0] == prep.EPISODE_START_TIME_NS + assert log_times[1] == expected_second_frame + assert log_times[1] != floored + + +def test_fractional_fps_reaches_the_transcoder_for_the_keyframe_interval( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The GOP is the half that makes provenance false rather than imprecise. + + _transcode_mp4_to_h264 sets the keyframe interval to + ``round(gop_seconds * frames_per_second)``. At the declared 29.97 that is + 30, at the floored 29 it is 29, so the file carried a 29 frame GOP while + provenance/v1 stamped gop_seconds as 1. #376 is open because that field is + recorded as actually used and never checked; this was one way it could + already be wrong. + """ + transcode_calls: list[float] = [] + _import_success_label_corpus( + tmp_path, + monkeypatch, + "none", + frames_per_second=29.97, + transcode_calls=transcode_calls, + ) + + assert transcode_calls, "the transcoder was never called" + assert transcode_calls[0] == 29.97 + keyframe_interval = max(1, round(prep.IMPORT_GOP_SECONDS * transcode_calls[0])) + assert keyframe_interval == 30 + assert keyframe_interval != max(1, round(prep.IMPORT_GOP_SECONDS * 29)) + + +def test_reuse_refuses_an_episode_written_before_the_fps_fix(tmp_path: Path) -> None: + """The resume half, which is the part a reader would assume rather than check. + + _episode_identity_matches never looks at fps, so a fractional-fps episode + delivered with the stretched time axis still matches on dataset, revision, + episode index, camera keys and gop_seconds. Only the converter version + separates it from a correct one, which is why the bump is what makes the + fix reach an existing landing tree instead of stopping at new imports. + """ + data_root = LocalStorageRoot(tmp_path / "out") + landing = tmp_path / "out" / "landing" / "lerobot_episode_0001.mcap" + _write_identity_matching_landing_mcap( + landing, + dataset_source=_MATCHING_SOURCE, + episode_index=0, + camera_keys=_MATCHING_CAMERA_KEYS, + marker="pre-fps-fix", + episode_record_overrides={"converter_version": "lerobot-converter-v7"}, + source_provenance_overrides={"converter_version": "lerobot-converter-v7"}, + provenance_overrides=None, + ) + + assert ( + prep._try_reuse_completed_episode( + data_root, + dataset_source=_MATCHING_SOURCE, + episode_index=0, + camera_keys=_MATCHING_CAMERA_KEYS, + ) + is None + )