Skip to content

Reject wrongly shaped params in ParameterizedOption.ground - #205

Open
yichao-liang wants to merge 3 commits into
masterfrom
ground-params-shape-check
Open

yichao-liang wants to merge 3 commits into
masterfrom
ground-params-shape-check

Conversation

@yichao-liang

@yichao-liang yichao-liang commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator

ParameterizedOption.ground now raises a clear ValueError when the params' shape differs from the params space's shape, for example Cannot ground 'PickJug': params [] have shape (0,), expected shape (1,).

Why. When params_space.contains(params) failed, ground fell back to a float-tolerance bounds check (added in #57) that never looked at the shape.
Empty params for a one-parameter space passed it vacuously (np.all over no values is True), so the option was grounded with no params and failed later.
This is how Grow's oracle crash got through grounding before #201 fixed its samplers: the NSRT's null_sampler gave the skill-factory PickJug no params, and pick.py's _descend_pose raised an IndexError reading params[0].
For a space with more than one entry, the bounds comparison raised a bare broadcast error instead.

The sampler path had the same hole. _GroundNSRT.sample_option and _GroundEndogenousProcess.sample_option clip sampled params with np.clip(params, low, high) before grounding, and np.clip broadcasts.
It turned a (1,) sample into (0,) for a parameterless option and copied one value into all four params of a (4,) space.
For empty params and a (4,) space it raised the operands could not be broadcast together with shapes (0,) (4,) (4,) error seen with Coffee's PlaceJugInMachine, before ground was even called.
Both now clip only correctly shaped params (_clip_sampled_params) and pass anything else to ground, which rejects it.
default_params substitution for empty params is unchanged, so Wait options still ground from [].

Callers that relied on the old behavior, found by the full suite and by a sampler-shape audit over every ground-truth NSRT and endogenous process (default config and each env's legacy options), now fixed:

  • utils.ops_and_specs_to_dummy_nsrts gave every NSRT the dummy sampler np.zeros(1) and relied on the clip to reshape it; test_llm_open_loop_approach grounded its parameterless PDDL options through it.
    Its samplers now return zeros of each option's params shape, which the clip turns into the same values as before.
  • Grow, Boil and Coffee pick processes returned a grasp offset [x] even with legacy options, whose PickJug takes no params; the clip used to drop it silently.
    They now return [] with legacy options, as their place and push samplers already do.
  • Coffee's TwistJug process used null_sampler although TwistJug takes a twist amount when coffee_twist_sampler is on (the default); it passed only through the vacuous check.
    It now draws the amount as the TwistJug NSRT's sampler does.

Left as is. On top of #201, which fixed the Grow and Coffee NSRT samplers, the audit finds only legacy Grow's Place (with grow_use_skill_factories off) still sampling the wrong shape.
Its PlaceJugOnTable process gives [] to the two-parameter Place, and with grow_place_option_no_sampler on, its PlaceJug NSRT gives [x, y] to the parameterless one.
Both already raised broadcast errors on master and now raise the clear error; legacy Grow does not plan on master in either mode.
LinearChainParameterizedOption's consistency check has the same hole (np.allclose on (0,) and (1,) bounds passes), and Coffee's combined Twist chain relies on it, so it is left for a separate change.

Test isolation fix. pytest tests/test_structs.py on its own errored in 5 tests, and tests/agent_sdk/test_bilevel_sketch_samplers.py on its own failed test_refine_and_validate_report_returns_plan.
State.pretty_str() reads CFG.excluded_objects_in_state_str, an args.py setting that exists only after a reset_config() call, so these tests passed only when an earlier test in the process had called it.
A session-scoped autouse fixture in tests/conftest.py now calls reset_config() once before the first test.

Test plan

  • New test_option_ground_rejects_wrong_params_shape (the Box(0, 0.1, (1,)) option grounded with np.array([]), plus (4,), (1, 1) and scalar cases, parameterless options and default_params) failed on master and passes here.
  • New test_sample_option_rejects_wrong_params_shape covers NSRTs and endogenous processes: wrong shapes raise, correct shapes are still clipped, empty params still take default_params.
  • Full suite on the final commit (rebased on master 423cae0c1), split round-robin by test file into 16 jobs in CI's Ubuntu 24.04 container: 2546 passed, 0 failed (4030 skipped, 14 xfailed, 4 xpassed).
    The first run, with only the ground check, found the order-dependent test_bilevel_sketch_samplers failure (also on master); the second found test_llm_open_loop_approach (the dummy samplers).
  • New test_process_samplers_match_option_params (Grow, Boil and Coffee, skill-factory and legacy options) fails on master in exactly the four fixed cases and passes here.
  • Sampler-shape audit over every ground-truth NSRT and process (default and legacy configs, plus Coffee's twist variants): the only mismatches left are legacy Grow's, listed above.
  • The README command (main.py --approach oracle and oracle_process_planning, seed 0, one task, 60 s) on Grow and Coffee ends as on master 423cae0c1: Grow's oracle times out in refinement and its process planning solves 1/1; Coffee fails on cup0's pose with both.
  • Before Sample the skill-factory parameters in Grow's and Coffee's NSRTs #201 (at 864ed48c7), Grow's oracle raised IndexError in _descend_pose; with this change it stopped at grounding with Cannot ground 'PickJug': params [] have shape (0,), expected shape (1,).
  • yapf 0.32.0, isort 5.10.1, docformatter 1.4, mypy 1.8.0 and pylint on a clean clone of the final commit: no diffs, no issues.

🤖 Generated with Claude Code

State.pretty_str() reads CFG.excluded_objects_in_state_str, a setting
that predicators/args.py defines and only reset_config() sets. Tests
that format a state without calling reset_config() themselves passed
only when an earlier test in the same process had called it: alone,
tests/test_structs.py errored in 5 tests and
tests/agent_sdk/test_bilevel_sketch_samplers.py failed
test_refine_and_validate_report_returns_plan. A session-scoped autouse
fixture in tests/conftest.py now calls reset_config() once before the
first test.
ops_and_specs_to_dummy_nsrts gave every NSRT the dummy sampler
np.zeros(1), whatever its option's params, and relied on the np.clip
before grounding to broadcast that to the option's shape (to no params
for the PDDL options of test_llm_open_loop_approach). Its samplers now
return zeros of each option's params shape.

With legacy options, PickJug takes no params, yet the Grow, Boil and
Coffee pick process samplers returned a grasp offset, which that clip
also broadcast away. They now return no params with legacy options, as
the place and push samplers already do.

Coffee's TwistJug process used null_sampler, although TwistJug takes a
twist amount when coffee_twist_sampler is on (the default). It grounded
only because ground()'s bounds fallback passed empty params vacuously.
It now draws the amount as the TwistJug NSRT's sampler does.

The new test checks that every endogenous process of Grow, Boil and
Coffee, with skill-factory and legacy options, samples params of its
option's shape.
When params_space.contains() failed, ground() fell back to a
float-tolerance bounds check that ignored the shape. Empty params
passed it vacuously for a one-parameter space, so Grow's oracle
grounded the skill-factory PickJug with no params and crashed later in
_descend_pose reading params[0]. ground() now raises a ValueError
naming the option, the received shape and the expected shape whenever
they differ, after the default_params substitution.

The NSRT and endogenous-process sample_option() clipped sampled params
with np.clip before grounding, which broadcasts: it dropped a value
meant for a parameterless option, copied one value into every entry of
a larger space, and raised a bare broadcast error for empty params and
a four-parameter space (Coffee's PlaceJugInMachine). They now clip only
correctly shaped params and leave the rest for ground() to reject.
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.

1 participant