Repository navigation
Conversation
4bbefe4 to
f0507d5
Compare
ad1ccef to
2dee1f0
Compare
f0507d5 to
ec1546b
Compare
2dee1f0 to
249e508
Compare
ec1546b to
1f628fa
Compare
9602f7f to
ac998dc
Compare
b6f9d4d to
26eb53e
Compare
ac998dc to
304440c
Compare
26eb53e to
362dd61
Compare
304440c to
45bd259
Compare
65fea4d to
9d8c5a2
Compare
3c06f5f to
6a06214
Compare
9d8c5a2 to
2679b17
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 medium, 1 nit.
| sampler stride that never applied, which made every multi-GPU video a | ||
| ``world_size``-times slow-motion (416 frames at 4 fps instead of 30). | ||
| """ | ||
| return int(source_fps) |
There was a problem hiding this comment.
medium (correctness): This is only right when val_dataloader returns a list. With a single head (train_viz=false, no unseen-op split, no video episodes), val_dataloader returns a bare CombinedLoader. Lightning then injects a DistributedSampler into its inner DataLoaders, so rank 0 renders every W-th frame and a fixed 30 fps gives a W-times fast-forward video. Either always return a list from val_dataloader so sharding does not depend on the number of heads, or keep the world_size division for the single-loader case, and add a test for that path.
| sampler stride that never applied, which made every multi-GPU video a | ||
| ``world_size``-times slow-motion (416 frames at 4 fps instead of 30). | ||
| """ | ||
| return int(source_fps) |
There was a problem hiding this comment.
nit (simplification): _video_fps now just returns int(source_fps), and its only caller uses the default. Consider a VIDEO_FPS = 30 constant with a one-line why ('val loaders are unsharded, so rank 0 renders every frame') in place of the 11-line docstring with the 2026-09-16 measurement and the account of the old bug. Later PRs in the stack (eval_video._write_video, pl_data_utils._video_gate) point to 'see _video_fps', so those references would need to move too. The world_size loop in the test then has nothing to test.
_video_fps divided 30 fps by world_size to undo a DistributedSampler stride that never applies: the val heads are CombinedLoaders Lightning does not shard (every rank runs every val batch; 1 rank = 223 val steps, 8 ranks = 8 x 223 in the 2026-09-16 DDP check), so rank 0 renders every frame and the 8-GPU videos came out as 8x slow motion (416 frames at 4 fps). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01THTuWdgk6M6pZ5Ya7ihZEF
2679b17 to
bbd5c3e
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
|

_video_fps divided 30 fps by world_size to undo a DistributedSampler stride
that never applies: the val heads are CombinedLoaders Lightning does not
shard (every rank runs every val batch; 1 rank = 223 val steps, 8 ranks =
8 x 223 in the 2026-09-16 DDP check), so rank 0 renders every frame and the
8-GPU videos came out as 8x slow motion (416 frames at 4 fps).
🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw