Repository navigation
Conversation
66443e0 to
de074f0
Compare
6c62f33 to
5c415cf
Compare
546c4fa to
dee1b2c
Compare
RyanPCo
left a comment
RyanPCo
left a comment
There was a problem hiding this comment.
Automated adversarial review (read-only; findings were checked by a second skeptical pass before posting): 1 low, 2 nit.
| ] | ||
| return _union(parts) if parts else None | ||
|
|
||
| copied = {n: union(n) for n in COPIED_TABLES} |
There was a problem hiding this comment.
low (data-integrity): --drop-orphan-steps drops steps rows of recordings missing from recordings.lance, but tasks, aliases and annotations are copied wholesale. Rows written by the same crashed append (aliases and tasks are written before recordings) therefore carry over and reference recordings that do not exist in the new root, and verify does not check for this. Filter the copied tables to recording_id in new_rec, or fail the plan when they reference unknown recordings. Report the count in the dry run and add a containment check to verify.
| episode_hash found twice fails the run. The legacy tasks.annotations column is dropped | ||
| (--dry-run counts the rows that carry one). Sources are never written. | ||
|
|
||
| Lance history does not carry over: every new table starts at version 1 and is tagged |
There was a problem hiding this comment.
nit (docs): The docstring says every new table starts at version 1, but drop_task_annotations (drop_columns) and create_scalar_index both add a version before tagging. Suggested wording: 'every new table starts fresh and the migrated version is tagged --tag'. Line 5 also says the recording_id hash input is 'aria/', but EVA uses 'eva/'; say '
| COPIED_TABLES = ("tasks.lance", "aliases.lance", "annotations.lance") | ||
|
|
||
|
|
||
| def new_steps_uri(root, path: str) -> str: |
There was a problem hiding this comment.
nit (dead-code): new_steps_uri is just join(root, path). Drop it and call join directly.
The schema is moving to `lab` = collecting lab (old `site`), a required
`end_effector` column, and steps tables keyed steps/{lab}/{embodiment}/{end_effector}.lance.
The pilot roots were written with lab = source format and steps/{aria,eva}.lance,
so they need a one-off copy into the new layout before the new readers can use them.
The script merges one or more old roots (--src, repeatable) into a new root, never in
place: it splits every old steps table by its recordings' new key in source row
order, unions tasks/aliases/annotations/changes, fails on recording_id or alias
episode_hash collisions across sources, drops the legacy tasks.annotations column
(dry-run counts the non-empty rows), copies videos and other files as is (video_path
is root-relative), maps changes rows to the new key in one function, refuses null
sites unless --lab-for-null-site is given, and verifies row counts and recording
placement after writing. Lance history is not carried over; new tables are tagged
instead. NEW_RECORDINGS_COLUMNS must match schema.RECORDINGS once that commit lands.
Claude-Session: https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw
The schema commit landed, so the migration takes the recordings columns from schema.RECORDINGS (null content_digest for roots that predate it), end_effector values from END_EFFECTOR and table paths from steps_path instead of local copies. Changes rows follow the landed design: one row per old row, the old `lab` becomes `source`, and `steps_tables` lists the steps tables its recordings live in ([] for annotation commits). Claude-Session: https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw
The aria and eva pilot roots were merged into /coc/flash7/scratch/lance_pilot/lance_v2 with the new lab/end_effector schema and per-key steps tables; the old roots use the pre-migration schema the readers no longer accept. Claude-Session: https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw
The old roots keep the pre-migration schema, which the stack no longer reads. Claude-Session: https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw
dee1b2c to
bd32eea
Compare
|
Review findings: status after the fixup pass. The fixes are squashed into this PR's commits, and the whole stack was force-pushed. Fixed
|
The pilot roots were written with
lab= source format ("aria"/"eva"),site=collecting lab, and steps/{aria,eva}.lance. The tables now have one
labcolumn (thecollecting lab), a required
end_effector, and steps tables atsteps/{lab}/{embodiment}/{end_effector}.lance, so the pilot roots need a one-off copy
into the new layout before the new readers can use them.
egomimic/scripts/lance_pilot/migrate_lab_end_effector.py merges one or more old roots
(--src, repeatable) into a new root (--dst), never in place:
sitebecomeslab, end_effector comes from the oldlab(aria ->hand, eva -> gripper). Null sites fail unless --lab-for-null-site is given.
end_effector) in source row order. Steps rows of recordings missing from
recordings.lance need --drop-orphan-steps.
alias episode_hash found twice fails the run. recording_id is unchanged (its hash
input stays "aria/").
labbecomessource, andsteps_tableslists the steps tableseach batch's recordings now live in ([] for annotation commits).
one.
Lance history is not carried over: every new table starts at version 1 and is tagged
--tag (default migrate-lab-end-effector). Old tags are listed and dropped; changes.tag
keeps naming them.
Run on the two pilot roots (/coc/flash7/scratch/lance_pilot/lance and lance_eva) into
/coc/flash7/scratch/lance_pilot/lance_v2 (5.5G); the source roots are unchanged:
same as the old steps/aria.lance and steps/eva.lance.
The branch also points aria_pilot_lance.yaml, eva_pilot_lance.yaml and the lance_pilot
bench/check scripts at lance_v2; the old roots keep the old schema, which the stack no
longer reads.
Testing: on a compute node, test_lance_migrate_lab_end_effector, test_lance_commit and the
pilot cases of test_data_configs_compose pass (25 passed). LanceStreamingResolver reads
frames from lance_v2 for 34 aria human_bimanual and 49 eva_bimanual recordings.
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw