Repository navigation
Conversation
82862c3 to
d655d4d
Compare
ac998dc to
304440c
Compare
d655d4d to
da62987
Compare
304440c to
45bd259
Compare
da62987 to
5d10546
Compare
45bd259 to
3c06f5f
Compare
5d10546 to
d429a6a
Compare
3c06f5f to
6a06214
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): 2 nit.
| self.device = kwargs.get( | ||
| "device", torch.device("cuda" if torch.cuda.is_available() else "cpu") | ||
| ) | ||
| self.device = None |
There was a problem hiding this comment.
nit (dead-code): Once self.device = None is set, the following model.device = self.device assigns None to an HPTModel whose init already sets device None. The new setter also handles propagation once nets['policy'] exists, so that line can go. Any device kwarg is now silently ignored; no shipped config passes one, but a pop-with-warning would make the change explicit.
| @@ -0,0 +1,124 @@ | |||
| """Deterministic evaluation. Mixed into HPT; nothing else uses it yet. | |||
There was a problem hiding this comment.
nit (comments): The module docstring includes roadmap and status prose ('nothing else uses it yet', 'Seven of these eight methods are algo-agnostic ... small follow-up') that goes stale on the next edit or when PI adopts the mixin. Keep the seeding formula and the list of attributes the host algo must set, and track the PI follow-up in an issue or TODO.
HPT.__init__ defaulted self.device to torch.device('cuda') and moved the
freshly built policy there. Under submitit every rank sees all 8 GPUs, so all
8 ranks resolved that to cuda:0 and built their policy on the same device:
8 x 3.34B, which OOMs during construction. The 1B arm fit and survived, which
made the failure look config-specific.
PI already builds on CPU and leaves self.device unset for Lightning to fill in
through ModelWrapper.on_fit_start / on_validation_start. HPT now does the same:
no device is chosen in __init__ and nothing leaves CPU there.
Lightning's write then has to reach the HPTModel the algo holds. That model
reads self.device to build the loss accumulators in compute_loss, while
HPT.compute_losses accumulates on the algo's, so a write that stopped at the
algo would leave the two on different devices -- 'Expected all tensors to be
on the same device, but found at least two devices, cuda:0 and cpu!'.
HPT.device is therefore a property whose setter also writes
nets['policy'].device, so the two cannot diverge through the path the trainer
uses.
Verified on 8xH100 with the 3.34B hpt_3b arch-comparison arm: every rank
reports CUDA_VISIBLE_DEVICES [0..7], all 8 reach fit start and train an epoch
without an OOM.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01EAGuZKPVfr2J9L7W9gtxET
The eval-time seeding (eval_base_seed + rank * EVAL_RANK_STRIDE + pass counter, the prompt sampling and the seeded generator) was 96 lines inside HPT, though nothing in it is HPT-specific: it reads only nets, device, eval_base_seed, annotation_key, annotation_sampling_mode and default_prompt. Moving it to egomimic/algo/eval_determinism.py as a mixin leaves hpt.py with the algo and gives the seeding contract one documented home. The attributes it reads are annotated, not defaulted: a class-level default would turn "the algo forgot to set this" from an AttributeError into a silent wrong value. The module docstring says plainly that this is HPT-only today -- PI mixes in nothing and has no eval seeding at all, so pi0.5 validation is still not reproducible -- and what wiring PI up would take. Pure code motion -- no behaviour change, the eight method bodies are byte-identical to the originals, and the existing determinism tests cover it unchanged. 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_01VBmhpVy7ZcMtsBJWh38cJp
d429a6a to
8768ba7
Compare
6a06214 to
2590fb5
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
|

HPT.init defaulted self.device to torch.device('cuda') and moved the
freshly built policy there. Under submitit every rank sees all 8 GPUs, so all
8 ranks resolved that to cuda:0 and built their policy on the same device:
8 x 3.34B, which OOMs during construction. The 1B arm fit and survived, which
made the failure look config-specific.
PI already builds on CPU and leaves self.device unset for Lightning to fill in
through ModelWrapper.on_fit_start / on_validation_start. HPT now does the same:
no device is chosen in init and nothing leaves CPU there.
Lightning's write then has to reach the HPTModel the algo holds. That model
reads self.device to build the loss accumulators in compute_loss, while
HPT.compute_losses accumulates on the algo's, so a write that stopped at the
algo would leave the two on different devices -- 'Expected all tensors to be
on the same device, but found at least two devices, cuda:0 and cpu!'. That was
7 of the unit tests (4 test_hpt_train_step, 2 test_norm_cache, 1
test_resolve_once) whenever the suite ran on a GPU node. HPT.device is
therefore a property whose setter also writes nets['policy'].device, so the two
cannot diverge through the path the trainer uses.
Note this sets a device, it does not move anything: the parameters follow
because Lightning moves the LightningModule itself. rollout.py::_load_policy is
the only construction path outside Lightning and already does both its own
.to(policy_device) and the device write, which now reaches the HPTModel too.
Verified on 8xH100 with the 3.34B hpt_3b arch-comparison arm: every rank
reports CUDA_VISIBLE_DEVICES [0..7], all 8 reach fit start and train an epoch
without an OOM.
Folded in (to keep the stack under Graphite's 50-branch cap):
🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw