Skip to content

feat(rollout): add Python 3.11 robot runtime and safety gates - #583

Open
ElmoPA wants to merge 18 commits into
elmo/pipeline-corefrom
elmo/rollout-py311
Open

feat(rollout): add Python 3.11 robot runtime and safety gates#583
ElmoPA wants to merge 18 commits into
elmo/pipeline-corefrom
elmo/rollout-py311

Conversation

@ElmoPA

@ElmoPA ElmoPA commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

feat(pipeline): consolidate Fold, PushT, overlays, and MoE

feat(rollout): unify Python 3.11 robot runtime

build(robot): validate amd64 Python 3.11 image

refactor(rollout): move policy ownership into algorithms

fix(robot): fail closed before live rollout

ElmoPA commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@ElmoPA
ElmoPA changed the base branch from main to graphite-base/583 August 23, 2026 20:29
@ElmoPA
ElmoPA changed the base branch from graphite-base/583 to elmo/pipeline-core August 23, 2026 20:29
@ElmoPA ElmoPA changed the title feat(pipeline): consolidate Fold, PushT, overlays, and MoE feat(rollout): add Python 3.11 robot runtime and safety gates Aug 23, 2026
@ElmoPA
ElmoPA force-pushed the elmo/rollout-py311 branch 4 times, most recently from 14ba921 to d69a3ef Compare August 24, 2026 02:31
@github-actions

Copy link
Copy Markdown

Claude Code Review

Review of PR #583

Summary

Large-scope PR that consolidates the robot rollout runtime to Python 3.11, moves live policy ownership into each Algo subclass, adds Fold-checkpoint compatibility shims, and introduces safety gates. The direction is sound but the diff mixes several concerns (Docker, checkpoint compat, EVA frames, safety gates, algo refactor) and there are a few real correctness issues that should block merge.

Key concerns

1. ACT.create_rollout_policy is broken — syntax/placement bug

In egomimic/algo/act.py, the new method is inserted into the class docstring, not the class body:

class ACT(Algo):
    """

    def create_rollout_policy(self, config):
        return Policy(self, config)
    BC training with a VAE policy.
    ...
    """

This means:

  • ACT.create_rollout_policy does not exist on the class — it inherits the base NotImplementedError.
  • The Policy stub with NotImplementedError is never reached because the method isn't wired up.
  • The docstring now contains garbage.

This will pass tests only because the base raise message is similar. Fix: move the method out of the docstring.

2. Removed pip install -e egomimic/robot/oculus_reader/. step is fine, but --no-deps on the source install is risky

RUN python -m pip install --no-deps -e . && \
    python -m pip install -e egomimic/robot/oculus_reader/.

If pyproject.toml gains a new runtime dep between the "dep prime" layer and the source install, --no-deps will silently drop it. The prime layer uses a stub egomimic/__init__.py copy of pyproject.toml — good — but any dep added after the cached layer will not appear. Consider either re-running a full resolve at the end or documenting this cache-bust requirement.

3. Legacy embodiment ID remap logic is dangerous

In checkpoint_compat.py:

LEGACY_EMBODIMENT_IDS = {8: 6, 18: 3}

Silent remapping of norm-stat embodiment IDs during checkpoint load is precisely the kind of thing that causes subtle training/rollout divergences. This is only asserted correct via the model-config SHA256 gate, which is good, but:

  • Please add a unit test that loads the exact fingerprinted checkpoint and confirms norm_stats.normalize produces identical numerics before and after the remap on a canonical batch.
  • The mapping direction (dataset 8→canonical 6, human 18→canonical 3) should be documented against the enum in egomimic/rldb/embodiment/embodiment.py so future enum changes don't drift.

4. prepare_rollout_checkpoint writes patched files next to source

Both PI.prepare_rollout_checkpoint and prepare_legacy_fold_rollout_checkpoint write .patched / .rollout-*.ckpt next to the source. If the source is on a read-only mount (bind-mounted external_ckpts — the docstring says it's mounted read-only), these writes will fail:

"The launcher bind-mounts the gitignored host external_ckpts/ directory read-only"

But _write_rollout_artifact writes into source.parent. This will crash at rollout time. Either:

  • Mount RW, or
  • Write patched artifacts to a separate writable cache dir (preferred).

5. PI.LOCAL_ROLLOUT_WEIGHT_PATH is hard-coded to a container path

LOCAL_ROLLOUT_WEIGHT_PATH = "/home/robot/robot_ws/egomimic/algo/pi_checkpoints/pi05_base_pytorch"

This will break any PI rollout outside the specific Docker container. Should be env-var driven (EGOMIMIC_PI_BASE_WEIGHTS with the current value as default).

6. EVA frame conversion — good addition, but coverage gap

eva_frames.py is exactly the right abstraction and the docstring note "Applying only one side is invalid" is well-taken. However:

  • The rollout-side application is not visible in this diff (truncated). Please confirm EvaObservationCodec / EvaActionCodec in egomimic/rollout/eva.py both call these helpers.
  • Add a round-trip test: dataset_ypr_pose_to_hardware_ypr(hardware_ypr_pose_to_dataset_ypr(x)) == x within tolerance.
  • Verify eva_to_zarr.py was updated to import from this new module rather than keeping its own copy of the matrix (the note claims it does, but not in diff).

7. PI torch.compile disable via del pi_model.sample_actions

if "sample_actions" in vars(pi_model):
    del pi_model.sample_actions

This is a fragile way to undo torch.compile — it depends on the compile call having bound the compiled fn as an instance attr shadowing the class method. If the compile pathway changes (e.g., _orig_mod wrapping), this becomes a silent no-op that still ships compiled code to the robot. Add an assertion after that the method is now the class method, or use a documented uncompile API.

8. Safety gate description vs. code visibility

The ROLLOUT_PY311.md says several gates are "currently disabled" (soft translation prompt, per-step Cartesian rotation-jump gate). Please either:

  • Land them enabled behind a flag, or
  • File tracking issues and reference them in the doc.

Shipping a rollout runtime that documents disabled safety gates in the same PR that adds "safety gates" to the title is confusing.

Suggestions

  1. Split this PR. At minimum: (a) Dockerfile/py311 runtime, (b) algo policy

Reviewed by Claude · Review workflow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant