Repository navigation
perf: vectorized image augs and a torch.compile knob for HPT and Pi; Pi clips gradients like openpi - #676
perf: vectorized image augs and a torch.compile knob for HPT and Pi; Pi clips gradients like openpi#676RyanPCo wants to merge 1 commit into
Conversation
732de9b to
ec36005
Compare
43cdc6c to
e10ccbe
Compare
183054e to
796c29c
Compare
e10ccbe to
4e35316
Compare
7f5dcda to
2a00698
Compare
446a560 to
fcdbf6e
Compare
2a00698 to
ee8dfd2
Compare
ee8dfd2 to
76b71f7
Compare
f1dee44 to
d3e5b0d
Compare
d3e5b0d to
f20e8aa
Compare
119657d to
bbbca3e
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, 4 low, 2 nit.
|
|
||
| if config_tree is not None: | ||
| self.model = self._instantiate_model(config_tree, norm_stats_state) | ||
| self._maybe_compile(config_tree) |
There was a problem hiding this comment.
medium (correctness): _maybe_compile runs in __init__ from config_tree, which save_hyperparameters persists. ModelWrapper.load_from_checkpoint in robot/rollout.py therefore re-applies model.compile.enabled=true for any HPT checkpoint trained with the flag. The trunk, encoders and head denoiser are then compiled with inductor on the first real-time inference call: about 40 s or more, plus recompiles for batch-1 and eval mode, and the robot box needs a working triton toolchain. PI is unaffected, because rollout goes through sample_actions, which Module.compile does not route. Consider applying compilation only for training, for example by calling compile_for_training from trainHydra after constructing the wrapper or in setup(stage="fit"), or by forcing compile.enabled=false in rollout's checkpoint patching.
| dynamic: bool = False, | ||
| ) -> List[str]: | ||
| """``module.compile(...)`` for each named module; returns the names done.""" | ||
| kwargs = {"dynamic": dynamic} |
There was a problem hiding this comment.
low (performance): dynamic is always passed as a bool. The yaml defaults to false, and _maybe_compile coerces null to False, so torch.compile's default automatic-dynamic mode (dynamic=None) can't be selected. No train loader sets drop_last, so the final partial batch, validation batches and each embodiment's batch/token shape in cotraining each trigger a full static recompile (about 40 s for HPT). With enough shapes, dynamo's recompile limit sends those modules back to eager without warning. Consider passing dynamic through as-is with a null default, or document the per-shape recompiles next to the ~40 s / ~4 min figures in the yaml.
| # possible values and float32(u)/float32(255) equals | ||
| # float32(u / 255.0) for every one of them (see | ||
| # tests/unit/test_image_decode_dtype.py). | ||
| data[k] = np.transpose(decoded, (2, 0, 1)).astype( |
There was a problem hiding this comment.
low (duplication): ZarrActionExpertDataset._load_obs_at (zarr_dataset_action_expert.py:94) still decodes with / 255.0 and returns float64, so the two zarr datasets now emit different dtypes for the same frame. Consider a small shared decode_jpeg_chw_float32(buf) helper and calling it from both sites. The 9-line comment could then shrink to one line pointing at the bit-identity test.
| assert np.array_equal(wide, narrow) | ||
|
|
||
|
|
||
| def test_decode_path_emits_float32(): |
There was a problem hiding this comment.
low (test-quality): test_decode_path_emits_float32 copies the expression instead of running the dataset code, so it still passes if ZarrDataset.getitem reverts to / 255.0. Its docstring also names MultiDataset._read_keys, which does not exist; the code is in ZarrDataset.getitem. Test a shared decode helper directly, or drop this test and keep the exhaustive bit-identity one.
| # Off by default -- a run that never reaches steady state pays the compile for | ||
| # nothing. Per run: model.compile.enabled=true. `mode` goes straight to | ||
| # torch.compile; null is the default inductor mode the numbers were taken with. | ||
| compile: |
There was a problem hiding this comment.
low (duplication): Only this file and hpt_bc_mecka_6d_300M.yaml define compile:. The default model (hpt_bc_flow_eva) and the other HPT configs, which HPT.compile_for_training would also speed up, do not, so the documented model.compile.enabled=true fails under Hydra struct mode for them and needs +model.compile.enabled=true. Consider putting the block in a shared place (e.g. hpt_bc_flow_eva / a defaults include) or documenting the + spelling once instead of repeating the long comment in each YAML.
| before DDP wraps anything -- inductor then sees one module at a time and | ||
| DDP's bucketing is unaffected. Algos that have nothing worth compiling | ||
| leave this alone; the default no-ops so a run that turns the flag on | ||
| against such an algo is slower to start, not broken. |
There was a problem hiding this comment.
nit (docs): The default compiles nothing, so a run with the flag on against such an algo starts exactly like an uncompiled one, not slower (ModelWrapper prints a warning). Suggest ending the docstring with: 'Returns the names of the modules compiled; the default compiles nothing and returns [].'
| cfg = {"enabled": True, "mode": "reduce-overhead", "dynamic": True} | ||
| assert _wrapper_with(cfg, monkeypatch).calls == [("reduce-overhead", True)] | ||
|
|
||
|
|
There was a problem hiding this comment.
nit (test-quality): This greps trainHydra's source for an exact kwarg string. A reformat or rename breaks it, and it checks nothing about behavior. Consider moving the ModelWrapper kwargs into a helper, as _model_trainer_defaults already does for the clip, and asserting on its output. Otherwise drop the test. The docstring should say what is guaranteed, not what used to happen.
The 300M HPT train step was launch-bound, not compute-bound: ~14,600 CUDA kernel launches per step, 230 ms of CPU against 111 ms of GPU. Two things dominated, and both are fixed here. PerSampleAugs called torchvision's ColorJitter once per image so each sample drew its own factors -- 64 Python round trips a step, each launching a few dozen kernels and copying its factors from the host. BatchedColorJitter draws the factors as one on-device tensor and runs each adjustment once over the batch; a degenerate factor range reproduces torchvision's scalar adjustment exactly (tests/unit/test_image_augs.py). Anything it cannot batch still goes through the per-image loop. model.compile.enabled=true routes to <algo>.compile_for_training(), which uses nn.Module.compile rather than torch.compile(module) so state_dict keys survive -- the wrapper form renames every parameter _orig_mod.* and would break checkpoints written before the flag. That only routes __call__, so HPT compiles the trunk, the flow head's denoiser and the image encoders (the stems and heads are reached through named methods), and PI's call site moves to __call__. On one H100 at batch 64 and the 360x640 frames mecka decodes to: HPT 245 -> 173 ms from the augs, -> 109 ms compiled (2.24x). Pi at batch 16: 827 -> 588 ms compiled (1.41x). In fp32 the compiled loss is bit-identical to eager. Also here: the flow head's attentions pass need_weights=False so they dispatch to scaled_dot_product_attention; frames decode to float32 instead of numpy's float64 default, halving the collate and H2D traffic (338 -> 169 MB a camera a batch, bit-identical); a per-camera per-step device->host sync is gone; and the batch is no longer deep-copied every step for an OT loss that is off. The all-zero (absent camera) guard moves into _apply_image_augs and becomes per-sample: the old test was one all() over the batch, so a batch mixing present and absent samples normalized the absent ones to -mean/std. The augs now run on every sample and the selector keeps the absent ones at zero, so RNG use no longer depends on the data. Jitter factors are drawn in float32 (uniform_ into bfloat16 gives ~40 distinct values; into uint8 it raises). enable_grad_norm was never passed to ModelWrapper, so pi0.5_base's false did nothing. It is wired now, and Pi follows openpi instead of opting out: it keeps logging the pre-clip grad norm every step and clips at openpi's 1.0, set as gradient_clip_val in the model config and handed to the Trainer unless trainer.gradient_clip_val is set. Pi recipes trained unclipped until now, so this is a behaviour change for every Pi run. One grad-norm pass remains (the second was removed with the log-only MAD detector). The compile comments note that CUDA-graph modes are where a rank can wedge in cudagraph_trees and keep its GPUs after a cancel. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw
f20e8aa to
a519280
Compare
bbbca3e to
9ce191d
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
Partial
|

Makes the HPT and Pi training step faster: batched image augs, an opt-in
torch.compileflag, and a few smaller fixes. It also changes how Pi trains (see below).Behaviour changes
enable_grad_normwas never passed toModelWrapper, so thefalseinpi0.5_base.yamldid nothing. It is wired through now.false, so it keeps loggingTrain/policy_grad_norms_*.gradient_clip_val: 1.0in its model config. trainHydra hands that to the Trainer (_model_trainer_defaults) unlesstrainer.gradient_clip_valis set explicitly.all()over the whole batch, so a batch mixing present and absent samples normalized the absent ones to-mean/std. The check moved into_apply_image_augsand runs per sample, on the device. The augs now run on every sample, so RNG use no longer depends on the data.Speed (one H100, 360x640 mecka frames)
model.compile.enabled=trueBatchedColorJitterdraws the factors as one on-device tensor instead of making 64 per-image Python round trips. A degenerate factor range matches torchvision exactly (tests/unit/test_image_augs.py). Anything it can't batch falls back to the per-image loop.model.compile.enabled=truecalls<algo>.compile_for_training(), which usesnn.Module.compilesostate_dictkeys keep their names;torch.compile(module)would prefix them_orig_mod.and break existing checkpoints. It is off by default, and the compiled fp32 loss was bit-identical to eager.need_weights=False, so it dispatches to SDPA.uniform_into bf16 gives only ~40 distinct values.Accuracy check
Measured with one-off harnesses that are kept out of this PR:
Notes
persistent_workers,pin_memory, more workers, max-autotune.reduce-overhead,max-autotune) are where a rank can wedge incudagraph_treesand keep its GPUs after a cancel. The compile config comments say so.Tests
test_image_augs.py(per-sample absent mask, float32 factors),test_image_decode_dtype.py, andtest_train_perf_wiring.py(enable_grad_normwiring, Pi clip default).🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw