Repository navigation
Conversation
e890bcb to
136fbff
Compare
1acc06b to
032dbab
Compare
bd5b670 to
a158012
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 medium, 7 low, 5 nit.
| errs.append(f"t_start_ns {t0} is not before t_end_ns {t1}") | ||
| if r["recording_id"] not in tasks: | ||
| errs.append(f"recording {r['recording_id']} has no tasks") | ||
| elif not any(s <= t0 and t1 <= e for s, e in tasks[r["recording_id"]]): |
There was a problem hiding this comment.
medium (correctness): This containment check rejects any annotation that covers a recording's last frame. Annotations are half-open, so such a span ends at ts[-1] + period. But aria_to_lance writes the task's t_end_ns as int(ts[-1]) (inclusive), and lance_pilot/commit.py copies that into tasks.lance. migrate-zarr produces exactly these end-of-clip spans, so review flags them and commit refuses the whole file. Please pick one convention: either make the task/alias end half-open in aria_to_lance (ts[-1] + period), or allow t1 <= e + period here. Also make tests/fixtures/lance_tables.py match what aria_to_lance actually writes.
| expected = None | ||
|
|
||
| # Only LocalEpisodeResolver can sync; other resolvers reject the kwarg. | ||
| extra = {"sync_from_s3": True} if sync_from_s3 else {} |
There was a problem hiding this comment.
low (backward-compat): This now passes sync_from_s3=True to every resolver. The old code passed it only to LocalEpisodeResolver. S3EpisodeResolver.resolve, the annotation-cutoff resolver and LanceEpisodeResolver.resolve don't accept the kwarg, so _from_resolver(S3EpisodeResolver(...), sync_from_s3=True) (as used in zarr_data_viz.ipynb) raises TypeError. Either drop the kwarg for resolvers that don't take it, or remove sync_from_s3 entirely and update the notebook. Also fix the comment: LocalEpisodeResolver is the one that ignores the flag.
| def append_chunks(uri: Path, deliveries: list, camera: str, k: int) -> None: | ||
| """Chunk rows for deliveries not yet in the chunk table, each recording's rows contiguous.""" | ||
| have = chunked_ids(uri) | ||
| todo = [d for d in deliveries if d.recording_id not in have] |
There was a problem hiding this comment.
low (data-integrity): Because chunk rows are written before commit.append, a crash between the two leaves chunk rows for an uncommitted recording. A rerun with --reconvert re-encodes the MP4, but this filter keeps the old chunks and also skips the num_frames check. The streaming reader then decodes stale chunks, or fails at train time if the frame count changed. Under --reconvert, delete the existing chunk rows for uncommitted recordings first. Alternatively, check the existing rows' total frames against the delivery and fail on a mismatch.
| from egomimic.rldb.filters import DatasetFilter, PinError, _ensure_dataset_filter | ||
| from egomimic.rldb.leaf import EpisodeLeaf | ||
|
|
||
| # Re-exported: Hydra `_target_`s (MultiDataset._from_resolver) and older imports |
There was a problem hiding this comment.
low (naming): MultiDataset now lives in egomimic.rldb.multi, but all 16 data configs, aria_pilot_lance.yaml included, still target egomimic.rldb.zarr.zarr_dataset_multi.MultiDataset._from_resolver, so a Lance-only run imports the zarr module just to reach it. Point at least the Lance config, and ideally all MultiDataset targets, at egomimic.rldb.multi.MultiDataset._from_resolver. Then trim this re-export block to what external callers still need, and drop _IndexMap and SEED.
| def edit(root: Path, table: str, rows: pa.Table, on: str) -> None: | ||
| """Update columns of existing rows keyed by `on` (label and metadata fixes); every key must exist.""" | ||
| if table not in CATALOG: | ||
| if table not in CATALOG and table != "annotations.lance": |
There was a problem hiding this comment.
low (correctness): edit() now accepts annotations.lance, but the error still says edit only applies to {list(CATALOG)}. Build an editable set that includes annotations (reuse annotations_uri(root).name rather than the literal) and list that set in the message. A merge_insert edit to text or t_*_ns on annotations.lance would also leave annotation_id out of sync with the row's content. Either restrict edits there to the grading columns or say in the docstring that content changes must go through apply_annotations.
| def edit(root: Path, table: str, rows: pa.Table, on: str) -> None: | ||
| """Update columns of existing rows keyed by `on` (label and metadata fixes); every key must exist.""" | ||
| if table not in CATALOG: | ||
| if table not in CATALOG and table != "annotations.lance": |
There was a problem hiding this comment.
nit (data-integrity): edit now accepts annotations.lance, but annotation_id is a uuid5 of the content columns. Editing text or the timestamps here would leave a stale id, which breaks later deletes and the resubmit dedupe. Consider allowing only the grading columns (quality_score, graded_by, rubric_version) for this table. The error message also still says edit applies only to CATALOG; please update it.
| if isinstance(filters, DatasetFilter): | ||
| return filters | ||
| raise TypeError( | ||
| "filters must be a DatasetFilter or None in the zarr resolver path. " |
There was a problem hiding this comment.
nit (naming): Lance now calls this helper too (lance/dataset.py resolve), so the message 'in the zarr resolver path' names the wrong backend there. Drop that phrase. Since two modules import it, consider making it public (ensure_dataset_filter).
| import pyarrow as pa | ||
| import pyarrow.parquet as pq | ||
|
|
||
| from egomimic.rldb.lance.commit import _exists, annotations_uri |
There was a problem hiding this comment.
nit (naming): _exists is now used outside commit.py, and the new aria_to_lance_ray.py repeats the same (uri / "_versions").exists() check at lines 83 and 115. Make it public (e.g. commit.table_exists) and use it in both places.
|
|
||
| have = commit.committed_ids(args.lance) | ||
| vids = [v for v in discover(args.raw_dir) if recording_id("aria", v) not in have] | ||
| delivered = {v for v in vids if (args.deliveries / v / "meta.json").exists()} - ( |
There was a problem hiding this comment.
nit (simplification): Simpler and equivalent: delivered = set() if args.reconvert else {v for v in vids if (args.deliveries / v / "meta.json").exists()}.
| recording_id=rid, | ||
| t_start_ns=0, | ||
| t_end_ns=n, | ||
| t_end_ns=n * 33_333_333, |
There was a problem hiding this comment.
nit (test-quality): Use PERIOD_NS from fixtures.lance_tables (already imported from there) instead of the literal 33_333_333 here and in the alias below.
MultiDataset, EvenStrideDataset, pinned_episode_subset, worker_context and _IndexMap move to egomimic/rldb/multi.py; PinError and _ensure_dataset_filter to rldb/filters.py. zarr_dataset_multi re-exports them so the Hydra `_target_`s (MultiDataset._from_resolver) and older imports keep resolving. Also moved, since both the zarr and Lance paths use them: - zarr/action_chunk_transforms.py -> rldb/transforms.py - zarr/norm_cache.py, zarr/episode_norm_samples.py -> rldb/norm/ - zarr/utils.py (set_global_seed) -> utils/seed.py MultiDataset._from_resolver no longer type-checks for LocalEpisodeResolver: it forwards sync_from_s3 only when set, so a resolver that cannot sync now raises instead of silently ignoring it. The tutorials that passed it to an S3EpisodeResolver (a no-op) drop it. norm_cache hashes both rldb.multi and zarr_dataset_multi as reader modules. code_hash covers module source, so every existing norm cache misses once. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01KY4JqVTaiGrzTaprPEzwZo
LanceRecordingDataset subclassed ZarrDataset only for its keymap windowing, and carried a hand copy of the end of ZarrDataset.__getitem__ that had already drifted (no multi-camera or 3x4 K handling). Both now subclass egomimic/rldb/leaf.EpisodeLeaf, which owns: - the windowing, padding and mask helpers (_read_windows, _history_indices, _action_pad_mask, ...), with _chunk_end_idx still the per-subclass hook; - _finish: masks, transforms, tensors, embodiment, fps, intrinsics and episode_hash for every leaf. A flat 9-value K (how Lance stores it) is now reshaped instead of becoming the NaN sentinel; - the batch contract (plan_sample / fetch_planned / assemble_sample) and a default cache_fingerprint. MultiDataset.__getitems__ batches leaves with batched_fetch set rather than any leaf with a fetch_planned attribute, which every leaf now has. LanceStreamingDataset and format_bench's FastZarrDataset set it. test_leaf_equivalence reads one recording as a Lance leaf and as a zarr episode written from the same rows and checks every sample matches. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01KY4JqVTaiGrzTaprPEzwZo
annotations.lance is one global table keyed by a content-addressed annotation_id, with half-open [t_start_ns, t_end_ns) intervals on the recording clock and in-place grading columns. apply_annotations adds and deletes in one commit; the release tag covers it like every other table. The loader optionally returns all covering texts per frame, with opt-in action clamping and annotated-only sampling. task_instance_id is now deterministic, and the pilot commit's validation is reusable as load_deliveries. The format bench writes its spans to annotations.lance too, since the tasks annotations column and schema.ANNOTATION are gone. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01AeFJsNDQrLsyrDyJ2xgbrW
Converts and chunks recordings as Ray tasks and commits them in batches from the driver; reruns skip committed recordings. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01AeFJsNDQrLsyrDyJ2xgbrW
49123e8 to
b7ceab0
Compare
a158012 to
248e2a0
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
Partial
Not fixed
|
rldb: move MultiDataset and format-neutral pieces out of zarr/
MultiDataset, EvenStrideDataset, pinned_episode_subset, worker_context and
_IndexMap move to egomimic/rldb/multi.py; PinError and _ensure_dataset_filter
to rldb/filters.py. zarr_dataset_multi re-exports them so the Hydra
_target_s (MultiDataset._from_resolver) and older imports keep resolving.Also moved, since both the zarr and Lance paths use them:
MultiDataset._from_resolver no longer type-checks for LocalEpisodeResolver:
it forwards sync_from_s3 only when set, so a resolver that cannot sync now
raises instead of silently ignoring it. The tutorials that passed it to an
S3EpisodeResolver (a no-op) drop it.
norm_cache hashes both rldb.multi and zarr_dataset_multi as reader modules.
code_hash covers module source, so every existing norm cache misses once.
Co-Authored-By: Claude Opus 5.5 (1M context) [email protected]
Claude-Session: https://claude.ai/code/session_01KY4JqVTaiGrzTaprPEzwZo
rldb: EpisodeLeaf base for zarr and Lance leaves
LanceRecordingDataset subclassed ZarrDataset only for its keymap windowing,
and carried a hand copy of the end of ZarrDataset.getitem that had already
drifted (no multi-camera or 3x4 K handling). Both now subclass
egomimic/rldb/leaf.EpisodeLeaf, which owns:
_action_pad_mask, ...), with _chunk_end_idx still the per-subclass hook;
episode_hash for every leaf. A flat 9-value K (how Lance stores it) is now
reshaped instead of becoming the NaN sentinel;
default cache_fingerprint.
MultiDataset.getitems batches leaves with batched_fetch set rather than
any leaf with a fetch_planned attribute, which every leaf now has.
LanceStreamingDataset and format_bench's FastZarrDataset set it.
test_leaf_equivalence reads one recording as a Lance leaf and as a zarr
episode written from the same rows and checks every sample matches.
Co-Authored-By: Claude Opus 5.5 (1M context) [email protected]
Claude-Session: https://claude.ai/code/session_01KY4JqVTaiGrzTaprPEzwZo
rldb/lance: annotations table with review, commit and loader support
annotations.lance is one global table keyed by a content-addressed
annotation_id, with half-open [t_start_ns, t_end_ns) intervals on the
recording clock and in-place grading columns. apply_annotations adds and
deletes in one commit; the release tag covers it like every other table.
The loader optionally returns all covering texts per frame, with opt-in
action clamping and annotated-only sampling. task_instance_id is now
deterministic, and the pilot commit's validation is reusable as
load_deliveries.
The format bench writes its spans to annotations.lance too, since the tasks
annotations column and schema.ANNOTATION are gone.
Co-Authored-By: Claude Opus 5.5 (1M context) [email protected]
Claude-Session: https://claude.ai/code/session_01AeFJsNDQrLsyrDyJ2xgbrW
aria_process: Ray pipeline from raw Aria recordings to Lance tables
Converts and chunks recordings as Ray tasks and commits them in batches
from the driver; reruns skip committed recordings.
Co-Authored-By: Claude Opus 5.5 (1M context) [email protected]
Claude-Session: https://claude.ai/code/session_01AeFJsNDQrLsyrDyJ2xgbrW
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw