Skip to content

feat(lerobot): publish imports into bucket-backed data roots - #377

Merged
kstonekuan merged 2 commits into
Hebbian-Robotics:mainfrom
VARUN3WARE:feat/lerobot-import-bucket-roots
Sep 3, 2026
Merged

feat(lerobot): publish imports into bucket-backed data roots#377
kstonekuan merged 2 commits into
Hebbian-Robotics:mainfrom
VARUN3WARE:feat/lerobot-import-bucket-roots

Conversation

@VARUN3WARE

Copy link
Copy Markdown
Contributor

Summary

  • Accept local paths and s3:// / gs:// / az:// data roots for hflow import lerobot
  • Stage Hugging Face downloads and MCAP builds locally; publish only landing/*.mcap
  • Publish prepared-manifest.json only after every selected episode succeeds
  • Keep _lerobot_cache in the local workspace/mirror (never upload it)
  • Return real episode URIs; concise CLI errors for missing bucket deps/auth

Closes #304.

Why

Bucket-backed workspaces had to import LeRobot locally and upload by hand. This reuses the existing StorageRoot publish boundary so import can land directly in a durable data root without changing the canonical MCAP layout.

Validation

  • uv run ruff check — passed
  • uv run ruff format --check — passed
  • uv run ty check — passed
  • uv run pytest tests/test_lerobot_converter.py tests/test_storage.py -q — passed
  • uv run pytest -q — 1442 passed, 6 skipped

Checklist

  • I added or updated outcome-focused tests for changed business logic.
  • I updated documentation for changed behavior, flags, formats, or requirements.
  • I ran uv run ruff check --fix, uv run ruff format, and uv run ty check.
  • I ran the relevant pytest suite.
  • I did not add recordings, generated media, credentials, private URLs, or runtime artifacts.
  • I preserved stored-data compatibility or documented an explicit version change.

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 kstonekuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@VARUN3WARE

Copy link
Copy Markdown
Contributor Author

Thanks @kstonekuan ,
good catch. I fixed the test so it now asserts the real published key layout.

New in 84747c7:

  • tests/test_lerobot_converter.py now captures the return value from the real _convert_single_episode call (with download/transcode stubbed) and asserts it ends with:
    • landing/lerobot_episode_0001.mcap
    • landing/lerobot_episode_0002.mcap

This ensures we’re covering the actual landing/ publish-path carried by the converter/plumbing (not the stubbed relative key).

Validation:

  • uv run pytest tests/test_lerobot_converter.py -q

@kstonekuan kstonekuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kstonekuan
kstonekuan merged commit dceaafb into Hebbian-Robotics:main Sep 3, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow LeRobot imports to publish into bucket-backed HFlow data roots

2 participants