Repository navigation
Conversation
ecb6aae to
73b5244
Compare
73b5244 to
d5599b8
Compare
d5599b8 to
64bcf03
Compare
e05524d to
e57b442
Compare
64bcf03 to
32b73de
Compare
e57b442 to
a023115
Compare
32b73de to
6fd89d1
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): 4 low, 7 nit.
| # kp9 structural zeros) cannot blow up the loss whatever offset | ||
| # they carry, so don't reject on them. Which stat decides that is | ||
| # norm_mode's business -- std under zscore, the quantile range | ||
| # under quantile -- and every shipped config is zscore, so reading |
There was a problem hiding this comment.
low (comments): 'every shipped config is zscore' is wrong: train_zarr_cartesian.yaml sets norm_stats.norm_mode: quantile. The code is fine because it follows norm_mode. Just say the exemption uses the same range stat normalize() uses for the active norm_mode.
| return random.choice(valid_candidates), attempts | ||
|
|
||
|
|
||
| def _bounds_check_channels(zarr_key: str, width: int) -> list[int] | None: |
There was a problem hiding this comment.
low (duplication): _bounds_check_channels repeats the key tuples and the key-to-layout dispatch already in pose_utils.rot6d_channels (_CARTESIAN_KEYS/_KEYPOINT_KEYS). Put a sibling bounds_check_channels(zarr_key, width) (or a shared layout_for_key) in pose_utils that reuses those constants, so a new key or width only needs one edit.
| """ | ||
| try: | ||
| anns = ds._load_annotations() | ||
| except Exception: |
There was a problem hiding this comment.
low (duplication): except Exception: return False makes a transient read error or a bug in _load_annotations indistinguishable from 'no annotations', and the episode is silently dropped (it shows up only in the aggregate drop count). Catch only the expected missing-array errors, or at least log the exception per episode. The start_idx < 0 or end_idx <= start_idx rule is also duplicated from _build_cutoff_mapping, so a small shared _valid_spans(anns) helper would keep them in sync.
| # kp9 structural zeros) cannot blow up the loss whatever offset | ||
| # they carry, so don't reject on them. Which stat decides that is | ||
| # norm_mode's business -- std under zscore, the quantile range | ||
| # under quantile -- and every shipped config is zscore, so reading |
There was a problem hiding this comment.
low (documentation): "every shipped config is zscore" is wrong: train_zarr_cartesian.yaml sets norm_stats.norm_mode: quantile, and nothing overrides it. Under quantile, _degenerate_cells exempts cells by q99-q1, so this rationale and the BOUNDS_REL_SLACK table above (measured with norm_mode=zscore) don't describe the shipped recipe. Fix the comment, and either re-measure the slack under quantile or say which mode the numbers apply to.
| # The bounds are per (timestep, channel) cell and the check is | ||
| # any-cell, so with 13 200 cells per keypoint chunk a strict | ||
| # compare against a subsampled stats pass rejects valid frames in | ||
| # bulk. Measured on the 2815-frame flagship episode through the |
There was a problem hiding this comment.
nit (comments): The slack/rejection table measured on 'the 2815-frame flagship episode' is a dated experiment result. Move it to the PR description and keep one line here: per-cell any-cell checks against subsampled stats reject valid frames, so allow BOUNDS_REL_SLACK of each cell's quantile range. The same goes for '(3-16 % of a single episode in the overfit runs)' at L1697.
| ) | ||
| # Every retry used to log at WARNING: tens of thousands of lines per | ||
| # run. Keep the first few per process, then one in every 500. | ||
| self._retry_log_count = getattr(self, "_retry_log_count", 0) + 1 |
There was a problem hiding this comment.
nit (magic-number): Initialize self._retry_log_count = 0 in __init__ next to _warned_violations rather than getattr(self, '_retry_log_count', 0), and name the 20/500 throttle as class constants (e.g. RETRY_LOG_FIRST, RETRY_LOG_EVERY).
| return data | ||
|
|
||
| def _next_after_failure( | ||
| self, idx: int, dataset_name: str, attempts: int | None, *, reason: str |
There was a problem hiding this comment.
nit (correctness): The return type changed to tuple[int | None, int], but this never returns None. Keep tuple[int, int]. While here, [c for c in self._global_indices_by_dataset[dataset_name] if c != idx] copies the whole per-dataset index list on every bad sample. Drawing with random.choice from the existing list and redrawing on idx avoids that.
| from egomimic.utils.action_utils import ( | ||
| _apply_norm_one, | ||
| _apply_unnorm_one, | ||
| _degenerate, |
There was a problem hiding this comment.
nit (api-design): Importing private _degenerate/_norm_center_scale and calling _norm_center_scale(stats, mode, torch.empty(0)) just for a dtype reference is awkward. A public degenerate_mask(stats, norm_mode) in action_utils, used by both _apply_norm_one and _degenerate_cells, would keep the 'constant channel' rule in one place.
| ) | ||
|
|
||
|
|
||
| def test_constant_cell_exemption_follows_norm_mode(): |
There was a problem hiding this comment.
nit (test-quality): _bounds_check_dataset already exists in this file, but this test and several others (here, in test_eva_wristframe_norm_pool.py and in test_infra_port_wiring.py) hand-build MultiDataset.__new__ shells, so any new attribute _check_bounds reads breaks them all. Consider one shared fixture in tests/fixtures and a dedicated test_multidataset_bounds.py. Also rename the '# adversarial-review fixes' section after what it tests.
| import egomimic.rldb.zarr.zarr_dataset_multi as m | ||
|
|
||
| orig = m.S3EpisodeResolver.resolve | ||
| m.S3EpisodeResolver.resolve = lambda self, filters=None, expected_embodiment=None: { |
There was a problem hiding this comment.
nit (test-quality): Use pytest's monkeypatch.setattr(m.S3EpisodeResolver, 'resolve', ...) instead of reassigning the class attribute and restoring it in try/finally. The inspect.signature(...).parameters['require_annotations'].default assertion pins the signature rather than behavior. Test the default by constructing the resolver without the kwarg, or drop it.
Three independent ways MultiDataset could fail a whole run on data that was
fine, or on data it should have rejected up front.
Bounds check. Quantile bounds on a rotation channel are meaningless: euler ypr
wraps at +-pi and rot6d columns are matrix entries, so neither respects a
quantile interval. _bounds_check_channels uses the layout tables to check only
translation, gripper and keypoint channels; unrecognized widths still get the
full-vector check, and the NaN/Inf check always covers everything. The bounds
themselves get slack -- 1e-6 absolute plus half of each cell's own quantile
range -- because the check is any-cell over ~13 200 cells per keypoint chunk.
Measured on the 2815-frame flagship episode through the real
keypoints_wristframe_6d pipeline (zscore, stats from sample_frac 0.10 over its
own frames, 403 probe frames, any-key rejection): 39.5 % of frames rejected at
slack 0, 17.9 % at 0.05, 11.4 % at 0.1, 5.2 % at 0.25, 3.2 % at 0.5, 1.0 % at
1.0. The 132-channel action chunk drives all of it. Half a range is the knee,
not a cushion -- though the absolute numbers move with which frames the stats
were drawn from, so the comment says how to reproduce them.
Corrupt values sit orders of magnitude outside the range and are still caught.
Cells the normalizer treats as constant are exempt, since a tiny offset there
cannot blow up the loss -- and which stat decides that is norm_mode's business
(std under zscore, the quantile range under quantile), so the exemption asks
_degenerate instead of hardcoding q99 - q1; every shipped config is zscore, and
a channel that is 0 in 99.6 % of samples and 1.0 in the rest has q99 - q1 = 0
but std = 0.063, i.e. it is still divided by and must still be checked.
quantile_0_01 / quantile_99_99 now snap to observed samples ("lower" /
"higher") instead of interpolating: under 10k samples the linear percentile
lands strictly inside the sample range, so the extreme frames of the very
episodes the stats came from were rejected (3-16 % of a single episode in the
overfit runs). A stats/sample shape mismatch is logged once per key instead of
silently disabling the check.
Sampler. Retries stayed inside the failing episode until they exhausted it,
so one wholly-bad episode killed the run and NCCL-timed-out every other rank.
Retries now widen to the global index space after 25 attempts, and 100
consecutive failures raise. The cap is a systemic-failure detector, not a
tolerance dial: attempts resets per __getitem__, so with p = the bad fraction
of the whole index space it fires with probability p**(N - 25), a step function
in p. At N = 100 the firing band sits at p >~ 0.8, so everything it can reach
is systemic; at N = 1000 it needed ~99 % of the store to be bad and got there
through ~975 sequential zarr/S3 reads per call per worker, which reads as a
hang rather than a failure. Retry logging was one WARNING per attempt -- tens
of thousands of lines per run -- now the first 20 per worker and then one in
500.
Annotation cutoff. Many Scale-"completed" episodes have no usable annotation
span because the injection step lagged, which makes the cutoff a silent no-op
on exactly the runs whose point is to clamp chunks at annotation boundaries.
S3AnnotationCutoffEpisodeResolver(require_annotations=True) drops them, logs
the count it dropped and the count it kept, and raises if nothing is left. The
per-episode span line drops to DEBUG: one line per episode per rank is
thousands of lines on a real split, and the resolver's aggregate says the same
thing.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01VBmhpVy7ZcMtsBJWh38cJp
6fd89d1 to
7a13410
Compare
a023115 to
9b9116e
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
|
Three independent ways MultiDataset could fail a whole run on data that was
fine, or on data it should have rejected up front.
Bounds check. Quantile bounds on a rotation channel are meaningless: euler ypr
wraps at +-pi and rot6d columns are matrix entries, so neither respects a
quantile interval. _bounds_check_channels uses the layout tables to check only
translation, gripper and keypoint channels; unrecognized widths still get the
full-vector check, and the NaN/Inf check always covers everything. The bounds
themselves get slack -- 1e-6 absolute plus half of each cell's own quantile
range -- because the check is any-cell over ~13 200 cells per keypoint chunk,
where a strict compare against a subsampled stats pass rejected 30-60 % of
valid frames. Corrupt values sit orders of magnitude outside the range and are
still caught. Channels the normalizer treats as constant are exempt, since a
tiny offset there cannot blow up the loss. quantile_0_01 / quantile_99_99 now
snap to observed samples ("lower" / "higher") instead of interpolating: under
10k samples the linear percentile lands strictly inside the sample range, so
the extreme frames of the very episodes the stats came from were rejected
(3-16 % of a single episode in the overfit runs). A stats/sample shape mismatch
is logged once per key instead of silently disabling the check.
Sampler. Retries stayed inside the failing episode until they exhausted it,
so one wholly-bad episode killed the run and NCCL-timed-out every other rank.
Retries now widen to the global index space after 25 attempts, and at 1000
consecutive failures the worker returns its last good sample with an ERROR and
a counter rather than crashing. A worker that has never produced a good sample
still raises, so a systemic failure (bad norm stats, corrupt store) fails loud.
The cached sample is copied because the collate pops list-valued keys in place.
Retry logging was one WARNING per attempt -- tens of thousands of lines per
run -- now the first 20 per worker and then one in 500.
Annotation cutoff. Many Scale-"completed" episodes have no usable annotation
span because the injection step lagged, which makes the cutoff a silent no-op
on exactly the runs whose point is to clamp chunks at annotation boundaries.
S3AnnotationCutoffEpisodeResolver(require_annotations=True) drops them, logs
what it dropped, and raises if nothing is left.
Co-Authored-By: Claude Opus 5 (1M context) [email protected]
🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw