Repository navigation
Conversation
2ae6b54 to
af92a04
Compare
f01abdc to
effdec5
Compare
a7e6042 to
286b8e8
Compare
3767a58 to
b91c1b3
Compare
1215a37 to
feea4f0
Compare
b91c1b3 to
592fa6f
Compare
feea4f0 to
7f6c8c3
Compare
592fa6f to
64303ff
Compare
7f6c8c3 to
e38fa26
Compare
64303ff to
e306f04
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 low, 2 nit.
| one dataloader as "move to the next". | ||
| """ | ||
|
|
||
| def __init__(self, iterables, mode, gate): |
There was a problem hiding this comment.
low (simplification): _VizGatedLoader accepts mode only to reject everything but 'max_size_cycle', and its single caller passes that literal. Drop the parameter and call super().init(iterables, "max_size_cycle"). More importantly, len and the module import depend on Lightning privates (_MaxSizeCycle, _iterator, flattened, limits), and lightning is unpinned in requirements.txt and pyproject.toml. An upgrade that renames _MaxSizeCycle breaks the import of pl_data_utils itself. Pin the Lightning version, or at least name the version these privates were checked against.
| because what they log is not: the metric loaders run everywhere and log | ||
| the same keys, the video loader logs nothing, so the ``sync_dist`` | ||
| reduction in ``_evaluation_epoch_end`` stays symmetric. It does mean the | ||
| other ranks sit in that reduction for as long as rank 0 renders. Measured |
There was a problem hiding this comment.
nit (comments): Keep the invariant in _video_gate's docstring (the video loader logs nothing, so the sync_dist reduction stays symmetric, and viz_max_batches bounds rank 0's render against the process-group timeout). Drop the measurements ('8.4 min for 76 batches', and '~0.1 s/frame, 4864 frames' in eval_video.py). The 30-minute figure is a configurable default, not a constant.
| def __init__(self, pinned): | ||
| super().__init__() | ||
| self.layer = torch.nn.Linear(1, 1) | ||
| self.pinned = pinned |
There was a problem hiding this comment.
nit (dead-code): _Module stores pinned but never reads it, since main() reads pinned.reads directly. Drop the parameter and attribute.
Two problems on the <head>_video loaders, one of them a correctness bug. EvalVideo._write_video had no rank guard. The val heads come back from val_dataloader() inside CombinedLoaders, which Lightning does not wrap in a DistributedSampler, so every rank iterates every val batch and renders the same frames -- and every rank then wrote the same mp4 path. The file was whichever rank finished last, and could be torn. Separately, the video loaders were iterated in full on every validation pass. ModelWrapper.validation_step already returned before the forward on a non-viz pass, but Lightning had built the batch by then, so limit_val_batches x 64 pinned frames were decoded and discarded on every rank on the ~90% of passes that render nothing: ~1.7 min a pass, ~4% of a 1000-step cycle. _VizGatedLoader yields nothing when its gate is closed, which is on a non-viz pass and on every rank but 0. The val loaders are built once (_EvaluationLoop.setup_data returns early once _combined_loader is set), so the gate cannot be a length; it is an empty iterator, which _Sequential already reads as "move to the next loader". __len__ answers WITHOUT creating an iterator, because creating one forks that loader's workers and starts them prefetching -- doing the skipped work at setup instead, and never freeing it. It asks a fresh _MaxSizeCycle for the length, which stores its loaders and limits and builds no iterator until __iter__ (lightning 2.6.1). Ranks therefore iterate different numbers of val batches. That is safe because what they LOG is not: the metric loaders run everywhere and log the same keys, the video loader logs nothing, so the sync_dist reduction stays symmetric. It does mean the other ranks sit in that reduction while rank 0 renders -- 8.4 min for 76 batches back when all four ranks rendered at once, so well inside the 30-minute process-group timeout, but that timeout is now the ceiling on evaluator.viz_max_batches. trainHydra therefore refuses a head that has pinned video episodes and an evaluator with viz_max_batches=None. Every recipe that pins video episodes already caps it at 10; the evaluators that leave it unset (eval_hpt, eval_hpt_cartesian, eval_hpt_wrist) pair with configs that pin nothing, so they keep the ungated path where every rank renders together, as before. A desync here would hang a campaign rather than fail it, so it is tested against real DDP rather than argued: tests/fixtures/ddp_video_gate.py runs a 2-process gloo fit with the same collective pattern, at num_workers > 0 so the prefetch is visible. Both ranks finish; rank 0 steps the video loader on the viz epoch, rank 1 never touches the pinned episode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5dTGNydAZbRP1EsXgbp4W
e306f04 to
d4712b6
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
|

Fixes two problems on the
<head>_videoval loaders. Every rank wrote the same mp4, which is a correctness bug. The pinned video batches were also decoded on every validation pass, including the ones that render nothing.Behaviour changes
viz_max_batches=None. Every recipe that pins video episodes already caps it at 10. The evaluators that leave it unset (eval_hpt,eval_hpt_cartesian,eval_hpt_wrist) pair with configs that pin nothing, so they keep the ungated path, where every rank renders together as before.What changed
EvalVideo._write_video. The val heads come back fromval_dataloader()insideCombinedLoaders, which Lightning does not wrap in aDistributedSampler. Every rank iterated every val batch, rendered the same frames and wrote the same mp4 path, so the file was whichever rank finished last and could be torn._VizGatedLoader(pl_utils/pl_data_utils.py) yields nothing when its gate is closed, which is on a non-viz pass and on every rank but 0. Before,ModelWrapper.validation_stepreturned before the forward on a non-viz pass, but Lightning had already built the batch. Solimit_val_batchesx 64 pinned frames were decoded and discarded on every rank on the ~90% of passes that render nothing: ~1.7 min a pass, ~4% of a 1000-step cycle._EvaluationLoop.setup_datareturns early once_combined_loaderis set), so the gate can't be a length. It is an empty iterator, which_Sequentialalready reads as "move to the next loader".__len__answers without creating an iterator. Creating one forks the loader's workers and starts them prefetching, which would do the skipped work at setup and never free it. It asks a fresh_MaxSizeCyclefor the length; on lightning 2.6.1 that stores its loaders and limits and builds no iterator until__iter__._should_viz).DDP safety
Ranks now iterate different numbers of val batches. That is safe because what they log is symmetric: the metric loaders run everywhere and log the same keys, and the video loader logs nothing, so the
sync_distreduction matches. The other ranks do wait in that reduction while rank 0 renders. Rendering took 8.4 min for 76 batches back when all four ranks rendered at once, well inside the 30-minute process-group timeout. That timeout is now the ceiling onevaluator.viz_max_batches, which is why the fail-fast above exists.Tests
tests/fixtures/ddp_video_gate.pyruns a 2-process gloo fit with the same collective pattern, atnum_workers > 0so prefetch is visible. Intest_two_rank_ddp_fit_survives_the_rank_divergence, both ranks finish; rank 0 steps the video loader on the viz epoch and rank 1 never touches the pinned episode.tests/unit/test_eval_loaders.py: rank-0-only writes, the closed gate yielding nothing, the gate's fallbacks,__len__opening no iterator, a real fit building video batches only on viz passes, andtest_video_head_without_a_viz_cap_is_rejected.🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw