Repository navigation
Conversation
9d0a133 to
624d97c
Compare
d53a9df to
715bba1
Compare
715bba1 to
b7053cc
Compare
624d97c to
97de94f
Compare
97de94f to
85c63c2
Compare
b7053cc to
30e5af4
Compare
38aaa92 to
e7eebed
Compare
345d201 to
c4a1766
Compare
… helpers (#653) > Branch 1 of 8 in the split of #640, which is the tip of the stack and carries the map of all eight. > Unit suite on this branch: **216 passed, 0 failed**. Groundwork for the 6D action pipeline, with no behavior change anywhere. pose_utils gains _ypr_to_rot6d / _rot6d_to_ypr (numpy, ZYX euler <-> the first two columns of the rotation matrix, Gram-Schmidt on the way back so a non-orthonormal model prediction still yields a proper rotation), matching the column convention of the torch helpers in action_utils. BIMANUAL_CARTESIAN_LAYOUTS maps a vector width (12/14 ypr, 18/20 6D) to its xyz / rot / grip channel indices; the norm-stat bounds check and the eval metrics both consume it to know which channel is which. action_chunk_transforms gains CartesianYPRToRot6D / CartesianRot6DToYPR, which convert a bimanual cartesian vector between xyz+ypr(+gripper) and xyz+rot6d(+gripper) while preserving the numpy/tensor type of the input. action_utils extracts the normalization math that was inlined in MultiDataset._apply_norm_one / ._apply_unnorm_one into module-level _apply_norm_one / _apply_unnorm_one, over a shared _norm_center_scale. _eps preserves the historical denominators exactly (std + 1e-6 for zscore, (hi - lo) + 1e-6 for minmax/quantile, i.e. half_range + 0.5e-6), so the three norm modes are numerically identical to before; MultiDataset now delegates. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…#654) > Branch 2 of 8 in the split of #640, which is the tip of the stack and carries the map of all eight. > Unit suite on this branch: **203 passed, 0 failed**. The pi0.5 converters stop doing rotation math. Actions now reach the algo already normalized and already in their model-native layout, so to32_norm_6d / from32_norm_6d are a pure rearrange into (and out of) openpi's 32-slot block layout: RobotBimanual's native 20D [L xyz 6d g | R xyz 6d g] IS that layout and only needs padding, HumanBimanual's 18D inserts a zero gripper slot per arm. BaseActionConverter raises on both, so an unregistered embodiment fails loudly. The four single-arm converters (RobotLeft/Right, HumanLeft/Right) are deleted along with PI._extract_xyz, which existed for the cam-frame MSE. PI's loss reduction and sample post-processing move into _reduce_loss and _postprocess_sampled_actions. The ypr->6D conversion is now a data transform. Eva gains cartesian_6d and cartesian_wristframe_6d, Human the same pair, each 6D-encoding the action AND the proprio ee_pose: normalized YPR saturates yaw/roll at +-pi (wraparound), so per-dim normalization is only meaningful on the continuous representation. get_revert_transform_list / the _build_*_revert_6d{,_wristframe}_ lists map a prediction batch back to cam-frame ypr for the viz overlay. Eva defaults to cartesian_wristframe_6d (eva, cotrain_pi_base, industry_eva_pi; cotrain_pi_lang_wrist now composes the same as cotrain_pi_lang). Human data configs move to cartesian_6d with pad_proprio_gripper, which pads the proprio ee_pose 18 -> 20 (zero grip slots at 9/19) so the pi0.5 State: prompt bins line up positionally with the robot 20D layout; UnpadGripperZeros inverts it in the revert lists. HPT dims follow: eva 14 -> 20, human 12 -> proprio 20 / action 18. Also here, because they are the same widening: SplitKeys rejects a layout whose widths do not sum to the input (an evaluator revert list built for 12-dim ypr fed an 18/20-dim 6D batch used to slice silently), and vendor-tagged human embodiment names (MECKA_/SCALE_/ARIA_/LIGHTWHEEL_*) alias onto HUMAN_* through canonical_embodiment_name, so pins and embodiment checks accept the legacy metadata. Robot (EVA_*) names are never aliased. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> 🤖 Generated with [Claude Code](https://claude.com/claude-code)
… zarrs (#655) > Branch 3 of 8 in the split of #640, which is the tip of the stack and carries the map of all eight. > Unit suite on this branch: **460 passed, 0 failed**. compute_hand_pose_xyzquat built the palm normal as cross(thumb_dir, pinky_dir). Thumb and pinky are anatomically swapped between hands, so that cross product already mirrors chirality for free; applying rot_left on top of it flipped x/y a second time and landed the LEFT hand on the RIGHT hand's spatial convention. Both hands now use the same rot_right relabel. Reconverting every mecka zarr would be the obvious fix, so this avoids it: rot_left == rot_right @ diag(-1, -1, 1), which makes right-multiplying the stored left pose by Rz(180 deg) exactly equivalent to reconverting with rot_right. RotateLocalFrame does that right-multiply (R_new = R_old @ R_fix), relabelling the pose's own axes without moving its origin, on (7,) poses and (T, 7) chunks alike, and leaves zero-norm quaternions alone because those mark padded frames. Human.get_transform_list(fix_mecka_left_wrist=True) prepends it to the raw LEFT pose keys before any frame math: ee_pose in the cartesian modes, wrist_pose in the keypoints modes, since the converter wrote the same rotation into both. data/mecka.yaml opts in; aria and scale must not, because they run different converters, and neither must mecka data reconverted after this commit, which would double-flip. test_wrist6d_roundtrip runs the real transform lists on synthetic world-frame poses, parametrized over the flag, and compares every stage against plain numpy/scipy SE(3) math that shares no code with the pipeline. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> 🤖 Generated with [Claude Code](https://claude.com/claude-code)
e7eebed to
ce8666e
Compare
c4a1766 to
f748613
Compare
f748613 to
17a05a0
Compare
ce8666e to
2ca1d41
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 high, 2 medium, 3 low, 5 nit.
|
|
||
|
|
||
| class RobotBimanualCartesianEuler(BaseActionConverter): | ||
| class RobotBimanualCartesian6D(BaseActionConverter): |
There was a problem hiding this comment.
high (backward-compat): This rename breaks loading of every pi0.5 checkpoint trained from main. Their saved config_tree has _target_: egomimic.utils.action_utils.RobotBimanualCartesianEuler (and HumanBimanualCartesianEuler), and ModelWrapper re-instantiates it, so rollout, eval and ckpt resume fail with 'Error locating target'. The class bodies are unchanged, so keep the old names as aliases (RobotBimanualCartesianEuler = RobotBimanualCartesian6D, likewise for Human) with a one-line comment that saved checkpoints reference them.
| ) | ||
| mode, source = self.LEGACY_TRANSFORM_MODE, "14-D action width" | ||
| print(f"[rollout] Eva transform_list mode ({source}): {mode}") | ||
| return None if mode == self.LEGACY_TRANSFORM_MODE else mode |
There was a problem hiding this comment.
medium (correctness): return None if mode == self.LEGACY_TRANSFORM_MODE also catches an explicit --transform-mode cartesian_wristframe_ypr. With it, revert_transform_list ends up None and wrist-relative xyz/ypr predictions go to cam_frame_to_base_frame as if they were camera-frame poses. Someone with an old wrist-frame ypr checkpoint is exactly who would pass this flag. Only the unrecorded 14-D fallback branch should return the legacy no-revert sentinel; an explicitly given mode should always build get_revert_transform_list(mode). A test of the init wiring, not just _checkpoint_transform_mode, would catch this.
|
|
||
|
|
||
| class RobotBimanualCartesianEuler(BaseActionConverter): | ||
| class RobotBimanualCartesian6D(BaseActionConverter): |
There was a problem hiding this comment.
medium (compatibility): The rename breaks loading of every checkpoint whose saved config_tree names egomimic.utils.action_utils.RobotBimanualCartesianEuler or HumanBimanualCartesianEuler, because ModelWrapper re-instantiates from that tree. That covers the 6D Pi runs already trained on this stack. Keep aliases (RobotBimanualCartesianEuler = RobotBimanualCartesian6D, HumanBimanualCartesianEuler = HumanBimanualCartesian6D) with a one-line comment saying they exist so saved configs still load.
| # constlr_5e5 ablation whose keypoint arm fit best). | ||
| _target_: egomimic.pl_utils.pl_model.ModelWrapper | ||
| robomimic_model: | ||
| _target_: egomimic.algo.hpt.HPT |
There was a problem hiding this comment.
low (duplication): This file is a copy of hpt_bc_mecka_6d_300M.yaml (trunk, head, shared stems, encoder, augs, optimizer) apart from dims, ac_keys, 6dof and the human stem. Make it defaults: [hpt_bc_mecka_6d_300M, _self_] and override only those keys (null state_ee_pose, add state_keypoints). A width change then lands in one file.
| human_bimanual: | ||
| _target_: egomimic.rldb.zarr.zarr_dataset_multi.MultiDataset._from_resolver | ||
| resolver: | ||
| _target_: egomimic.rldb.zarr.zarr_dataset_multi.S3EpisodeResolver |
There was a problem hiding this comment.
low (duplication): mecka_all_6d, mecka_fold_freeform_6d, mecka_fold_freeform_opsplit_6d and mecka_fold_flagship_opsplit_hpt_6d each repeat the same S3EpisodeResolver, cartesian keymap, cartesian_wristframe_6d transform (stride 1, fix_left_wrist_convention, pad_proprio_gripper), loader params and knob comment. Consider a shared base (e.g. data/mecka_6d_base.yaml) that each slice inherits, setting only filters, split mode and the operator list. Retiring fix_left_wrist_convention would then be a single edit.
| try: | ||
| from PIL import Image, ImageDraw | ||
| except Exception: | ||
| Image = None |
There was a problem hiding this comment.
nit (dead-code): PIL is not optional here: decode() imports it, and line 384 calls Image.fromarray without a check. Import from PIL import Image, ImageDraw at the top and drop the try/except and the if Image branches.
| eps: 1e-8 | ||
| weight_decay: 0.0 | ||
|
|
||
| # Constant LR after warmup (matches the remote pi0.5 training stack). The |
There was a problem hiding this comment.
nit (comments): This comment describes the previous cosine schedule, which belongs in the commit message. Reduce it to the reason, e.g. # Constant after warmup: a fixed-horizon cosine decays to 0 regardless of run length.
| # is a CLI override, not a data config; one data config per dataset slice. | ||
| raw = yaml.safe_load((DATA_DIR / f"{name}.yaml").read_text(encoding="utf-8")) | ||
| body = {k: v for k, v in (raw or {}).items() if k != "defaults"} | ||
| if not raw.get("defaults") or not body: |
There was a problem hiding this comment.
nit (test-quality): (raw or {}) guards the comprehension, but raw.get("defaults") on this line raises AttributeError for an empty YAML. Use raw = yaml.safe_load(...) or {} once and drop the inline guard.
| assert unseen.filters.filter_lambdas[-1].endswith(f" in {ops}") | ||
|
|
||
|
|
||
| def test_bounds_quantiles_include_the_observed_extremes(): |
There was a problem hiding this comment.
nit (test-quality): These bounds and packed-width loss tests now sit under the 'train_viz second loader' header but don't test it, and the module docstring no longer mentions the packed-width loss reduction that test_pi_loss_is_reduced_over_the_packed_width still covers. Leave them where they were (or give them their own header) so the diff stays limited to the rename, and restore the docstring mention.
| emb, | ||
| (f"+data.train_datasets.{emb}.resolver.key_map.annotation_key=annotations",), | ||
| ) | ||
| # data/eva.yaml already emits the continuous-6D (wrist-frame) layout Pi |
There was a problem hiding this comment.
nit (comments): This rewrite doesn't change behaviour, and the comment describes data/eva.yaml's 6D layout, which _pi doesn't touch. Revert to the original tuple form or drop the comment.
The fold recipes this stack was built for: mecka_all_6d, mecka_fold_freeform_6d
and its opsplit variant (wrist frame; cam frame is a transform_list.mode
override rather than a near-duplicate config, and test_data_configs_compose
rejects any data config that differs from its parent only in that mode),
mecka_fold_flagship_opsplit_hpt_6d and
mecka_fold_freeform_opsplit_hpt_keypoints, plus the two 300M HPT models and
pi0.5_bc_mecka_6d.
Defaults, each for a failure seen in a run:
- checkpoints every 50 epochs, saved on train-epoch end. With
check_val_every_n_epoch > 1 the validation-end hook only fires on val epochs,
so every_n_epochs silently skipped the others.
- ddp_pi inference_mode: false. The compiled pi sampler does in-place setup on
its static buffers during cudagraph capture, which is illegal on inference
tensors ("Inplace update to inference tensor outside InferenceMode"); no_grad
is equivalent for eval and creates none.
- ddp_pi validates every 20 epochs instead of 200, and eval_hpt renders viz
every 200.
- pi0.5 gets a constant LR after warmup. The cosine schedule decayed to 0 over
num_training_steps=60000, so runs past ~60k steps trained at ~0 lr and
shorter runs never saw the configured lr for most of training.
- The Qwen stem resolves a repo id to its cached snapshot directory when
offline: transformers >= 4.57 probes the Hub while loading a tokenizer by
repo id even under HF_HUB_OFFLINE, which raises on compute nodes with no
internet. Loading from the directory skips the probe. Offline is read the
way huggingface_hub reads it -- 1 / ON / YES / TRUE in any case, from the
live environment -- rather than by matching the literal "1", which left
HF_HUB_OFFLINE=true hitting the probe the helper exists to avoid.
viz_mecka_episode.py and viz_eva_episode.py render an episode straight from
its zarr, which is how the left-wrist convention bug was found. Both take the
dataset dir from egomimic.utils.paths.default_dataset_dir(), which reads
paths/default.yaml, instead of each re-stating the cluster path.
HumanBimanualCartesianEuler / RobotBimanualCartesianEuler are renamed to
HumanBimanualCartesian6D / RobotBimanualCartesian6D: both pack xyz + rot6d and
have not touched an Euler angle since the 6D conversion, and this is the commit
that makes the human one the converter for a new default recipe.
Also prunes unit tests whose assertions another test already makes on the same
code path, and parametrized cases that repeat a stricter test's compose call.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…evert its outputs NOT TESTED ON HARDWARE. The rollout hard-coded cartesian_wristframe_ypr inputs and used the policy's predictions as-is. Now that eva checkpoints can be trained in several modes (cartesian, cartesian_6d, cartesian_wristframe_6d), that silently feeds a model the wrong inputs and sends the wrong frame to the arms. It now reads data.train_datasets.<name>.resolver.transform_list.mode off the checkpoint's own config, builds the matching input pipeline, and maps predictions back to camera-frame xyz+ypr+gripper through Eva.get_revert_transform_list. The revert needs the un-normalized proprio, which process_batch_for_training normalizes in place, so the raw ee_pose is cloned before that call. A checkpoint whose config records no mode keeps exactly the old behaviour -- legacy inputs, no revert -- and says so loudly, so this cannot silently change what an existing checkpoint does. test_eva_revert_for_rollout_recovers_camframe_ypr covers the revert math by cross-checking the wrist-frame revert against the cam-frame cartesian_6d one, which never enters the wrist frame -- so the frame composition is genuinely validated, though not against independently written SE(3) math. The hardware path itself still needs a real rollout. Both get_transform_list calls here pass allow_legacy_rotation=True: this is inference, and a checkpoint that predates the 6D conversion records exactly the modes the training-time guard rejects. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01VBmhpVy7ZcMtsBJWh38cJp
…eckpoint
The rollout looked for data.train_datasets.<eva>.resolver.transform_list.mode
in the checkpoint's config tree, but _build_model_config_tree saves only
model, so every checkpoint fell back to the legacy 14-D ypr pipeline and a
6D policy would have produced garbage commands. trainHydra now records
{dataset: transform_list.mode} as data_transform_modes in that tree, and the
rollout reads it from there. A checkpoint without it (every one trained
before this commit) is taken as legacy only when its saved actions_cartesian
width is 14; otherwise the rollout refuses to guess and asks for the new
--transform-mode flag.
EMBODIMENT_MAP still held the pre-collapse ids (8/7/6 for both/left/right);
it now reads them from EMBODIMENT.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01GqAcWA2Lk9zubUhKX5DW1v
17a05a0 to
dbfa725
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
Not fixed
|

This PR is the tip of an eight-branch split. The 111-file commit that used to live
here was broken up so each branch carries one feature. #640 keeps its original head branch
(
ryanco/infra-port) and its review history, but its base isryanco/eval-metrics-train-vizand its diff is only the last slice. Review comments above this line may point at code that
has moved to one of:
rot6d-coreembodiment-6d-modes*_6dmodes; vendor embodiment aliasingmecka-left-wrist-fixRotateLocalFramefor existing zarrshand-keypoints-144dnorm-stats-poolingNORM_MIN_RANGEdegenerate-channel guard,pool_horizon, the precompute scriptdataset-robustnessrequire_annotationseval-metrics-train-vizaction_metrics, train-split viz head, viz cadence, reverse-KL removalThe stack was rebased onto current main. The single-operator flagship config
(
mecka_fold_flagship_topop_hpt_6d) is kept out of this PR.What is left in this PR
Recipes, models and defaults
mecka_all_6d,mecka_fold_freeform{,_opsplit}_6d(wrist frame; cam frame is atransform_list.mode=cartesian_6doverride, and a guard test rejects data configs that onlyswap the mode),
mecka_fold_flagship_opsplit_hpt_6d,mecka_fold_freeform_opsplit_hpt_keypoints,and the composed recipes
train_zarr_mecka_flagship_6d_hpt/train_zarr_mecka_kp_wrist_hpt.hpt_bc_mecka_6d_300M/hpt_bc_keypoints_wrist_300M,pi0.5_bc_mecka_6d, and constant-LR-after-warmup onpi0.5_base. The 6D PI converters arerenamed from
*CartesianEulerto*Cartesian6Dto match what they pack.ddp_piinference_mode=false+ validation every 20 epochs, checkpoints every 50on train-epoch end; the HPT Qwen stem loads the local snapshot when offline.
viz_mecka_episode.py,viz_eva_episode.py, withegomimic/utils/paths.pyreadingthe dataset dir from the hydra paths config.
Rollout revert
trainHydrasaves each train dataset'stransform_list.modewith the checkpoint config.rollout.pyreads the Eva mode from there and maps predictions back to camera-frame ypr viaEva.get_revert_transform_list. Checkpoints without a recorded mode keep the old inputs and norevert. Not tested on hardware.
Testing
636 passed, 2 skipped, 0 failed at this branch; every branch of the split green on its own
suite, in order 216 / 203 / 460 / 467 / 487 / 496 / 634 / 636.
commit, so they cover the stack as a whole rather than this branch alone. Final epoch:
PI eva xyz MSE 9.3e-5 / rotation 3.2°; PI mecka keypoint error 24.9 mm; HPT eva rotation 21.6°.
chunk on all four; pooled is equal or better late.
rollout.pyrevert: not tested on hardware.🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw