Repository navigation
Conversation
6373ee6 to
e87595e
Compare
e87595e to
68b01e7
Compare
032dbab to
8902fdb
Compare
68b01e7 to
8023eba
Compare
8902fdb to
bd5b670
Compare
8023eba to
499170f
Compare
bd5b670 to
a158012
Compare
499170f to
012b637
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 high, 1 low, 3 nit.
| filters = _ensure_dataset_filter(filters) | ||
| cat = self.catalog() | ||
| keep = cat[[filters.matches(row) for row in cat.to_dict("records")]] | ||
| recs = self.recordings() |
There was a problem hiding this comment.
high (correctness): recs is now the bare recordings table, which has no episode_hash column. LanceStreamingResolver._leaf still calls _chunks_of(t, row.recording_id, kwargs["num_frames"], row.episode_hash), so every streaming resolve raises AttributeError. Pass kwargs["name"] (or row.recording_id) there, and add a non-env-gated unit test that resolves the fixture tables with LanceStreamingResolver.
| _, _, tbl = self._open(name) | ||
| return tbl.to_table(columns=columns).to_pandas() | ||
|
|
||
| def dataset_keys(self, hashes) -> dict[str, str]: |
There was a problem hiding this comment.
low (duplication): aliases.lance is read three times per resolve (_ranges for pins, _ranges for excludes, the clips groupby) and once more in dataset_keys. dataset_keys and _ranges also each carry their own copy of the 'a hash is a recording_id or an aliased clip' rule. Suggest reading the table once (e.g. a cached _aliases() frame with all four columns) and building dataset_keys, _ranges and clips from it.
| n, | ||
| ) | ||
| anchors = _intervals(mask & eligible) | ||
| task_text = [ |
There was a problem hiding this comment.
nit (naming): The local task_text is a list of every task's text, but the leaf gets task_text=task_text[0]. Please rename the local to task_texts so it is visible that a multi-task recording's task_text is only its first task.
| seg["episode_hash"] = row.recording_id | ||
| return segs | ||
|
|
||
| def _clocks(self, steps, rows: dict, ranges: dict) -> dict[str, "_Clock"]: |
There was a problem hiding this comment.
nit (simplification): _clocks never uses self. Make it a @staticmethod, as the _timestamps it replaces was, or a module-level helper next to _Clock.
| pins = filters.episode_hashes if isinstance(filters, DatasetFilter) else () | ||
| dropped = sorted(set(pins) - set(resolved)) | ||
| # Lance keys leaves by recording_id; a pin may name the recording or an old clip. | ||
| keys = getattr(resolver, "dataset_keys", lambda ps: {p: p for p in ps})(pins) |
There was a problem hiding this comment.
nit (simplification): getattr(resolver, "dataset_keys", lambda ps: {p: p for p in ps}) grows the resolver interface by convention, and #726 does the same for manifest. Consider adding dataset_keys (identity) and manifest (None) to the zarr EpisodeResolver base and calling them directly.
Leaves are keyed by recording_id. A recording is cut into segments, one per tasks row plus (with include_taskless_frames) the untasked gaps; filter lambdas run per segment, so a recording with several tasks or aliases is one leaf instead of duplicate catalog rows where the last one won. Pins and excludes take a recording_id or an old clip episode_hash, which narrows or subtracts that clip's time range. pinned_episode_subset renders a pinned clip alone. Claude-Session: https://claude.ai/code/session_01Kt1E8T2x8kdV9akAvbJyVd
a158012 to
248e2a0
Compare
012b637 to
0e76b20
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
Not fixed
|
Leaves are keyed by recording_id. A recording is cut into segments, one per
tasks row plus (with include_taskless_frames) the untasked gaps; filter lambdas
run per segment, so a recording with several tasks or aliases is one leaf
instead of duplicate catalog rows where the last one won. Pins and excludes
take a recording_id or an old clip episode_hash, which narrows or subtracts
that clip's time range. pinned_episode_subset renders a pinned clip alone.
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw