Repository navigation
Conversation
f50f94c to
c9c284f
Compare
3f17dfa to
c76a447
Compare
72d026d to
bbac4a0
Compare
c76a447 to
865520b
Compare
05f1e3e to
bbbf371
Compare
b97f495 to
61037b6
Compare
61037b6 to
97bea62
Compare
bbbf371 to
8ac8f46
Compare
97bea62 to
635b94a
Compare
8ac8f46 to
4098a1c
Compare
635b94a to
7d794a9
Compare
4098a1c to
256238b
Compare
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.
| f"resolved split; got limit_val_batches={limit!r}, " | ||
| f"batch_size={batch_size!r}, episodes={n_episodes!r}" | ||
| ) | ||
| k = max(1, limit * int(batch_size) // n_episodes) |
There was a problem hiding this comment.
high (correctness): When val_dataloader returns a list (the default, since train_viz is on), the val loaders get no DistributedSampler (#666 and #673 rely on this). Scaling K by _trainer_world_size therefore makes every rank read the same first limit_val_batches*batch_size frames of an episode-major set that is W times that size. On 8 GPUs, Valid/* and unseen_op_valid/* end up scoring only ~1/8 of the episodes, which defeats the per-episode subsampling, and the metrics change with GPU count. The multiplier predates this PR, but auto builds it into the recommended path. Drop the world-size factor whenever the metric loaders are unsharded (or shard them explicitly), and make the _trainer_world_size/_metric_frames_per_episode docstrings agree with EvalVideo._video_fps.
… empty prompt The metric budget can be 'auto': trainHydra derives K = floor(limit_val_batches * batch_size / episodes) per head from the resolved split, so K follows the live filters instead of a hand count that goes stale when SQL grows (or when limit_val_batches or the batch size change); limit_val_batches=0 (validation off) subsamples nothing. Explicit ints keep their meaning, so the flagship recipes that pin K are untouched. mecka_fold_freeform_opsplit_6d has no annotation guard and stays frozen; its header now says so, and HPT warns once when a sample falls back to an empty default_prompt, as Pi does. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw
7d794a9 to
d8fc6e7
Compare
256238b to
f8a8ff4
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
|

Adds an opt-in
metric_frames_per_episode: autofor val heads and a one-time warning when a sample falls back to an empty prompt.Behaviour changes
None by default.
autois opt-in, explicit ints keep their meaning, and no shipped config uses it yet. The prompt change is a log line only.metric_frames_per_episode: autoK = floor(limit_val_batches * batch_size / episodes)per head and dataset, from the resolved split and that head's loaderbatch_size, then scales by world size as before. K follows the live filters instead of a hand count that goes stale when a split is defined by SQL filters rather than an episode pin.limit_val_batches=0(validation off) subsamples nothing.limit_val_batches, or a missing batch size or split, raises.Other changes
DeterministicEvalMixin._build_promptswarns once when a sample has no annotation anddefault_promptis empty, so the text stem silently sees''.mecka_fold_freeform_opsplit_6dstays frozen. It has no annotation guard (112 of its 462 episodes havesegments = NULL), but checkpoints were trained on it. Its header now says so.The operator-scaling ladder data configs (k = 1/2/4) and their test are kept out of this PR.
Tests
tests/unit/test_eval_loaders.py::test_auto_k_is_derived_from_the_resolved_splitcovers the formula, the world-size scaling, thelimit_val_batches=0pass-through and the error cases.🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw