fix(policies/wbc): allow_missing_models is checked, not read by truthiness - #3574
cagataycali merged 2 commits into
Conversation
…iness WBCPolicy.__init__ checked `walk` with boolean_flag_error and read `allow_missing_models` beside it by truthiness. Every non-empty string is truthy, so "false" selected the test seam: no ONNX session was loaded, construction succeeded, and the missing checkpoint surfaced at the first get_actions as a refusal advising allow_missing_models=False - the value the caller believed they had passed. None, 0 and [] took the loading branch while spelling neither posture. The check sits on the base class only, ahead of the load it gates. WBCGaitPolicy forwards the flag to super().__init__, so one site covers both providers.
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Applies the AGENTS.md posture-flag rule ("Posture flags are checked, never read by truthiness") to the second flag in WBCPolicy.__init__: allow_missing_models is now refused at construction via the shared boolean_flag_error domain, placed before the ONNX load it gates, exactly as walk already is in the same signature. The registry exposes allow_missing_models as a config_keys entry for both wbc and wbc_gait, so the JSON-string spelling the fix targets is a reachable input, and WBCGaitPolicy forwards the flag to super().__init__ so the one check site covers both providers. The only remaining truthiness read of the flag (if not allow_missing_models at the load site) now sees a guaranteed boolean. Independently verified at 777ad866: the new 21-cell test passes on the head and reports the claimed 16 failed / 5 passed when run against pre-fix production at the merge base fb159ba; the pre-existing wbc suite plus the shared posture grader (869 tests) is green; the xref/Raises/Args/ASCII/changelog graders (638 tests) are green; ruff check and ruff format --check are clean on the touched files. All in-tree callers pass real booleans, so nothing regresses; the only behaviour change is an explicit ValueError on values that previously selected a posture the caller did not spell, with the Raises: block updated to match.
What's good
- Refused set is derived from
boolean_flag_erroritself rather than copied, so the test tracks the shared domain without edits. - Over-reach controls pin both boolean postures (
True/np.bool_(True)still skip the load;False/np.bool_(False)still reach it and fail loudly). - Verbatim-wording cell keeps this flag and
walkon one refusal owner, and the ordering cell pins the check ahead of the loader. - Changelog fragment named with the PR number and pushed while still a draft; docs updated beside the parameter block that shows the flag.
What
WBCPolicy.__init__refuses a non-booleanallow_missing_modelsat construction withboolean_flag_error- the domain the same signature already applies towalk- placed before the ONNX load the flag gates.Why
Two posture flags in one signature:
walkwas checked andallow_missing_modelsbeside it was read byif not allow_missing_models. Every non-empty string is truthy, so the spelling a JSONpolicy_configreaches for to ask for the eager load selected the test seam instead.Measured on
fb159ba:allow_missing_models"false","False","no","off","0","none"1.5None,0,[]RuntimeError, onnxruntime absent)The
"false"caller then reads, at the firstget_actions:which advises the value they believe they passed. That is the refusal-inherits-the-inversion shape the posture-flag rule in AGENTS.md names: the branch is reachable only where its advice is not actionable.
WBCGaitPolicyforwards the flag tosuper().__init__, so it had the same reading and the one check covers both providers.Four corners
tests/policies/wbc/test_allow_missing_models_is_checked_not_read_by_truthiness.py, 21 cells. The refused set is derived fromboolean_flag_erroritself rather than copied.The 5 that pass on both trees are the over-reach controls:
True/np.bool_(True)still skip the load,False/np.bool_(False)still reach it. The 16 are the 13 non-boolean values, the verbatim-wording cell, the ordering cell (a falsy0is refused by name rather than reaching the loader), and the gait-variant cell.Not a row in
tests/policies/test_posture_flags_are_checked_not_coerced.pyon purpose: its table asserts the value is stored, and this flag is consumed inline - storing it would add an attribute with no reader.Gate
ruff check+ruff format --checkon touched filesmypyonpolicy.py+ the new testtests/policies/wbc/+ shared posture graderGr00tPolicyrows needingpyzmq, byte-identical onmaintests/test_changelog_fragments.py--continue-on-collection-errors)mainfb159ba, every one an absent extra (lerobot,awsiot,torch,serial,psutil,mujoco); 0 namewbcOverlap
check_merge_base_overlap.py --pathsnames #3343 onpolicy.py. Its hunk is the_extract_statedocstring at 601-618; this one is__init__at 230-240 and the class docstring. Disjoint, merge-order only.Cost
Production:
+2executable statements, a comment, two docstring sentences. Tests:+1file (21 cells). Docs:+7indocs/policies/wbc.mdbeside the parameter block that shows the flag. Fragment:changelog.d/3574-wbc-allow-missing-models-checked.md, pushed while still a draft so no approval can be dismissed by it.Auto-merge (squash) armed.
Round log