Skip to content

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

Open
ElmoPA wants to merge 20 commits into
graphite-base/583from
elmo/rollout-py311
Open

feat(rollout): add Python 3.11 robot runtime and safety gates#583
ElmoPA wants to merge 20 commits into
graphite-base/583from
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

This PR consolidates the robot rollout runtime onto Python 3.11, moves policy ownership into each algorithm class, adds safety gates for live rollout, and introduces a narrow compatibility shim for one specific frozen Fold checkpoint. It's a large, mostly well-structured change with strong safety framing, but a few concerns warrant attention before merge.

Key concerns

1. Hardcoded secret was previously in Dockerfile 🚨

The removed Dockerfile chunk contained a Graphite auth token (SX4r5gXXW83uNr4x1USeYFcc2VUEzp5YfBOGJVP7xXiGUk4vhEYEnPObeTY8). Removing it from the current image is correct, but this token is now in git history and must be revoked immediately if it's still valid. Please rotate it and confirm in the PR before merging.

2. ACT.create_rollout_policy is defined inside a docstring

In egomimic/algo/act.py:

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

The method is embedded in the class docstring and will not be a real method. Either this is intended (so ACT falls back to the base NotImplementedError, which matches the stated "explicit unsupported-live-EVA boundary") or it's a bug. If intended, please remove the misleading fake definition; if not, move it out of the docstring. As written, this is confusing at best.

3. Norm-stat embodiment ID remapping (8→6, 18→3)

_remap_norm_stats_state silently rewrites saved embodiment IDs. This is a very sharp edge: if the canonical enum for eva_bimanual/human_bimanual shifts again, this hardcoded map will produce plausible-looking but wrong stats. Please:

  • Assert that LEGACY_EMBODIMENT_IDS[8] == get_embodiment_id("eva_bimanual") and same for 18 → human_bimanual at load time, rather than trusting the literal 6/3.
  • Add a regression test that constructs a minimal legacy norm_stats_state and confirms round-trip normalize/unnormalize matches expectations.

4. PI checkpoint patching writes next to source

PI.prepare_rollout_checkpoint does torch.save(checkpoint, checkpoint_path + ".patched") unconditionally on every rollout, in the same directory as the source. For multi-GB PI checkpoints, this is slow, may race if multiple rollouts start simultaneously, and won't work if the checkpoint directory is read-only (which is the pattern used for the Fold artifact above). Consider:

  • Using the same fingerprint-based caching pattern as prepare_legacy_fold_rollout_checkpoint
  • Writing to a configurable cache dir, not next to the source
  • At minimum, skip if .patched already exists and its mtime ≥ source

5. Silent torch.compile disable via vars() mutation

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

This is fragile — it depends on torch.compile having monkey-patched the instance attribute in exactly that way. If PyTorch changes how compile attaches, this becomes a silent no-op and rollout runs the compiled path (which was presumably broken enough to warrant disabling). Please assert post-condition that the method now resolves to the class-level definition, or use torch._dynamo.reset() / a documented API.

6. Cartesian rotation-jump gate disabled

The doc says: "The live soft translation prompt and per-step Cartesian rotation-jump gate are currently disabled." Given the EVA pose-frame bug that this PR is fixing, disabling the rotation gate seems like exactly the wrong direction. Was this intentional, and is there a tracking issue? Please add one and reference it in ROLLOUT_PY311.md.

Suggestions

  • Tests referenced in the doc (tests/test_arx5_backend.py, tests/test_eva_frames.py, tests/test_pipeline_rollout.py) — the diff was truncated so I couldn't confirm they exist and cover the round-trip. Please confirm at minimum:
    • EVA frame conversion round-trips within tolerance for non-identity rotations
    • prepare_legacy_fold_rollout_checkpoint rejects checkpoints with wrong model SHA
    • The gripper [0,1] clamp is applied only on decode, not corrupting training
  • The RolloutPipeline graph construction in PipelineAlgo.create_rollout_policy mutates the frozen head's inference schedule as a side effect of configure_frozen_fold_live_sampling. Consider making this explicit at the policy call site rather than hidden inside a "create" method — a future caller instantiating two policies would double-configure.
  • EVA_DATASET_FROM_HARDWARE_ROTATION — good that it's a shared module with eva_to_zarr.py. Please add an assertion in eva_to_zarr.py (or a test) that imports from eva_frames.py so a future refactor can't silently diverge them.
  • The forward_rollout normalize_raw opt-out parameter is easy to misuse. Document loudly that observation_is_normalized=True is a footgun and used only by tests.
  • .gitattributes with *.patch whitespace=-blank-at-eol — unrelated to the PR; consider splitting.
  • Doc lists a bonjour RTX 4090 benchmark showing 100ms at 16 steps vs. 33ms control period. The doc acknowledges this, but the rollout code should probably log a warning at startup so operators aren't surprised by rate dips.

Verdict: Request Changes

Primary blockers: (


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.

2 participants