Repository navigation
Conversation
56731c5 to
c39e112
Compare
dedae99 to
1c8e707
Compare
1c8e707 to
e51b3b4
Compare
c5fb24d to
add5691
Compare
e51b3b4 to
3488d12
Compare
3488d12 to
ece3cd2
Compare
add5691 to
d4b33aa
Compare
d4b33aa to
98d8d22
Compare
ece3cd2 to
025517c
Compare
98d8d22 to
cac0fd0
Compare
025517c to
816e441
Compare
816e441 to
b28a7ff
Compare
cac0fd0 to
0a74649
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): 3 low, 2 nit.
| drawn uniformly from ``leaves`` (episode hash -> ZarrDataset). Episodes | ||
| missing from ``root`` at the needed density are sampled and cached first.""" | ||
| root = Path(root) | ||
| fingerprints = { |
There was a problem hiding this comment.
low (duplication): collect() recomputes the episode fingerprints that trainHydra just computed for the split cache key, a scandir plus a stat per array per episode, done twice on a split-cache miss. Pass trainHydra's dict through infer_norm_from_dataset(..., episode_fingerprints=...) into collect(). Also handle a None fingerprint (episode_fingerprint returns None on an OSError), which currently crashes _file/find with a TypeError on fingerprint[:16]. Sample such an episode without caching it.
| if pending[h] == 0: | ||
| _write( | ||
| _file(root, h, fingerprints[h], stride), | ||
| idx_buf.pop(h, []), |
There was a problem hiding this comment.
low (data-integrity): A frame that fails in _Frames.__getitem__ becomes None, but the episode's npz is still written once pending[h] hits 0. A transient failure is therefore cached permanently as a partial or empty sample, keyed only by recipe, fingerprint and stride. Skip the cache write (keep the rows in memory) when any frame of the episode failed, or record the failure count and treat such a file as a miss in find. Relatedly, _write swallows OSError, but collect then np.loads files[h] unconditionally and crashes. Fall back to the in-memory rows there.
| missing from ``root`` at the needed density are sampled and cached first.""" | ||
| root = Path(root) | ||
| fingerprints = { | ||
| h: norm_cache.episode_fingerprint(getattr(ds, "episode_path", None)) |
There was a problem hiding this comment.
low (error-handling): norm_cache.episode_fingerprint can return None (scandir failure, no metadata files), and find/_file then evaluate fingerprint[:16] on it and raise TypeError at startup. The old cache path only hashed the value, so it tolerated None. Treat a None-fingerprint episode as uncacheable: sample it without writing, or fall back to the whole-dataset loader.
| return {str(k): z[f"k{i}"][rows] for i, k in enumerate(z["keys"])} | ||
|
|
||
|
|
||
| def _to_numpy(x): |
There was a problem hiding this comment.
nit (duplication): This local _to_numpy duplicates egomimic.utils.type_utils._to_numpy. Import that one instead.
| generator=torch.Generator().manual_seed(seed), | ||
| worker_init_fn=die_with_parent, | ||
| ) | ||
| if episode_cache is not None and not all( |
There was a problem hiding this comment.
nit (simplification): The double negation is hard to read. An equivalent form is if episode_cache is not None and any(isinstance(ds, MultiDataset) or getattr(ds, "episode_path", None) is None for ds in dataset.datasets.values()):.
Norm stats were cached per split, so a new episode set (20% -> full, an operator hold-out, newly collected data) resampled every episode. Each episode's sampled proprio/action rows are now cached on their own under <cache_dir>/episodes/, keyed by the resolver recipe and reader code but not by the split, and a split's stats are computed from the union of its episodes' rows. Frames sit on a power-of-two stride with a per-episode phase, so a file cached at stride s serves every coarser stride. Adds norm_stats.max_samples (now part of the split key; KEY_VERSION 3). Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_0118RhoJmzcWC75B3vtc3zkF
…andas Startup on the 41.6k-episode full-mecka split spent ~20 min before training: populate_from_datasets read a full sample from every episode to learn the post-transform keys, though a resolver hands every episode the same key_map and transform objects. It now probes one episode per (embodiment, key_map, transform) group, trying up to 8 before falling back to the raw key_map. Resolving pinned filters was ~50 s each: a row-wise df.apply(row.to_dict()) over the ~458k-row episode table plus a per-pin .loc in _check_pins. Pinned filters now narrow the table to their hashes first, rows are filtered as records, and _check_pins builds one dict of pinned rows. Same paths on the real table; 41.6k pins 2.5 s, val pins ~0 s, unpinned lab filter 32 s -> 13 s. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_0118RhoJmzcWC75B3vtc3zkF
0a74649 to
bd2cb3e
Compare
b28a7ff to
36f7cf1
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
|
Norm stats were cached per split, so a new episode set (20% -> full, an operator hold-out, newly collected data) resampled every episode. Each episode's sampled proprio/action rows are now cached on their own under
<cache_dir>/episodes/, keyed by the resolver recipe and reader code but not by the split, and a split's stats are computed from the union of its episodes' rows. Frames sit on a power-of-two stride with a per-episode phase, so a file cached at stride s serves every coarser stride.Adds
norm_stats.max_samples(now part of the split key;KEY_VERSION3).Also probes one episode per keymap instead of every episode, and filters SQL pins without per-row pandas.
Tests:
tests/unit/test_episode_norm_samples.py,test_norm_cache.py,test_populate_probe.py.Folded in (to keep the stack under Graphite's 50-branch cap):
🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw