feat(lerobot): publish imports into bucket-backed data roots - #377
feat(lerobot): publish imports into bucket-backed data roots#377VARUN3WARE wants to merge 2 commits into
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:
|
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.