Skip to content

fix(training/rl): the three RL posture flags are checked, not read by truthiness - #3548

Merged
cagataycali merged 2 commits into
strands-labs:mainfrom
cagataycali:fix/rl-posture-flags-are-checked-not-read-by-truthiness
Sep 12, 2026
Merged

cagataycali merged 2 commits into
strands-labs:mainfrom
cagataycali:fix/rl-posture-flags-are-checked-not-read-by-truthiness

Conversation

@cagataycali

Copy link
Copy Markdown
Member

What

normalize_obs, normalize_advantage and autotune_alpha on RLTrainSpec are held to boolean_flag_error - the domain TrainSpec.resume and streaming already take - through three field-scoped Trainer gates, each consulted by exactly the backends that read the flag.

Why

Each flag selects a posture: wrap both observation streams in EmpiricalNormalization or feed them raw; standardize advantages per batch or use them as computed; build a temperature optimizer or hold alpha at init_alpha. Every RL backend read the ones it consumes by truthiness (... if spec.normalize_obs else None on all three, if spec.normalize_advantage: at two PPO sites, if self.autotune_alpha: in SAC) while its validate() graded every numeric knob around them. So the spellings a caller reaches for to opt out selected the affirmative branch, and 0/None selected the negative one without being a declared spelling of it.

Measured on b4c3ea8 through create_trainer(<provider>).validate(spec):

flag backend(s) reading it "false" / "no" / "0" / 0 / None
normalize_obs ppo, fast_sac, fast_td3 [] on all three - the first three build the normalizers the caller declined
normalize_advantage ppo [] - advantages standardized against the opt-out
autotune_alpha fast_sac [] - "false" builds the temperature optimizer test_rl_fast_sac.py pins that False must not; 0 holds the temperature fixed while spelling neither

autotune_alpha also gates whether alpha_lr is read, and temperature_learning_rate_problems read it by truthiness too:

autotune_alpha alpha_lr before after
"false" -1.0 ['fast_sac: alpha_lr must be > 0, got -1.0.'] - the rate of an optimizer the caller asked not to build one problem, naming autotune_alpha
True -1.0 refused as alpha_lr unchanged
False -1.0 [] unchanged

This is the shape AGENTS.md records for lerobot_camera's async_mode / timeout_ms ("a flag that gates whether a numeric row is READ is checked ahead of the numeric guard"): the flag gate goes ahead of the rate gate in FastSacTrainer.validate, and the rate gate reads alpha_lr only once the flag is a usable True, so a misread posture is refused by its own name rather than as the knob it would have selected. The ordering is pinned both behaviourally and structurally (AST over the one module that consults both).

Where the checks live

The existing pattern: _posture_flag_problems in training/_validate.py (already the owner of resume / streaming) gains three callers, Trainer gains three thin gates beside _resume_problems / _streaming_problems, and each RL validate() consults the gates for the flags it reads - three for normalize_obs, PPO alone for normalize_advantage, SAC alone for autotune_alpha. The biconditional the numeric gates document holds here too: a backend that ignores a flag reports nothing about it (fast_td3 with normalize_advantage="false" reports []), pinned by TestABackendThatIgnoresTheFieldReportsNothing and by the one-owner scan, whose reader set is derived from the tree with the shared reads_spec_field rule so a backend that starts reading a flag is graded on arrival.

The two meta-guards that grade gates from the outside are extended rather than bypassed: FIELD_SCOPED_GATES and the guard roster in test_spec_field_read_discovery.py, and _GATE_FIELDS in test_spec_fields_are_documented.py - the latter is what failed first when the gates landed without a table entry, which is the table doing its job.

Four corners

tests production result
new pre (gates absent) collection ImportError - the gate functions do not exist
new gates defined, no backend consults them 51 failed, 72 passed
new new 123 passed
pre (tests/training/) new 174 failed / 5010 passed on main -> 174 failed / 5138 passed here, byte-identical failing node ids (all torch / lerobot / mujoco absent)

The 51 are every refusal cell across the five reader cells, the ordering cells, and the one-owner scan's every_reader_routes_through_the_gate for all three gates. The 72 that pass on both trees are controls: usable booleans untouched, defaults report nothing, ignorers report nothing, the gates carry their context.

Gate

check result
ruff check + ruff format --check on touched files clean
mypy on _validate.py, base.py, rl/base_algo.py no issues
whole-tree graders (139 in the roster) 71 failed / 4944 passed / 2 errors - byte-identical node ids on main (73 of 73), all lerobot / qpsolvers / torch-mock, 0 attributable
non-ASCII in touched Python none

Cost

Production: +3 gate functions and +2 lines in the rate gate (_validate.py), +3 thin Trainer methods, +4 validate() calls across three backends, spec docstring sentences. Tests: one new module in the shape of test_posture_flag_domain.py, plus the two table entries the meta-guards require. Docs: one paragraph in docs/training/rl.md beside the alpha_lr paragraph it qualifies.

Not touching IsaacConfig's four unchecked flags (headless, ground_plane, enable_rtx_sensors, verbose), which the same sweep found: #3343 (open) edits simulation/isaac/config.py, so that is a merge-order question for after it lands.

Opened as a draft so the changelog fragment - which needs this number - ships before any approval can be dismissed by it. Auto-merge (squash) armed once ready.

… truthiness

normalize_obs, normalize_advantage and autotune_alpha each select a posture,
and every RL backend read the ones it consumes by truthiness while its
validate() graded every numeric knob around them. So "false", "no" and "0"
selected the affirmative branch - the normalizers or the temperature optimizer
the caller had declined - and 0 or None selected the negative one without being
a declared spelling of it. autotune_alpha also gates whether alpha_lr is read,
and that gate read it by truthiness too: autotune_alpha="false", alpha_lr=-1.0
was refused as the rate of an optimizer the caller asked not to build.

Each flag now takes the shared boolean_flag_error domain through a field-scoped
Trainer gate, consulted by exactly the backends that read it: all three for
normalize_obs, PPO for normalize_advantage, FastSAC for autotune_alpha - ahead
of the alpha_lr check, which now reads the rate only once the flag is a usable
True. The one-owner scan derives the reader set from the tree, so a backend
that starts reading a flag is graded on arrival.
@cagataycali
cagataycali marked this pull request as ready for review September 12, 2026 13:40
@cagataycali
cagataycali enabled auto-merge (squash) September 12, 2026 13:40

@yinsong1986 yinsong1986 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Holds the three RL posture flags (normalize_obs, normalize_advantage, autotune_alpha) to the shared boolean_flag_error domain through three field-scoped Trainer gates, consulted by exactly the backends that read each flag - all three RL backends for normalize_obs, PPO alone for normalize_advantage, FastSAC alone for autotune_alpha. temperature_learning_rate_problems now reads alpha_lr only once autotune_alpha is a usable boolean, and FastSAC's validate consults the flag gate ahead of the rate gate it depends on, so a misread posture is refused by the flag's own name rather than as the rate it would have selected. This matches the PR description exactly and follows AGENTS.md's "Posture flags are checked, never read by truthiness" convention, including the lerobot_camera async_mode/timeout_ms shape for a flag that gates whether a numeric field is read at all.

What's good

  • Verified locally at 505f6401: the new test module (123 passed), both extended meta-guards (test_spec_field_read_discovery.py + test_spec_fields_are_documented.py, 111 passed), the full tests/training/ suite (5538 passed, 4 skipped), and the whole-tree graders most likely to bite this diff (test_docstring_xref_roles_resolve.py, test_no_host_paths.py, test_log_strings_are_ascii.py, test_test_case_names_describe_behaviour.py, 74 passed) are all green.
  • The one-owner scan derives its reader set from the tree via the shared reads_spec_field rule and roots the walk at inspect.getfile(Trainer), so a backend that starts reading a flag is graded on arrival - and the planted-defect control keeps the scanner non-vacuous.
  • The biconditional holds in both directions: ignorers report nothing (fast_td3 with normalize_advantage="false" is []), and test_no_backend_gates_a_field_it_does_not_read pins the converse.
  • The temperature_learning_rate_problems change keeps the ordering safe by construction: the only module that consults the rate gate also consults the flag gate first, pinned both behaviourally and structurally (AST over fast_sac.py).
  • Changelog fragment named with the PR number, docs paragraph placed beside the alpha_lr prose it qualifies, IsaacConfig's four flags correctly left to the merge order with #3343 rather than pulled into scope.

@cagataycali
cagataycali merged commit 7245233 into strands-labs:main Sep 12, 2026
13 checks passed
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.

2 participants