refactor(pi07): rename low_level_planner -> low_level (it's not a planner)#250
Conversation
…nner) The low-level component of π0.7 is a flow-matching action generator with FAST discrete action prediction, not a planner. The high-level component IS a planner (predicts subtask strings autoregressively) and keeps its naming. This PR aligns the source-side naming with that distinction. Closes #247. Changes: - Move src/opentau/policies/pi07/low_level_planner/ -> low_level/. - Rename classes: - PI07LowLevelPlannerConfig -> PI07LowLevelConfig - PI07LowLevelPlannerPolicy -> PI07LowLevelPolicy - PI07LowLevelPlannerFlowMatching -> PI07LowLevelFlowMatching - Update factory.py, policies/__init__.py, gemma3_with_expert.py, high_level_planner docstring, and the test suite (test_pi07_cpu.py, test_pi07_attention_layout.py, test_pi07_low_level.py, test_pi07_video_encoder_cpu.py, test_available.py). - The TestPI07LowLevelPlannerAttentionLayout class in test_pi07_attention_layout.py drops "Planner" too. Backward compatibility: A deprecation shim is left at the old low_level_planner/ path that re-exports the new classes under the old names with a DeprecationWarning at package-import time. This keeps existing user code, pickled checkpoints (config classes), and any draccus configs that pinned the old import path working through one release cycle. The shim is exercised by tests/policies/test_pi07_low_level_shim.py: verifies the warning fires, and that PI07LowLevelPlanner* names are the same class objects as the new PI07LowLevel* names. The policy-type strings in factory.py (`pi07_low_level`, `pi07_high_level`) are unchanged, so draccus `policy.type` configs keep working without edits. Out of scope: - The legacy pi07_paligemma/low_level_planner/ package and its PI07lowlevelPlannerConfig / PI07LowLevelPlannerPolicy classes are intentionally untouched. That is a separate retired package. - High-level naming is preserved everywhere. Verification: - pre-commit run --all-files: clean - pytest -m "not gpu" -n auto tests/policies/test_pi07_attention_layout.py tests/policies/test_pi07_cpu.py tests/policies/test_pi07_video_encoder_cpu.py tests/policies/test_pi07_low_level_shim.py tests/test_available.py: 62 passed in 54s. - Full CPU suite (matching cpu_test.yml): 903 passed, 13 skipped, plus 2 pre-existing timing flakes in tests/utils/test_utils_benchmark.py (timing tolerance on a 50ms sleep — unrelated to this PR).
There was a problem hiding this comment.
Doc nit, scoped to the file the PR is renaming: this line still says "This planner uses SpaceTimeSiglip" — given the PR's whole point is that the low-level component is not a planner, and the class docstring just below (line 41) was correctly updated to "low-level component", this preceding module-docstring sentence was missed. (The class docstring at line 41-47 is fine; only the module-docstring sentence that introduces the file is stale.)
|
[claude-review] summary for commit 1ce5b87 No blocking issues found.
Verification notes:
|
|
@claude fix per review. |
- addresses @claude (stale "planner" wording in module docstring): updated src/opentau/policies/pi07/low_level/configuration_pi07_low_level.py:19 to say "This component uses..." instead of "This planner uses..." so the module docstring matches the class docstring and the rename's intent. tests: skipped — single-word change inside a module docstring; no runtime impact. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
We don't even want this. Remove it outright.
@claude fix accordingly. |
- addresses @shuheng-liu (drop the deprecation shim): removed src/opentau/policies/pi07/low_level_planner/ entirely along with tests/policies/test_pi07_low_level_shim.py. The PR is now a pure rename with no back-compat aliases. tests: passed — pytest -m "not gpu" -n auto tests/policies/test_pi07_attention_layout.py tests/policies/test_pi07_cpu.py tests/policies/test_pi07_video_encoder_cpu.py tests/policies/test_pi07_low_level.py tests/test_available.py Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
[claude-fix] Done in f3684781e6c40b3c4b3502b4cabc1a64fec88e85. Removed the
Verified no remaining The PR description's "Backward compatibility (deprecation shim)" section is now stale — happy to update it in the same branch if you'd like, just say the word. tests: passed — |
- addresses @shuheng-liu (pi07 rename): updated CLAUDE.md module layout to reflect `pi07/low_level/` (renamed from `low_level_planner/` in #250); kept `pi07_paligemma/low_level_planner/` as-is since the legacy PaliGemma variant was not renamed, and added a parenthetical noting the low-level policy is a controller, not a planner. tests: skipped — CLAUDE.md prose-only edit, no runtime impact Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The low-level component is a controller, not a planner — rename to match the canonical pi07 (#250). Renames the directory, the registered policy type/name pi07_paligemma_low_level_planner -> pi07_paligemma_low_level, and drops "Planner" from the class names (PI07PaligemmaLowLevel{Config,Policy,FlowMatching}). Also addresses PR review parity nits: - Missing-camera subgoal placeholders fill -1 (black at the [-1,1] SigLIP range), matching pi07; the no-subgoal-keys fabrication stays 0 to remain byte-identical to the per-sample-padded path. - Re-add prepare_videos' 4-D-input guard (raise when n_obs_steps > 1) so a 4-D video can't silently broadcast-replicate against the new pixel-zeroing.
What this does
Closes #247.
The low-level component of π0.7 is a flow-matching action generator with FAST discrete-token prediction — not a planner. The high-level component IS a planner (predicts subtask strings autoregressively) and keeps its naming. This PR aligns the source-side naming with that distinction.
Renames
src/opentau/policies/pi07/low_level_planner/→src/opentau/policies/pi07/low_level/PI07LowLevelPlannerConfig→PI07LowLevelConfigPI07LowLevelPlannerPolicy→PI07LowLevelPolicyPI07LowLevelPlannerFlowMatching→PI07LowLevelFlowMatchingTestPI07LowLevelPlannerAttentionLayout→TestPI07LowLevelAttentionLayout(intests/policies/test_pi07_attention_layout.py)Updated callers:
factory.py,policies/__init__.py,gemma3_with_expert.py, plus the test suite (test_pi07_cpu.py,test_pi07_attention_layout.py,test_pi07_low_level.py,test_pi07_video_encoder_cpu.py,test_available.py). Docstrings that mentioned "low-level planner" were updated to "low-level component" for consistency.Backward compatibility (deprecation shim)
The old
low_level_planner/path is preserved as a thin re-export shim that aliases the new classes under their old names and emits aDeprecationWarningat package-import time. This keeps existing user code, pickled checkpoints (config classes), and any draccus configs that pinned the old import path working through one release cycle.The shim is exercised by
tests/policies/test_pi07_low_level_shim.py:PI07LowLevelPlanner*names are the same class objects as the newPI07LowLevel*names (soisinstancechecks and pickled state survive).make_att_2d_masks,pad_discrete_tokens,resize_with_pad, and the SpaceTime SigLIP encoder helpers still resolve.Policy-type strings in
factory.py(pi07_low_level,pi07_high_level) are unchanged, so draccuspolicy.type=pi07_low_levelconfigs keep working without edits.Out of scope
pi07_paligemma/low_level_planner/package and itsPI07lowlevelPlannerConfig/PI07LowLevelPlannerPolicyclasses are intentionally untouched — that's a separate retired package.Note on the diff
git mvfollowed by class-rename edits across both the moved directory and the new shim files breaks git's automatic rename detection (the new shim files atlow_level_planner/have low content similarity with the moved-and-renamed files atlow_level/). The diff therefore looks like "deletelow_level_planner/'s old content + add newlow_level/files + add small shim files atlow_level_planner/" instead of one clean rename. History is best traced via the new path going forward.How it was tested
pre-commit run --all-files— clean.pytest -m "not gpu" -n auto \ tests/policies/test_pi07_attention_layout.py \ tests/policies/test_pi07_cpu.py \ tests/policies/test_pi07_video_encoder_cpu.py \ tests/policies/test_pi07_low_level_shim.py \ tests/test_available.pycpu_test.yml):tests/utils/test_utils_benchmark.py(test_context_manager_usage,test_exception_handling) — both are tight timing assertions ontime.sleep(0.05)withpytest.approx(0.05, abs=0.01). They reproducibly fail at ~0.06s under typical macOS scheduling load. Unrelated to this PR.This is a pure rename with class identity preserved by aliases (verified by the shim test). No model / training-loop logic changes, so no GPU-specific regression risk; the nightly
gpu_test.yml+regression_test.ymlruns will catch any surprise breakage. Happy to trigger them manually before merge if preferred.How to checkout & try? (for the reviewer)
Quick smoke check on the deprecation shim:
Checklist