Repository navigation
Conversation
30e5af4 to
a26895f
Compare
c314000 to
690b00f
Compare
f263010 to
294da30
Compare
9c8662e to
76facc6
Compare
345d201 to
c4a1766
Compare
76facc6 to
0093b4e
Compare
c4a1766 to
f748613
Compare
0093b4e to
e63242c
Compare
e63242c to
939f249
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, 2 low, 2 nit.
| # config nodes itself (eval mode included). | ||
| datamodule_kwargs["unseen_op_valid_datasets"] = unseen_op_valid_datasets | ||
| # Built above; hydra has nothing to instantiate for these. | ||
| datamodule_kwargs["video_datasets"] = video_datasets |
There was a problem hiding this comment.
medium (correctness): In eval mode video_datasets is still built and passed here, so with data.video_episodes.valid set (e.g. mecka_fold_flagship_opsplit_hpt_6d) val_dataloader() returns [valid, valid_video]. But model.val_loader_names is assigned only in the mode == "train" branch, so ModelWrapper falls back to ["valid", "train_viz"]. As a result, valid_video is loaded up to limit_val_batches and then dropped as a train_viz batch with no evaluator, and valid runs in mode "both", which renders the video from the EvenStride-subsampled frames. Fix by moving model.val_loader_names = datamodule.val_loader_names() above the mode branch, or by skipping _video_datasets outside train mode.
| raise ValueError( | ||
| f"data.metric_frames_per_episode.{head} must be a positive int, got {k}" | ||
| ) | ||
| return k * _trainer_world_size(cfg) |
There was a problem hiding this comment.
low (correctness): K is scaled by devices * num_nodes from the training trainer config, but in eval mode trainHydra later forces devices=1, num_nodes=1 and replaces limit_val_batches with the evaluator's value (400 by default). With a 4-GPU config, eval keeps 4x73 frames per episode on a single rank, so the 'every episode inside limit_val_batches' guarantee no longer holds. Apply the eval trainer overrides before building and subsampling the val datasets, or skip metric subsampling when mode != train.
| if video_only: | ||
| mode = "video" | ||
| # Non-viz epoch: skip before the forward pass, it would be wasted. | ||
| should_viz = getattr(active, "_should_viz", None) |
There was a problem hiding this comment.
low (simplification): validation_step reaches into the evaluator's private _should_viz through getattr/callable, and EvalVideo.on_validation_step(mode="video") already returns on the same condition. Either add a public should_viz() to the Eval interface (default True) and call that, or drop this check and let the evaluator decide. As written, the viz gating lives in two classes and only one of them applies the viz_max_batches cap.
| ), | ||
| } | ||
|
|
||
| # Episode counts these K values were computed from (measured 2026-09-15). |
There was a problem hiding this comment.
nit (test-quality): This test copies the YAML's K table and pinned hashes, plus a dated episode count ("measured 2026-09-15"), so it mostly checks that the YAML equals a copy of itself, and it will break on every re-measurement. The docstring also says "Both flagship configs", but only one config is parametrized, and lines 117-118 repeat the same values. Assert the structural properties instead (keys are VAL_HEADS, K > 0, pins are lists of str, _metric_frames_per_episode round-trips) and move the K derivation into the YAML comment.
| assert wrapped.base is split | ||
|
|
||
| per_episode = {name: 0 for name in sizes} | ||
| starts = {} |
There was a problem hiding this comment.
nit (dead-code): starts and offset are computed and never used. Delete the loop.
The unshuffled val loaders were truncated by limit_val_batches (80 x 64 x W frames) to their leading episode-hash-sorted slice, so at W = 1 the seen-op head scored 25 of 70 episodes and 6 of 11 operators, and the unseen head 33 of 191 episodes with an 81/19 operator mix on a 52/48 split (measured 2026-09-15, single-rank). The loaders are unshuffled only so the overlay video is coherent, and the video renders just the first viz_max_batches (10) batches anyway. Split each val head in two: * metric loader = EvenStrideDataset(split, frames_per_episode=K) with K from data.metric_frames_per_episode[head], so every episode -- and so every operator -- is scored, at the same 80 batches and with no rendering; * video loader = <head>_video, the head's split restricted to the episode hashes pinned in data.video_episodes[head] (one per operator), contiguous and unsubsampled, logging no metrics. The pinned episodes are taken out of the already-resolved split, so resolve-once still holds. EvalVideo.on_validation_step gains a mode argument (metrics / video / both); ModelWrapper.validation_step routes <head>_video to the same evaluator in video mode and skips it entirely on a non-viz epoch, before the forward pass. Heads with no pins are called exactly as before, so a data config that declares neither key behaves as today. K = floor(5120 / episodes-in-split), from the measured splits: opsplit valid 73 (70 eps), train_viz 3 (1346 eps), unseen_op_valid 26 (191 eps). Those are SINGLE-RANK numbers and trainHydra scales them by trainer.devices x num_nodes, because DistributedSampler strides rather than chunks: the W ranks between them walk the whole split, so the pre-split window was already 80 x 64 x W frames wide. Without the scaling the shipped 4-GPU ddp_pi run would score 5,110 frames of the seen-val head where it previously scored all 13,744 -- a regression on the one head small enough to score whole. EvenStrideDataset keeps an episode entire when K exceeds its length, so the scaling cannot overrun a short split. Expect a shift in the per-batch Frechet _min/_max curves: they now span many episodes instead of ~2 s of one, so they are not comparable with older runs. Eval.on_validation_step, the abstract base, takes the mode kwarg too: the metrics/video path passes it unconditionally, so an evaluator that predates the split would have raised TypeError at the first val step of any config that declares video_episodes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBmhpVy7ZcMtsBJWh38cJp
17a05a0 to
dbfa725
Compare
939f249 to
5fafcb3
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
|
The unshuffled val loaders were truncated by limit_val_batches (80 x 64 =
5120 frames) to their leading episode-hash-sorted slice, so the seen-op head
scored 25 of 70 episodes and 6 of 11 operators, and the unseen head 33 of 191
episodes with an 81/19 operator mix on a 52/48 split. The loaders are
unshuffled only so the overlay video is coherent, and the video renders just
the first viz_max_batches (10) batches anyway.
Split each val head in two:
data.metric_frames_per_episode[head], so every episode -- and so every
operator -- is scored, at the same 80 batches and with no rendering;
hashes pinned in data.video_episodes[head] (one per operator), contiguous
and unsubsampled, logging no metrics. The pinned episodes are taken out of
the already-resolved split, so resolve-once still holds.
EvalVideo.on_validation_step gains a mode argument (metrics / video / both);
ModelWrapper.validation_step routes _video to the same evaluator in
video mode and skips it entirely on a non-viz epoch, before the forward pass.
Heads with no pins are called exactly as before, so a data config that
declares neither key behaves as today.
K = floor(5120 / episodes-in-split), from the measured splits of
mecka_fold_flagship_opsplit_hpt_6d: valid 73 (70 eps), train_viz 3 (1346 eps),
unseen_op_valid 26 (191 eps). The same keys for the single-operator flagship
config are kept out of this PR along with that config.
Expect a shift in the per-batch Frechet _min/_max curves: they now span many
episodes instead of ~2 s of one, so they are not comparable with older runs.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw