fix(embodiments): un-hardcode verify script, finish RTC horizon integer migration - #308
Conversation
…er migration - verify_embodiment_structure.sh hard-coded the expected preset list and has been failing since quadcopter shipped; validate whatever list_presets() returns instead. - emit_embodiment_presets.py and configs/embodiments/ still stored the legacy fractional rtc_execution_horizon; re-running the emitter would regress the migrated integer values in the package presets, and every from-source load emitted a deprecation warning. Sync all copies to the migrated integer counts (franka/ur5: 25, so100: 12). - docs/embodiment_schema.md: fix shipped preset count (4, quadcopter included), canonical preset location, and the rtc_execution_horizon field semantics + cross-field rules to match schema.json/validate.py (ADR 2026-04-25 decision FastCrest#8). No behavior change beyond the verify script now passing. Prep for FastCrest#69 (the Unitree Z1 arm preset will be the 5th preset).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc3c0b0915
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| presets = list_presets() | ||
| expected = ["franka", "so100", "ur5"] | ||
| if presets != expected: | ||
| print(f" ✗ list_presets() returned {presets}, expected {expected}") | ||
| # Don't hard-code the expected preset list — it already drifted once when | ||
| # quadcopter shipped, and hard-coding it here would break again on the | ||
| # next preset. Validate whatever the package actually ships. | ||
| if not presets: |
There was a problem hiding this comment.
Keep an independent inventory of required package presets
When any single bundled preset is accidentally omitted from src/tether/embodiments/presets/, list_presets() simply returns the surviving files, this nonempty check succeeds, and the loop reports that the package structure is valid. The earlier expected-list comparison caught such omissions; deriving both the subject and expected inventory from the same directory makes this preflight unable to detect incomplete packaging, including a missing quadcopter.json or one of the package copies whose dev copy was checked above.
Useful? React with 👍 / 👎.
| | `frequency_hz` | float | 0–1000 (exclusive 0) | Robot control loop rate. | | ||
| | `chunk_size` | int | 1–200 | Actions in a single inference chunk. | | ||
| | `rtc_execution_horizon` | float | 0–5.0 (exclusive 0) | Seconds of chunk to execute before requesting next inference. | | ||
| | `rtc_execution_horizon` | int | 1–`chunk_size` | Integer count of actions to lock during RTC replan. Legacy fractional values (0 < v < 1) auto-migrate to `int(v × chunk_size)` at load with a one-time deprecation warning; schema v2 will reject them. | |
There was a problem hiding this comment.
Document the migration's actual rounding behavior
For legacy fractions whose product with chunk_size is not integral, this formula gives a different result from the loader: EmbodimentConfig.from_dict() uses int(round(horizon * chunk_size)), whereas the new documentation promises truncation via int(v × chunk_size). For example, v=0.25 and chunk_size=30 is documented as 7 actions but loads as 8, so operators tuning custom legacy configs cannot predict the resulting lock horizon from this schema guide.
Useful? React with 👍 / 👎.
…t migration formula in docs Address Codex review feedback on FastCrest#308: - verify_embodiment_structure.sh validated whatever list_presets() returned, so an accidentally dropped preset file (e.g. quadcopter, which has no configs/ dev copy) passed the preflight silently. Keep a required-minimum set checked as a SUBSET of what ships: omissions fail the check, while new presets still validate automatically without editing the script. - schema.json description and docs/embodiment_schema.md documented the legacy fractional rtc_execution_horizon migration as int(v x chunk_size) (truncation), but EmbodimentConfig.from_dict() rounds: round(v x chunk_size), minimum 1. Document the actual behavior (v=0.25, chunk_size=30 migrates to 8, not 7). No behavior change; the shipped presets' migrated values are unaffected.
|
Both suggestions addressed in 7fd2768:
Verification: |
|
Hi @rylinjames, a gentle ping for review whenever you have a moment. Both Codex suggestions are addressed in 7fd2768 (omission detection restored in the verify script, migration formula corrected in the schema docs), the required Happy to address anything that comes up in review. Since this is the prep step for the Z1 arm-only preset from #69, getting it merged early keeps that work moving too. Thanks! |
What
Consistency fixes in the embodiment config layer, prepping the ground for the Unitree Z1 arm preset (#69):
scripts/verify_embodiment_structure.shno longer hard-codes the expected preset list. It has been failing since quadcopter shipped (expects['franka', 'so100', 'ur5'], butlist_presets()returns 4). It now validates whatever the package actually ships.rtc_execution_horizonfractional → integer migration (ADR 2026-04-25 decision [Orchestration] Enhance reflex doctor for Optional Dependencies #8) in the copies that were left behind:scripts/emit_embodiment_presets.pyandconfigs/embodiments/{franka,so100,ur5}.json(franka/ur5: 25, so100: 12 — matching the already-migrated in-package presets). Re-running the emitter would previously have regressed the package presets back to fractional values, and every from-source load emitted a deprecation warning.docs/embodiment_schema.md: fix shipped preset count (4, quadcopter included), the canonical preset location (in-package presets dir;configs/embodiments/copies are dev fallbacks), and thertc_execution_horizonfield semantics + cross-field rules to matchschema.json/validate.py.Why
The Z1 preset (#69) will be the 5th preset and needs these gates green: the verify script must pass with a new preset present, and the emitter must be re-runnable without regressing the integer migration.
Testing
bash scripts/verify_embodiment_structure.sh— fails onmain(hard-coded 3 presets), passes here (4/4 presets validated)pytest tests/test_embodiments.py tests/test_guard.py— 90 passedpython scripts/emit_embodiment_presets.py— 3/3 validated; re-emit is content-identical (modulo pre-existingto_dict()key ordering of the optionalgripperblock)ruff check scripts/emit_embodiment_presets.py— no new findings vsmain(I001 etc. are pre-existing)Prep for #69 — no hardware values included in this PR; the Z1 preset itself will follow once the hardware contract is confirmed there.