docs: document the LeRobot importer's episode/v1 keys in FORMAT.md - #410
Conversation
|
👋 Hi @alorentiar — thank you so much for your first contribution to HFlow! A maintainer will review your pull request as soon as possible. In the meantime:
💡 Tip: one open pull request per contributor at a time. Issues with an assignee are taken; everything else is fair game. We are excited to have you here and appreciate your help making the project better! 🙌 |
The importer keys sat below *(any user key)*, which reads as a catch-all, so specific keys after it look like a kind of user key: the opposite of what the surrounding prose says. Moved above it, and the prose now names the first key instead of referring to a position. Em dashes are not used in this repo's prose.
kstonekuan
left a comment
kstonekuan
left a comment
There was a problem hiding this comment.
LGTM, merging. Welcome, and this is a strong first PR.
You did the thing the issue was really asking for, which was not "list the keys" but "explain what they are for". Every format claim checks out against the source:
camera_keys json.dumps(..., separators=(",", ":")) compact JSON array lerobot.py:138
gop_seconds f"{IMPORT_GOP_SECONDS:g}" %g-formatted lerobot.py:1239
source_episode_index str(episode_index) decimal string lerobot.py:1236
And the six-key resume contract is exactly the six _episode_identity_matches compares at lerobot.py:186-193, which is the part a reader most needs and the part that was easiest to get subtly wrong.
The gop_seconds paragraph is the best thing here. Documenting why the same key appears in two records, rather than documenting it twice and leaving a reader to wonder which is authoritative, is the harder and more useful choice.
I pushed one fixup, 2e26b31, both parts cosmetic. The new rows sat below the *(any user key)* row, which is a catch-all, so specific keys after it read as a kind of user key: precisely the opposite of what your prose then says. Moving them above it lets the catch-all stay last where it belongs, and the prose now names the first key rather than pointing at a position, so it survives the next key being added. I also dropped the em dashes; this repo's prose does not use them, which is a house style thing you had no way to know.
One judgment call I made rather than asked about: I changed converter_version's description from "currently lerobot-converter-v7" to the form lerobot-converter-vN. The value was accurate this morning and moved twice today (v6 in #390, v7 in #403), and a version pinned in prose that nothing tests is exactly the kind of thing that quietly goes stale. Saying the shape rather than the current value keeps it true.
Gate on the merged result: link check 378 links, 374 OK, 0 errors; ruff check, ruff format --check, ty check clean, 1531 passed / 6 skipped.
Closes #409. If you want another, there are three more starter issues open right now: https://github.com/Hebbian-Robotics/hflow/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22
Two tips that will save you time on a second PR, and neither is a criticism of this one. Only an assignee reserves an issue, so everything unassigned is fair game and a linked PR does not block you. And we cap non-collaborators at one open PR at a time, so a bot will politely close a second and invite you back once this one lands.
|
Flagging a line in this PR that looks unintended: it changes the |
* fix(checks): restore the max_retries type guard The guard went out disabled with #410, so bools and floats were accepted as max_retries. Restore the isinstance check and add a regression test covering True and 2.5, both of which slipped through while it was off. * test(build-ai): split the max_retries cases so ty can read them The shared **kwargs dict inferred as dict[str, str], so splatting it into endpoint, model and max_retries produced seven ty errors. Parametrized instead, with the positive case as its own test, and ty's own suppression comment rather than the mypy-style one, which ty does not honour. --------- Co-authored-by: Kingston <kingston@hebbianrobotics.com>
3 participants
Closes #409
What
docs/FORMAT.mddocumentsepisode/v1as five named keys plus "(any userkey)". The LeRobot importer actually writes eleven:
task,operator,embodiment,success, plus seven importer-only keys that the table does notmention.
Why
Six of the undocumented keys —
source_dataset,source_revision,source_episode_index,converter_version,camera_keys, andgop_seconds—are load-bearing:
_episode_identity_matchesreads them back to decide whetheran already-published landing file can be reused instead of reconverted. A
reader who sees "any user key passes through untouched" could reasonably
conclude these are free-form, and break the resume contract without meaning to.
success_derivationis the key a data consumer actually needs: it names themethodology behind
success(max(stats/next.success): any success framemakes the episode a success). Both keys are absent together when the source
declares no outcome feature.
gop_secondsalso appears inprovenance/v1; for an imported episode bothrecords carry the same value, and the episode copy exists so the resume check
can compare the keyframe interval without opening the transform's record. The
new prose says so explicitly rather than duplicating the row silently.
Changes
episode/v1table, with enoughdetail to reproduce each value's format (
camera_keysis a compact JSONarray of strings,
gop_secondsis%g-formatted,converter_versioniscurrently
lerobot-converter-v7).that six of them are a contract with the importer's resume path rather than
free-form user keys, and how
success/success_derivationrelate.Markdown only — no source or test changes.
Validation