Repository navigation
Conversation
This was referenced Sep 19, 2026
Contributor
Author
This was referenced Sep 19, 2026
Closed
RyanPCo
force-pushed
the
ryanco/eval-video-fps
branch
from
September 20, 2026 19:09
4bbefe4 to
f0507d5
Compare
RyanPCo
force-pushed
the
ryanco/ddp-bf16-hook
branch
3 times, most recently
from
September 22, 2026 13:18
c5fd130 to
d54b1cf
Compare
This was referenced Sep 22, 2026
RyanPCo
force-pushed
the
ryanco/eval-video-fps
branch
from
October 12, 2026 21:50
b6f9d4d to
26eb53e
Compare
This was referenced Oct 12, 2026
RyanPCo
force-pushed
the
ryanco/ddp-bf16-hook
branch
from
October 12, 2026 23:19
3adf25b to
57330fa
Compare
RyanPCo
force-pushed
the
ryanco/eval-video-fps
branch
from
October 12, 2026 23:19
26eb53e to
362dd61
Compare
Open
RyanPCo
force-pushed
the
ryanco/eval-video-fps
branch
from
October 19, 2026 19:44
362dd61 to
65fea4d
Compare
RyanPCo
force-pushed
the
ryanco/ddp-bf16-hook
branch
from
October 19, 2026 19:44
57330fa to
3b31bf1
Compare
RyanPCo
force-pushed
the
ryanco/eval-video-fps
branch
from
October 19, 2026 20:22
65fea4d to
9d8c5a2
Compare
RyanPCo
force-pushed
the
ryanco/ddp-bf16-hook
branch
from
October 19, 2026 20:22
3b31bf1 to
383f798
Compare
This was referenced Oct 19, 2026
RyanPCo
force-pushed
the
ryanco/ddp-bf16-hook
branch
from
October 30, 2026 18:27
383f798 to
31aba89
Compare
RyanPCo
force-pushed
the
ryanco/eval-video-fps
branch
from
October 30, 2026 18:27
9d8c5a2 to
2679b17
Compare
RyanPCo
commented
Oct 30, 2026
RyanPCo
left a comment
Contributor
Author
There was a problem hiding this comment.
Automated adversarial review (read-only; findings were checked by a second skeptical pass before posting): 1 nit.
| # gradient reduction, not the math, sets the step time. On the lab's 8 x A40 | ||
| # nodes -- no NVLink, two NUMA islands joined by cross-socket links -- a ~1.2 B | ||
| # -parameter policy measured 6.4 s/step at 8 x batch 16 against 1.4 s/step on a | ||
| # single GPU, and 3.6 s/step once the reduction was halved (2026-09-16). Only |
Contributor
Author
There was a problem hiding this comment.
nit (leftover-experiment): The dated 8xA40 benchmark (6.4 s/step vs 3.6 s/step, 2026-09-16) belongs in the PR description, not the config. 'bf16 gradient all-reduce for comm-bound DDP; master weights and optimizer stay fp32' is enough. On line 15, a one-line note that the parent's strategy: ddp string has to become a DDPStrategy object to accept ddp_comm_hook would explain the re-specified target.
On the lab's 8 x A40 nodes -- no NVLink, two NUMA islands joined by cross-socket links -- the per-step gradient all-reduce, not the math, sets the step time once a model is large enough. Measured 2026-09-16 on a ~1.2 B -parameter policy: 6.4 s/step at 8 x batch 16 against 1.4 s/step on a single GPU, and 3.6 s/step with the reduction in bf16. Only the reduction is bf16; master weights and the optimizer stay fp32. It is plain DDP otherwise -- no unused-parameter detection -- so a model that leaves a trainable tensor without a gradient needs ddp_pi's `ddp_find_unused_parameters_true` instead and will crash under this one. The config says so, because nothing else would stop you selecting it for Pi. The hook is a zero-argument factory rather than a Hydra `_partial_` because Lightning logs the hook's __qualname__, which a functools.partial does not carry. `make_bf16_compress_hook` is named for what it is: torch's own hook is `bf16_compress_hook`, and sharing that name made the factory look like the hook. test_eval_determinism's trainer parametrization now enumerates the config group instead of a hardcoded list, so a new trainer config cannot land without the gradient-clip knob being checked. Co-Authored-By: Claude Fable 5.1 <[email protected]> Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01THTuWdgk6M6pZ5Ya7ihZEF
RyanPCo
force-pushed
the
ryanco/eval-video-fps
branch
from
October 30, 2026 23:05
2679b17 to
bbd5c3e
Compare
RyanPCo
force-pushed
the
ryanco/ddp-bf16-hook
branch
from
October 30, 2026 23:05
31aba89 to
bb5001f
Compare
Contributor
Author
|
Review findings: status after the fixup pass. The fixes are squashed into this PR's commits, and the whole stack was force-pushed. Fixed
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

On the lab's 8 x A40 nodes -- no NVLink, two NUMA islands joined by
cross-socket links -- the per-step gradient all-reduce, not the math, sets the
step time once a model is large enough. Measured 2026-09-16 on a ~1.2 B
-parameter policy: 6.4 s/step at 8 x batch 16 against 1.4 s/step on a single
GPU, and 3.6 s/step with the reduction in bf16. Only the reduction is bf16;
master weights and the optimizer stay fp32.
It is plain DDP otherwise -- no unused-parameter detection -- so a model that
leaves a trainable tensor without a gradient needs ddp_pi's
ddp_find_unused_parameters_trueinstead and will crash under this one. Theconfig says so, because nothing else would stop you selecting it for Pi.
The hook is a zero-argument factory rather than a Hydra
_partial_becauseLightning logs the hook's qualname, which a functools.partial does not
carry.
make_bf16_compress_hookis named for what it is: torch's own hook isbf16_compress_hook, and sharing that name made the factory look like the hook.test_eval_determinism's trainer parametrization now enumerates the config group
instead of a hardcoded list, so a new trainer config cannot land without the
gradient-clip knob being checked.
🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw