feat(envs): make LIBERO sim control frequency configurable

[shuheng-liu]

What this does

🗃️ Feature / 🐛 Bug

The LIBERO eval env built OffScreenRenderEnv with only bddl_file_name + camera sizes, so the simulation always ran at robosuite's hardcoded control_freq=20 Hz. The EnvConfig.fps field existed but was dead — it was never placed in gym_kwargs, so it never reached env construction. (Upstream LeRobot's LIBERO env carries the same dead field.)

This wires env.fps through to robosuite's control_freq:

gym_kwargs  ->  LiberoEnv.__init__(control_freq=...)  ->  OffScreenRenderEnv(control_freq=...)

The LIBERO default is set to 20 (robosuite's native rate, so the no-arg path is unchanged), with a fps > 0 guard.

Why 20 is the right default. An fps ablation on a pi05_mem checkpoint (libero_10, 64 episodes per setting, single GPU, identical seed/config — only env.fps varied):

sim control_freq	libero_10 success
20 Hz (default)	92.19%
10 Hz	39.06%

20 Hz reproduces the training-time eval; 10 Hz collapses it. The data is authored at the sim's native 20 Hz.

Dataset: physical-intelligence/libero → TensorAuto/libero (20 fps)

Every reference to physical-intelligence/libero is re-pointed to TensorAuto/libero and dataset_mixture.action_freq is set to 20 across the demo configs and docs. TensorAuto/libero is published — a v2.1 re-export of the same frames/episodes (1693 episodes / 273465 frames, verified equal) with the fps label corrected 10 → 20 Hz (no resampling). Upstream physical-intelligence/libero ships a wrong fps=10 label even though its actions are authored at 20 Hz; the corrected copy makes the dataset rate, action_freq, and sim control_freq all line up at 20.

Compatibility notes

• env.fps is now active. If your own config explicitly set it to a non-20 value (the old example configs shipped fps: 10), that value was previously ignored and will now take effect — set it to 20, or remove it, to keep the prior 20 Hz behavior.
• physical-intelligence/libero is intentionally removed from standard_data_format_mapping.py (not aliased), with an explanatory comment, to discourage the wrongly-labeled dataset. A config still pointing at it (without its own data_features_name_mapping) will error at dataset build — switch to TensorAuto/libero.

How it was tested

• Added tests/envs/test_libero_control_freq.py: CPU mock tests (env.fps → gym_kwargs["control_freq"]; OffScreenRenderEnv built with the configured control_freq; default 20; rejects fps <= 0) plus a @pytest.mark.gpu real-sim readback.
• pre-commit run --all-files and pytest -m "not gpu" -n auto pass; the gpu test passes on a CUDA box.
• fps ablation above (20 Hz → 92.19% vs 10 Hz → 39.06% on libero_10).
• Verified TensorAuto/libero metadata: fps=20, v2.1, same episode/frame counts as the upstream copy.

How to checkout & try? (for the reviewer)

pytest -sx tests/envs/test_libero_control_freq.py -m "not gpu"

The sim frequency is now a config field / CLI override on any libero config:

opentau-eval --accelerate-config configs/examples/accelerate_ddp_config.yaml --config_path=configs/libero/reproduce_pi05_libero.json --env.fps=20

Checklist

• I have added Google-style docstrings to important functions and ensured function parameters are typed.
• My PR includes policy-related changes.
  • If the above is checked: I have run the GPU pytests (pytest -m "gpu") and regression tests.

Note: Before submitting this PR, please read the contributor guideline.

[shuheng-liu]

Review: LIBERO control-freq wiring

Overview. Wires the previously-dead EnvConfig.fps through to robosuite's control_freq: LiberoEnv.gym_kwargs["control_freq"] → create_libero_envs → LiberoEnv.__init__ → OffScreenRenderEnv(control_freq=...). Default set to 20 (robosuite's native LIBERO rate), and the 5 stale libero configs (fps 10/30) bumped to 20.

Bottom line: correct and well-tested — good to merge after the optional nits below. Verified the call chain end-to-end; the default is behavior-preserving (20 = robosuite's default, so the no-arg path is unchanged), and the 39% vs 92% ablation is a convincing justification for both the default and the config bumps.

Verified

• Propagation is real: gym_kwargs → create_libero_envs pops task_ids and forwards the remainder via _make_env_fns/_make_env as **kwargs into LiberoEnv.__init__, which threads control_freq into OffScreenRenderEnv.
• fps really was dead before: the only other reader in the eval path is env.unwrapped.metadata["render_fps"] (a hardcoded class constant = 80), not the config field — so nothing else regresses.
• Config coverage: all 5 "type": "libero" configs under configs/ are updated; none missed.
• Tests are well-targeted: default value, gym_kwargs propagation (no-accelerator branch patched), mocked construction at an explicit 13 and at the default 20, plus a real-sim GPU readback. The mocked assertions on call_args.kwargs correctly match the all-keyword OffScreenRenderEnv(**env_args) call.

Optional suggestions (none blocking)

1. Field name vs. meaning. The field stays fps but now drives a control-loop rate (control_freq). Numerically equal here but conceptually distinct; the new docstring handles it well, so reusing fps (for LeRobot compat) is fine — just flagging the conflation.
2. Base-class EnvConfig.fps left at 30 with the old "stepping/rendering" docstring (configs.py:128/145), while LiberoEnv overrides to 20. Harmless since LIBERO is the only env, but a reader of the base sees a stale description — consider aligning the base docstring or noting the override.
3. No fps > 0 guard. A non-positive value would pass straight to robosuite and fail obscurely. Pre-existing (no validation before either), low priority — a one-line check in __post_init__ would be friendlier.
4. Pre-existing / out of scope: saved eval videos use render_fps = 80 regardless of the sim rate, so playback speed doesn't reflect the 20 Hz sim. Not introduced by this PR; noting only so it's on the radar.

Risk: low. Default path unchanged; the only behavioral delta is for configs that explicitly set fps 10/30, which are intentionally corrected. No new deps; CPU tests are self-contained.

---

Generated by Claude Code

[shuheng-liu]

Thanks for the review. Addressed in 804dbdb:

• Comment 2 (stale base docstring): EnvConfig.fps's docstring now says it's a stepping frequency in Hz whose exact meaning is env-specific, and notes that LiberoEnv overrides the default to 20.
• Comment 3 (no fps > 0 guard): added a guard in LiberoEnv.__post_init__ that raises ValueError for a non-positive value, plus a parametrized test (fps=0, fps=-5).

Leaving as-is:

• Comment 1 (fps name vs control_freq meaning): intentional. Reusing the existing fps field (rather than adding a separate control_freq) keeps config parity with upstream LeRobot and matches the goal of making the already-present knob actually take effect. The docstrings now spell out the control-loop semantics.
• Comment 4 (saved videos use render_fps=80 regardless of sim rate): real but pre-existing and orthogonal to this change, so out of scope here.

[shuheng-liu]

Re-review (updated for 804dbdb + 627c455)

Prior nits — resolved ✅

• Base EnvConfig.fps docstring now states the meaning is env-specific and that LIBERO overrides to 20 (configs.py).
• fps <= 0 now rejected in LiberoEnv.__post_init__ with a clear message, plus a parametrized test (0, -5).

The env.fps → control_freq wiring itself is unchanged and still good.

New change needs attention: action_freq 30.0 → 10.0 (commit 627c455)

This commit is not the same concern as the rest of the PR. action_freq is a training-time DatasetMixtureConfig field (configs/default.py:268) consumed in datasets/factory.py:149 to build delta_timestamps as i / action_freq — it controls how dataset actions are resampled during training/tokenizer-fit, not the eval sim rate. Three points:

1. Direction is correct. Most LIBERO training configs already use action_freq: 10.0 (pi05_training, pi06_training, pi07_libero, reproduce_pi05_libero, …); pi_config.json and value_config.json at 30.0 were the outliers, and both train on physical-intelligence/libero. Aligning them to 10 matches the dataset's labelled timestamp spacing so delta_timestamps line up 1:1 with stored frames. Good fix.

2. Consistency gap — please confirm intentional vs. missed. configs/examples/advantage_config.json is the same kind of mixture over physical-intelligence/libero (two datasets, lines 4 & 9) and is still at action_freq: 30.0 (line 18) — the PR didn't touch it. By this commit's own stated goal ("align physical-intelligence/libero action_freq to 10") it looks like it should be 10 too, or be explicitly excluded. (For contrast, pi05_libero_eval_config.json and pi05_inference_config.json correctly stay at 30 — their mixture is lerobot/droid_100; LIBERO there is only the eval env.)

3. Undocumented + surface-level contradiction. The PR body / "What this does" only describes the env.fps wiring; the action_freq change isn't mentioned. A reader also hits an apparent contradiction: the PR argues LIBERO data is effectively 20 Hz (→ sim control_freq=20) yet sets training action_freq=10. These are reconcilable — 10 Hz is the dataset's stored/labelled frame spacing (pick consecutive demo frames), while 20 Hz is how those frames are stepped in the sim — but the PR should say so in one sentence, or split the action_freq alignment into its own PR. (Also note: unlike the well-tested env.fps path, the action_freq edit is config-only with no test guarding it.)

Bottom line

The env.fps/control_freq work is solid and merge-ready. Before merging, please (a) note the action_freq change in the PR description (or split it out) with the 20-vs-10 rationale, and (b) decide on advantage_config.json — align it to 10 or confirm it's intentionally excluded.

---

Generated by Claude Code

[shuheng-liu]

Re-review (updated for 9818a2 — now 4 commits / 20 files)

Big change since the last pass: rather than just documenting the action_freq edit, this now migrates every physical-intelligence/libero reference → TensorAuto/libero and sets action_freq: 20 everywhere.

Resolved from prior reviews ✅

• env.fps → control_freq wiring + fps > 0 guard + docstrings: unchanged and good.
• The advantage_config.json gap I flagged is fixed (folded into the migration), and action_freq is now consistent across all libero configs.
• The 10-vs-20 tension is resolved at the source: dataset rate, action_freq, and sim control_freq all line up at 20. Conceptually this is the right fix.
• The migration is thorough — every reference (10 configs, both RECAP/datasets docs, the mapping key, the value_visualizer_app.py default, the speed-percentiles snapshot test) is updated; I grepped for stragglers and found none.

Concerns — now structural

1. 🚧 Merge-blocker: depends on an unpublished dataset. The PR states TensorAuto/libero isn't published yet, yet all example/training configs and the docs now point at it. Merging to main makes every libero config non-runnable (dataset-not-found) until it lands. Please don't merge until TensorAuto/libero is live (or land the dataset first). Note nothing in CI catches this — test_speed_percentiles skips when the dataset is absent.

2. ✂️ Scope/title mismatch — recommend splitting. The title is still "make LIBERO sim control frequency configurable," but the PR now also does a repo-wide dataset migration. The two halves have very different risk/mergeability:

   • (A) env.fps → control_freq: self-contained, tested, mergeable now.
   • (B) physical-intelligence/libero → TensorAuto/libero @ 20 fps: blocked on the external artifact, and backward-incompatible (see Fixing reward normalizer #3).

   Splitting lets (A) ship immediately while (B) tracks the dataset publication.

3. ⚠️ Backward-incompatible key rename (under-documented). standard_data_format_mapping.py renames the key instead of aliasing it, and consumers use bracket access — factory.py:137, dataset_mixture.py:147, lerobot_dataset.py:784/830/2009 — so a missing key is a hard KeyError. Any user config still pointing at physical-intelligence/libero (without supplying its own data_features_name_mapping) will crash at dataset build. The "Compatibility note" only covers the env.fps change. Fix: keep physical-intelligence/libero as an alias entry in the mapping, or call out this removal explicitly.

4. 🔎 action_freq: 20 correctness is unverifiable until the dataset lands. The 20/20/20 alignment is sound if TensorAuto/libero is a 20 fps re-export of the same frames (same frames, corrected timestamps). Worth asserting meta fps == 20 and matching frame counts once it's published, since nothing in CI exercises it.

Bottom line

The conceptual direction is good and the migration is clean and complete. But as a single PR it can't merge yet (unpublished dataset) and silently breaks existing physical-intelligence/libero configs. Recommend: split out the env.fps fix (mergeable now), gate the dataset migration on publication, and either alias the old mapping key or document its removal.

---

Generated by Claude Code

[shuheng-liu]

Thanks — addressing the re-review of the migration (incl. bddcf0b):

1. Merge-blocker (unpublished dataset) — resolved. TensorAuto/libero is now published. Verified its metadata: codebase_version=v2.1, fps=20, and the same 1693 episodes / 273465 frames as physical-intelligence/libero (a relabel, not a resample). PR body updated to drop the "not published" caveat.

2. Scope/title split — keeping it unified. The split rationale was that half (B) was blocked on the external dataset; that no longer holds now that TensorAuto/libero is live. The two halves are also cohesive — the point is aligning dataset fps + action_freq + sim control_freq at 20. Happy to split if you'd still prefer it, but there's no longer a mergeability gap.

3. Mapping-key rename / KeyError — documented, intentionally not aliased. Per maintainer direction we deliberately do not alias physical-intelligence/libero: it carries the wrong fps label, so leaving it unmapped (hard error at dataset build) is the intended way to discourage it. Added an explanatory comment at the mapping site (bddcf0b) and a "Compatibility notes" bullet in the PR body so the removal is explicit.

4. action_freq=20 correctness — verified. Since TensorAuto/libero is the same frames with corrected timestamps (fps 10→20), action_freq=20 selects consecutive stored frames 1:1 and matches the 20 Hz sim. Confirmed equal episode/frame counts above.

[shuheng-liu]

Re-review (bddcf0b, 5 commits) — prior concerns resolved ✅

All four issues from the last pass are addressed:

Prior concern	Status
🚧 Unpublished dataset (merge-blocker)	Resolved — body now states TensorAuto/libero is published: a v2.1 re-export of the same frames (1693 eps / 273465 frames, "verified equal"), fps label corrected 10 → 20, no resampling.
⚠️ Back-incompat key rename, undocumented	Resolved — now a deliberate choice with an explanatory comment in standard_data_format_mapping.py + an explicit "Compatibility notes" entry. Fail-loud to discourage the mislabeled upstream is a reasonable call.
🔎 action_freq: 20 unverifiable	Resolved — metadata verified (fps=20, v2.1, matching counts). Since it's a relabel-only re-export, the same physical frames are sampled as before (consecutive frames): training behavior is preserved for the already-correct configs and fixed for the old action_freq: 30 ones.
✂️ Scope/title	Kept as one PR. Now that the dataset is live the blocking rationale is gone, so this is just hygiene — consider broadening the title beyond (envs) since it also migrates the dataset, but not a blocker.

One caveat — I couldn't independently verify the dataset from the review sandbox. huggingface.co is egress-blocked here (403 on TensorAuto/libero and on a public control lerobot/droid_100), so I'm trusting the author's stated verification + green CI rather than confirming the HF metadata myself.

CI: check-checklist ✅, Pre-commit Hooks ✅, review ✅; CPU Tests still in progress. (PR is a draft with requested reviewers pending — hence blocked.)

Bottom line

LGTM once CPU Tests lands green. The env.fps → control_freq fix is solid and well-tested, and the dataset relabel cleanly aligns dataset rate / action_freq / sim control_freq at 20.

---

Generated by Claude Code