Repository navigation
Conversation
cc32016 to
fc6258a
Compare
e7468e3 to
b086056
Compare
fc6258a to
ad2cc51
Compare
f9a08b1 to
73fe194
Compare
d343a85 to
2302dcf
Compare
73fe194 to
11cbb10
Compare
2302dcf to
e680eb5
Compare
11cbb10 to
868598a
Compare
e680eb5 to
cb1db1d
Compare
868598a to
133be3f
Compare
cb1db1d to
f6d6d51
Compare
133be3f to
e9ac33c
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, 3 nit.
| action_pad_mask = processed_batch[embodiment_id].pop( | ||
| "action_pad_mask", None | ||
| ) | ||
| if self.use_pad_mask and action_pad_mask is not None: |
There was a problem hiding this comment.
low (correctness): use_pad_mask only takes effect through DenoisingPolicy.compute_loss. PolicyHead.compute_loss (MLPPolicyHead / MultiBlockTransformerDecoder) still computes LOSS(pred_action, target_action) and ignores data["pad_mask"]. On a non-diffusion HPT config the flag is accepted and the mask is built and validated, but the padded tail is still fully supervised. Either apply the mask in PolicyHead.compute_loss or raise when use_pad_mask is set and the head is not a DenoisingPolicy.
| stride=stride, | ||
| mode="xyzwxyz", | ||
| ), | ||
| InterpolatePadMask(new_chunk_length=chunk_length, stride=stride), |
There was a problem hiding this comment.
nit (duplication): InterpolatePadMask(new_chunk_length=chunk_length, stride=stride) is pasted into five builders (eva.py x2, human.py x3). A future builder that forgets it only shows up as a horizon-mismatch ValueError once use_pad_mask is on. Since the transform is idempotent, consider appending it once in a shared helper, or after the builder in get_transform_list.
| device = processed_batch[embodiment_id][ac_key].device | ||
| processed_batch[embodiment_id]["pad_mask"] = torch.ones( | ||
| B, S, 1, device=device | ||
| # Dropped either way: downstream sees the same keys as before. |
There was a problem hiding this comment.
nit (comments): "downstream sees the same keys as before" describes the diff. Say why instead, e.g. # action_pad_mask is consumed here; only pad_mask goes downstream.
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def _pad_mask_transform_cls(): |
There was a problem hiding this comment.
nit (test-quality): InterpolatePadMask exists now, so _pad_mask_transform_cls ("so the module still collects before the transform exists") is leftover TDD scaffolding. Import it at module top and drop the helper.
ZarrDataset emits a real action_pad_mask (1.0 per real frame, 0.0 per repeat-last pad, honouring the annotation-cutoff EOS clamp) next to every horizoned action chunk. A new InterpolatePadMask transform carries it through the 30 -> 100 upsampling on the interpolators' own grid: an output step is real only if both source frames it interpolates between are real. HPT gains a use_pad_mask algo key (default false, declared in the two flagship model yamls; every other HPT config takes the __init__ default). With it on, process_batch_for_training builds pad_mask (B, S, 1) from the dataset mask instead of ones; DenoisingPolicy.loss_fn takes the mask and means the squared error over the valid steps only. With it off nothing changes: the mask is dropped from the batch and pad_mask stays all ones, which is exactly F.mse_loss. It ships off to match openpi, which asks LeRobotDataset for action_is_pad and then ignores it (Pi0.compute_loss means over the whole horizon). LeRobot's own policies do mask, but reduce with .mean() over all elements including the zeroed ones, so their loss shrinks as padding grows; this divides by the valid count instead, which keeps the gradient magnitude constant. Measured on the three pinned flagship episodes, 6.4-7.6 % of supervised action steps are repeat-last padding and 12.8-15.2 % of samples contain some, so turning it on changes the loss: a run before and a run after are not comparable, and the layout-aware eval metrics would have to be masked in the same change or train and val would be measuring different things. Padding is still repeat-last and no tail samples are dropped. 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_01VBmhpVy7ZcMtsBJWh38cJp
f6d6d51 to
63e3374
Compare
e9ac33c to
19e047a
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
Not fixed
|

Adds an opt-in mask so the HPT loss can skip the repeat-last action padding at episode and annotation tails. The dataset now emits the mask; the loss uses it only when
use_pad_mask: true.Behaviour changes
None by default.
use_pad_maskisfalseinhpt_bc_mecka_6d_300M.yamlandhpt_bc_keypoints_wrist_300M.yaml, and every other HPT config takes the__init__default offalse. With it off, the mask is dropped from the batch andpad_maskstays all ones, which is exactlyF.mse_loss.Turning it on changes the loss: the repeat-last padding stops being supervised, so runs with and without it are not comparable. On the three pinned flagship episodes, 6.4–7.6 % of supervised action steps are repeat-last padding and 12.8–15.2 % of samples contain some. The layout-aware eval metrics would have to be masked in the same change, or train and val would measure different things.
What changed
ZarrDatasetemitsaction_pad_mask(1.0 per real frame, 0.0 per repeat-last pad) next to every horizoned action chunk. It honours the annotation-cutoff EOS clamp.InterpolatePadMasktransform carries the mask through the 30 → 100 upsampling on the interpolators' own grid. An output step is real only if both source frames it interpolates between are real. It is a no-op without the key and idempotent, so a mode that chains two builders (include_ee_pose) upsamples once. It is added to the eva and human cartesian and keypoint transform lists.use_pad_maskon,process_batch_for_trainingbuildspad_mask(B, S, 1) from the dataset mask instead of ones, and raises if the mask length doesn't match the action horizon.DenoisingPolicy.loss_fntakes the mask and averages the squared error over the valid steps only. An all-padding chunk contributes 0, not NaN.Notes
action_is_padfromLeRobotDatasetbut never uses it (Pi0.compute_lossaverages over the whole horizon)..mean()over all elements including the zeroed ones, so their loss shrinks as padding grows. This divides by the valid count instead, which keeps the gradient magnitude constant.Tests
tests/unit/test_loss_masking.py: the dataset mask (tail chunks, annotation cutoff, survival through bounds check and normalization),InterpolatePadMask(30 → 100, stride, no-op, idempotent),loss_fnagainstF.mse_loss, and theuse_pad_maskflag inprocess_batch_for_trainingand the model yamls.tests/unit/test_action_chunk_transforms.py: the eva and human transform-list snapshots now includeInterpolatePadMask.🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw