fix(horizon): replace mock-scale max_steps with physical max_seconds (#28)#29
fix(horizon): replace mock-scale max_steps with physical max_seconds (#28)#29Adityakk9031 wants to merge 1 commit into
Conversation
jeqcho
left a comment
There was a problem hiding this comment.
Thanks for taking #28 on. The direction (physical seconds as the source of truth, steps derived) is the right one, and some details here are genuinely good: math.ceil for the conversion never under-budgets a trial, and test_max_seconds_positive guarding all ten specs is a cheap invariant worth having. But I can't merge this version, for three reasons.
First, the new control_hz parameter never engages on the path #28 is about. Every registered @task factory still calls make_task(SPEC_BY_KEY[...]) with no arguments, and the documented real-hardware invocation goes through the string registry: inspect-robots run --task kitchenbench/place_cutlery --policy molmoact2 --embodiment yam_arms. That resolves the zero-arg factory, so effective_hz is always the 10.0 default and eval() uses the resulting max_steps verbatim. Concretely: on a 15 Hz arm, place_cutlery gets ceil(60.0 * 10.0) = 600 steps, which is 40 seconds of wall clock, not the intended 60. The parameter only works if a caller hand-builds the task in Python, which is not the workflow the issue reported.
Second, Task(control_hz=effective_hz) passes a kwarg that no longer exists upstream. Task.control_hz was removed in inspect-robots 0.18.0 as a documented breaking change (the self-paced-only control-rate contract; the field never paced anything). CI is green only because this repo's uv.lock still pins 0.6.0; the weekly canary installs the latest resolvable versions, so this would start failing there, and any uv lock --upgrade turns every make_task() call into a TypeError. The kwarg can simply be dropped since nothing downstream reads it.
Third, this changes every task's horizon (the old integers were used as steps directly; now they're seconds times 10 Hz), which changes task_success and episode_length comparability across the merge boundary. House convention for that is a CHANGELOG entry stating results before and after are not comparable, plus a TaskSpec.version bump on every affected task, and this diff has neither. Related: the max_seconds values are the old mock-scale integers with .0 appended. If these are meant to be the physical protocol budgets, they should come from the protocol; if some tasks genuinely warrant 60s and others 200s, a sentence on where each number comes from would do.
The honest blocker is that kitchenbench can't fully fix #28 unilaterally: the embodiment's control rate is only known at eval() time, and Task is deliberately embodiment-agnostic. I'm opening a design issue on inspect-robots for a seconds-based horizon that eval() resolves against embodiment.info.control_hz. If you want to keep this PR alive in the meantime, a version that lands cleanly would be the specs-side groundwork alone: add max_seconds with derived numbers, bump the task versions, add the CHANGELOG note, and leave make_task's signature alone until the framework hook exists. Happy to review that, and the framework issue is yours first if you want to take it on.
|
Design issue is up: robocurve/inspect-robots#160. As said, it's yours first if you want it. |
|
@jeqcho check this |
yes i will open the pr now |
Description
Fixes #28.
Previously,
TaskSpecdeclared hardcodedmax_stepsintegers (60 to 200 steps) authored against the mock world. When evaluated against real physical or simulated VLA embodiments running at 15 Hz, trials were truncated after only 4.0 to 13.3 seconds—far below the physical protocol budget of 120 seconds.This PR transitions
TaskSpecto physicalmax_secondsbudgets, and updatesmake_taskto computemax_stepsdynamically based on the target embodiment's control frequency (control_hz).Key Changes
TaskSpecRefactor (src/kitchenbench/specs.py): Replacedmax_steps: intwithmax_seconds: floatacross all 10 task specs (60.0,80.0,100.0,120.0,200.0seconds).src/kitchenbench/tasks.py): Updatedmake_task(spec, *, control_hz=..., max_steps=...)to calculatemax_steps = ceil(spec.max_seconds * control_hz)(defaulting to10.0Hz for the mock world).tests/test_specs.py,tests/test_tasks.py): Added assertions formax_secondsand test coverage for 15 Hz control frequency scaling and explicitmax_stepsoverrides.Verification
pytest --cov: 320/320 passed with 100.00% test coverage.mypy src/kitchenbench: Clean, 0 errors.ruff check .&ruff format --check .: Clean.