Repository navigation
Conversation
| COLUMNS = ( | ||
| "episode_hash, operator, lab, embodiment, rig_name, scene, task, task_description," | ||
| " objects, license, num_frames, zarr_processed_path, zarr_mp4_path," | ||
| " zarr_processing_error, is_eval" |
There was a problem hiding this comment.
medium (dead-code): COLUMNS selects is_eval, which nothing reads, but it leaves out is_deleted. load_rows writes this frame to --snapshot and on the next run evaluates df[~df.is_deleted...], which raises AttributeError, so a snapshot written by this script can never be read back. Either select is_deleted (and drop is_eval), or drop the filter on the snapshot path since the SQL query already excludes deleted rows.
| def load_rows(snapshot: Path | None) -> pd.DataFrame: | ||
| if snapshot and snapshot.exists(): | ||
| df = pd.read_parquet(snapshot) | ||
| return df[~df.is_deleted.astype(bool)].reset_index(drop=True) |
There was a problem hiding this comment.
medium (correctness): --snapshot can't be read back. COLUMNS doesn't select is_deleted, so the parquet written on the first run has no such column, and on the second run df.is_deleted raises AttributeError. The SQL query already filters not is_deleted, so just return pd.read_parquet(snapshot) here (or add is_deleted to COLUMNS).
| if args.lance: | ||
| m = m[~m.recording_id.isin(commit.committed_ids(args.lance))] | ||
| if args.stage_to: | ||
| staged = set(commit.list_dirs(commit.join(args.stage_to, STAGING_SOURCE))) |
There was a problem hiding this comment.
medium (data-integrity): A delivery whose upload died partway (worker killed, or put_file raised, so status failed) leaves <stage-to>/zarr/<id>/ without meta.json. This check still treats it as staged, even with --retry-failed, and the committer ignores it because meta.json is missing, so the episode never migrates. Count an id as staged only if <id>/meta.json exists.
| if not pending: | ||
| break | ||
| finished, pending = ray.wait(pending, num_returns=1) | ||
| res = ray.get(finished[0]) |
There was a problem hiding this comment.
medium (error-handling): If a task dies outside Python (OOM kill, worker crash after max_retries), ray.get raises here and the whole driver exits, losing every in-flight task. No report row is written for that episode, so a resume picks it again and can crash the same way. Wrap ray.get in try/except, keep a ref-to-row map, and write a failed row with repr(e). Also guard the select() report read against an empty file: an empty pd.read_json(..., lines=True) has no episode_hash column.
|
|
||
|
|
||
| def convert(args) -> dict: | ||
| def _value(v): |
There was a problem hiding this comment.
low (duplication): _value and _objects repeat zarr_to_lance._text and zarr_to_lance._objects. The only difference is that _value skips str(). Move one text_or_none/objects_list pair into a shared module (or reuse zarr_to_lance's) so the raw-EVA and zarr routes can't drift apart on how they parse SQL objects.
| return {**md, **{k: _value(sql.get(k)) for k in SQL_FIELDS}} | ||
|
|
||
|
|
||
| def read_manifest(path: Path) -> dict[str, dict]: |
There was a problem hiding this comment.
low (duplication): There are now two read_manifest functions. zarr_to_lance.read_manifest reads CSV only (string values), and this one reads parquet or CSV through pandas (typed values, NaN for blanks). The planner writes manifest.parquet, so zarr_to_lance --manifest can't read its output. Keep one reader (this pandas version) in a shared place and import it from both converters.
|
|
||
|
|
||
| # Values zarr_to_lance.episode_metadata rejects for lab, task_name and operator. | ||
| PLACEHOLDERS = {"", "none", "null", "nan", "unknown", "debug"} |
There was a problem hiding this comment.
low (duplication): PLACEHOLDERS and placeholder copy zarr_to_lance.PLACEHOLDERS and _placeholder, and the comment says they have to match. Import them from zarr_to_lance (make _placeholder public) so the planner can't route episodes that the converter will then reject.
|
|
||
| sample = None | ||
| if args.check_r2: | ||
| both = pd.concat([ok, bad]) |
There was a problem hiding this comment.
low (correctness): ok and bad both have indexes starting at 0, so this concat has duplicate labels. rows.loc[picks] in check_r2 then returns rows from both frames for each label, often from other strata, which inflates and mixes the R2 sample and skews bytes_per_frame, est_zarr_gb and sample_missing_frac. Use pd.concat([ok, bad], ignore_index=True).
| return ok, bad | ||
|
|
||
|
|
||
| def _list(uri: str) -> tuple[int, int]: |
There was a problem hiding this comment.
nit (duplication): _list repeats the pagination in commit._objects. You could write files = commit.list_files(uri); return len(files), sum(s for s, _ in files.values()). download_prefix in zarr_to_lance_ray could iterate commit.list_files too.
| return df | ||
|
|
||
|
|
||
| def _epoch_ms(episode_hash) -> str | None: |
There was a problem hiding this comment.
nit (conventions): aws_sql is already imported at the top, so import episode_hash_to_timestamp_ms there as well. Catch only (ValueError, TypeError) so that an unexpected error isn't quietly treated as a non-timestamp hash and routed to the zarr namespace.
|
|
||
| log = logging.getLogger("zarr_to_lance_ray") | ||
|
|
||
| STAGING_SOURCE = "zarr" |
There was a problem hiding this comment.
nit (duplication): STAGING_SOURCE repeats zarr_to_lance.STAGING_SOURCE. The staging dir used here and in select() has to match the converter's, so import it instead of redefining it.
| work / "out", | ||
| metadata, | ||
| episode_hash=row["episode_hash"], | ||
| **({"ffmpeg": opts["ffmpeg"]} if opts.get("ffmpeg") else {}), |
There was a problem hiding this comment.
nit (simplification): Give --ffmpeg the default zarr_to_lance.FFMPEG (or "ffmpeg"), as the converter's own CLI does, then pass ffmpeg=opts["ffmpeg"] directly instead of the conditional **{...} splat.
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): 4 medium, 4 low, 5 nit.
Comments that could not be anchored to a diff line:
egomimic/scripts/eva_process/eva_to_lance.py:82nit (simplification): This copieszarr_to_lance.PLACEHOLDERS. If the two drift, the planner will accept rows the converter rejects, or the reverse. Import it fromegomimic.scripts.zarr_to_lance(or a shared module), and likewise reuse_objectsineva_to_lanceinstead of redefining it.
eab64d3 to
d5864a2
Compare
72692a6 to
369caa4
Compare
|
Review follow-up (force-pushed). 12 of 13 findings fixed fully and 1 in part:
|
The committer takes Aria's recording and task metadata from SQL, but eva_to_lance read them from the capture's _metadata.json, so the two raw routes could disagree with the episodes table. convert() now takes the SQL row (--manifest: a migration manifest or an app.episodes export) and its lab, operator, task, task_description, scene and objects replace the capture's; a field the row leaves null stays null and the required check rejects it. Without a row the capture's values are used as before. Claude-Session: https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw
docs/zarr_to_lance_migration.md plans converting every non-deleted episode into one Lance root: inventory, destination layout and ids, sharding, worker flow, idempotency, validation gates, cost, rollout, and the raw path for Aria and EVA. plan_zarr_migration reads app.episodes (read-only) and writes a manifest of convertible episodes with their route, converter, recording_id, steps table and work unit, plus the rejected rows with every reason. Aria and EVA migrate from raw only: with --raw-listing, rows whose capture is complete on R2 get the raw route and the rest are rejected (no_raw, incomplete_raw). yam_bimanual (EVA_STEPS, parallel_jaw_gripper) and dexmate_bimanual (DEXMATE_STEPS, dex_hand) are now routed like the vendor zarrs. zarr_to_lance_ray converts and stages the zarr-route rows, one Ray task per episode, resumable from committed ids, staged meta.json files and its report; a task whose worker dies is reported as failed and the run goes on. --direct-videos uploads the MP4s to the Lance root and stages only steps and meta.json. Claude-Session: https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw
d5864a2 to
853c113
Compare
369caa4 to
01cbd79
Compare
The plan and tooling for migrating every non-deleted episode into one Lance root, with the
robot embodiments now routed.
destination layout and recording ids, sharding and work units, worker flow, how
annotations migrate, idempotency, validation gates, cost, rollout, and the raw path for
Aria and EVA. It also notes that MANO weights (
external_ckpts/, gitignored) must shipto the workers.
lists). It writes manifest.parquet (route, converter, recording_id, steps table, shard,
work unit), rejected.parquet with every reason, an inventory and a summary. Aria and EVA
rows migrate from raw only: with
--raw-listing, a row whose capture is complete on R2gets the raw route and the rest are rejected (no_raw / incomplete_raw). yam_bimanual
(EVA_STEPS, parallel_jaw_gripper) and dexmate_bimanual (DEXMATE_STEPS, dex_hand) are no
longer rejected for want of a schema.
episode, resumable from committed ids, staged meta.json files and its report; a task
whose worker dies is reported as failed and the run goes on.
--direct-videosuploadsMP4s straight to the Lance root (commit.stage_direct).
(
--manifest), as the committer does for Aria, instead of the capture's _metadata.json;the converter's required-field check stays.
Planner run v2 (/coc/flash7/scratch/lance_pilot/migration_plan/v2/, SQL read 2026-10-30):
289,734 episodes (514M frames) convertible into 23 steps tables and 4,539 work units.
Zarr route 284,402 (microagi v2 186,188; mecka 41,611; abc yam 36,882; scale 17,090; rl2
yam 2,334; yam 182; sharpa_dex dexmate 115). Raw route 5,332 (aria 2,569, eva 2,763).
335,892 rejected: missing_operator 335,751 (microagi v1 and trace, skipped for now),
no_zarr_path 128, missing_task 8, unknown_embodiment 5, incomplete_raw 4, no_frames 9,
zarr_processing_error 2. 99 are already in lance_v2.
Testing: on a compute node, tests/unit/test_plan_zarr_migration.py (ids, tables, robot
routing, rejections, raw index and raw-only routing) and test_eva_to_lance (SQL row
replaces the capture metadata; a null SQL operator is not filled in): 25 passed. Full unit
suite at the top of the stack, in 4 parallel jobs: 1,565 passed, 5 skipped (3 of the skips are test_mano_fit without weights in the checkout; it passes with them linked in).
🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw