Repository navigation
fix(policies): log an out-of-range discrete state once per process - #560
Merged
Merged
Conversation
prepare_discrete_state clamps the normalized state into [-1, 1] before binning it into prompt tokens, and warned on every call that had a value outside that range. Under QUANTILE / MIN_MAX normalization a real robot's state routinely overshoots a little, so a serving process logged the line for nearly every request, and on a nearly full disk the synchronous writes stalled requests for seconds. All five copies (pi05, pi06, both high-level planners, value) now go through one helper, warn_once_if_state_out_of_range in policies/utils.py: one WARNING per process per policy, then an INFO summary after the 10th, 100th, 1000th, ... out-of-range call. The clamped bins are unchanged. Four of the docstrings claimed a ValueError the code never raised; all five now describe the clamp.
Contributor
There was a problem hiding this comment.
Reviewed commit f96689d. No blocking issues found — details in the [claude-review] summary comment.
Verified locally: all 13 new tests in tests/policies/test_discrete_state_out_of_range.py pass, and the full tests/policies CPU subset passes (1737 passed, 2 skipped). The five-copy sweep is complete (grep -rn "def prepare_discrete_state" src/ returns exactly the five routed copies plus the non-logging onnx_inference.py helper), all five policy name keys are distinct, and the clamp/binning lines are untouched.
Contributor
|
[claude-review] summary for commit f96689d No blocking issues found. Verification performed on this run:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Fixes a log flood in every discrete-state policy (🐛 Bug). Only the logging changes: the state tokens stay bit-identical.
prepare_discrete_stateclamps the normalized state into [-1, 1], then bins it into 256 prompt tokens. It loggedState values are not normalized between -1 and 1whenever any value fell outside that range. Under QUANTILE / MIN_MAX normalization a real robot's state routinely overshoots a little, because q01/q99 and the dataset min/max are not hard bounds. So the gRPC server logged the warning on nearly every request. In a real deployment this flooded the log, and on a nearly full disk the synchronous writes stalled individual requests for seconds.warn_once_if_state_out_of_range(state, policy_name)insrc/opentau/policies/utils.py.name).@torch.compiler.disabled. If traced, its data-dependent branch and.item()calls would split a compiledsample_actionsinto several small graphs (measured: 5 graphs and 3 breaks, against 1 graph and 1 break with the decorator).state.to(...).numpy()is split into two statements so the helper receives the tensor.Raises: ValueError: If the state values are not normalized between -1 and 1., but every copy only warned and clamped. All five now say that out-of-range values are clamped into [-1, 1] (bin 0 or 255) and logged once per process.### Fixedentry appended to[Unreleased]. The change is logging-only, but it changes what a serving process logs and removes the stalls. CLAUDE.md exempts only docs-only, test-only andchore(claude)PRs from an entry.Sweep and inclusion criterion. The copies were enumerated mechanically before scoping.
grep -rn "State values are not normalized" src/andgrep -rn "def prepare_discrete_state" src/both return exactly these five.prepare_discrete_statethat logs about the out-of-range state it clamps. All five warned and clamped, and none raised, so each one keeps that behavior.scripts/onnx_inference.py::_prepare_discrete_state. It clamps without logging, so there is nothing to rate-limit.How it was tested
tests/policies/test_discrete_state_out_of_range.py(13 CPU tests). The tests call each copy's real, unboundprepare_discrete_statewith a stubself. The method reads onlyname, plusconfig.state_typein pi05, so no model is built. The tests check that:caplog), and every call, warned or silent, returns the hand-computed clamped bins;def prepare_discrete_stateis in the test's list and calls the helper, and none callslogging.*directly. A sixth copy therefore can't silently bring back the per-call warning.main's and this branch'sprepare_discrete_stateran for all five copies on ~1.9M values: uniform and normal float32, bfloat16, every bin edge ±1 ulp, ±1e30 and ±inf. The outputs are identical.configs/dev/ci_config.json(pi05, discrete state) for 3 steps on CPU with the same seed. The 6 per-micro-batch losses (exact float hex of loss, MSE and CE) were bit-identical across two runs of this branch and one run of the base commit (main@ a048e55). This config min-max-normalizes the state with the dataset's own min/max, so the state never leaves [-1, 1] and the warning path stays idle. A second scenario switched the state to MEAN_STD in a scratch copy of the config, so the state does leave [-1, 1]. The losses were again bit-identical across two branch runs and the base run. The base logged the warning on 5 of the 6 micro-batches; this branch logged it once.cpu_test.ymlruns it: 3749 passed, 14 skipped, 1 failed. The failure wastests/utils/test_utils_benchmark.py::test_context_manager_usage, a timing assertion wheresleep(0.05)measured 60 ms under full-core load. It passes when run on its own. Pre-commit is clean.pytest -m "gpu" -n 0over the five affected policies' test files on a GPU dev box. On commit f96689d: 5 passed, 1 skipped, 1 failed.test_pi05.py::TestPI05Integration::test_complete_pi05_pipeline_integrationfails withRuntimeError: Float did not match BFloat16in its Normalize→Unnormalize round-trip assertion. It fails the same way onmain@ a048e55 without this change: fix(serving): keep the state float32 for delta-action targets #559's stats-dtypeNormalizemet a stale float32 round-trip assertion. Itspolicy.forward, which runs through the new helper and logs the warning once, passes before that assertion. With test(policies): compare the normalize round trip at bfloat16 precision #561's test fix applied on top of this branch, the same targeted run gives 6 passed, 1 skipped and 0 failed.@pytest.mark.skipped. That copy is covered by the CPU tests above.regression_test.yml).How to checkout & try? (for the reviewer)
Checklist
Note: Before submitting this PR, please read the contributor guideline.