feat(lerobot): publish imports into bucket-backed data roots - #377
Conversation
Accept any parse_storage_root destination for LeRobot import: stage HF cache and MCAP builds locally, publish landing episodes through StorageRoot, write prepared-manifest.json only after every selected episode succeeds, and return real URIs instead of Paths for remotes. Closes Hebbian-Robotics#304.
kstonekuan
left a comment
There was a problem hiding this comment.
The design is right and most of it is well held. One gap to close before this lands.
Nothing tests that episodes publish under landing/. Dropping the prefix from the production key leaves the whole suite green:
landing/ prefix dropped from the published key -> 80 passed, nothing noticed
The cause is that all three publish-path tests monkeypatch _convert_single_episode, and the stub builds the key itself:
def fake_convert(*, storage: StorageRoot, episode_index: int, ...) -> str:
relative_key = f"landing/lerobot_episode_{episode_index + 1:04d}.mcap"
...
return storage.publish(staged, relative_key)So assert episode_uris == [f"{data_root.url}/landing/lerobot_episode_0001.mcap"] is asserting against the test's own string. The landing_relative_key at lerobot.py:897 is never exercised. Stubbing there is the right call for what those tests are about, which is the publish plumbing and the manifest ordering; it just means the layout the plumbing carries has no cover.
That layout is not incidental. The how-to globs landing/*.mcap, examples/lerobot/export.py reads it, and ingest resolves against it, so a wrong key would be a silent break in every consumer rather than a failure here.
There is already a real-conversion call to hang this on. test_video_cache_distinguishes_file_indices_and_reuses_same_source at tests/test_lerobot_converter.py:330 drives the real _convert_single_episode against a LocalStorageRoot with the download and transcode boundaries stubbed, and discards the return value. Asserting that its returned URI ends in landing/lerobot_episode_0001.mcap would close this, or a dedicated test in the same shape if you would rather not overload that one.
Everything else checks out, and some of it is better than the DoD asked.
The cache location is properly held. Moving it out of storage.workspace fails three tests, which is the right number, and LocalStorageRoot.workspace returning self.path is what keeps local imports byte-compatible; I confirmed examples/lerobot/export.py still finds the cache where it expects and its 17 tests pass unchanged.
The manifest-last atomicity test is the strongest thing here. It asserts the second convert was attempted, that the first episode is published, and that the manifest is absent from both the mirror and list_names(). Asserting the partial episode survives is what makes it a real statement about the guarantee rather than a vague "nothing happened".
The list[Path] to list[str] return change is what #304 asked for in as many words ("It doesn't represent a remote object as a Path"), so it is sanctioned rather than incidental. Worth knowing that no in-repo caller reads the return value except the CLI, which you updated.
Gate clean otherwise: ruff check, ruff format --check, ty check, 1442 passed / 6 skipped with current main merged in.
|
Thanks @kstonekuan , New in
This ensures we’re covering the actual Validation:
|
kstonekuan
left a comment
There was a problem hiding this comment.
LGTM, merging.
landing/ prefix dropped from the published key -> 1 failed
test_video_cache_distinguishes_file_indices_and_reuses_same_source
That was 80 passed and nothing noticed before 84747c7. Hanging it on the existing real-conversion call rather than writing a fourth publish test is the right economy: that test already drives the real _convert_single_episode with only the download and transcode boundaries stubbed, so capturing its return costs two lines and covers the production key rather than a stub's copy of it.
Asserting both episode indexes is worth the extra line too, since a key built from a constant rather than the episode index would pass with only the first.
Everything from the previous round still holds: the cache location fails three tests when moved out of storage.workspace, the manifest-last atomicity test asserts the partial episode survives while the manifest does not, and examples/lerobot/export.py still finds the cache where it expects with its 17 tests unchanged.
Gate clean with current main merged in: ruff check, ty check, 1442 passed / 6 skipped.
Closes #304. #303 is the resumable-imports sibling this was designed to compose with, and it is unassigned.
Summary
s3:///gs:///az://data roots forhflow import lerobotlanding/*.mcapprepared-manifest.jsononly after every selected episode succeeds_lerobot_cachein the local workspace/mirror (never upload it)Closes #304.
Why
Bucket-backed workspaces had to import LeRobot locally and upload by hand. This reuses the existing
StorageRootpublish boundary so import can land directly in a durable data root without changing the canonical MCAP layout.Validation
uv run ruff check— passeduv run ruff format --check— passeduv run ty check— passeduv run pytest tests/test_lerobot_converter.py tests/test_storage.py -q— passeduv run pytest -q— 1442 passed, 6 skippedChecklist
uv run ruff check --fix,uv run ruff format, anduv run ty check.