fix(lerobot): stop flooring a fractional source fps

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
@@ -1053,7 +1057,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 +1093,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 +1316,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
                 )

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",
     [
@@ -1921,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": {
@@ -2018,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(
@@ -2048,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")
 
@@ -2157,9 +2220,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(
@@ -2247,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
+    )