Repository navigation
test(policies): compare the normalize round trip at bfloat16 precision - #561
Merged
Merged
Conversation
#559 made Normalize compute in its stats' dtype, so on the bfloat16 policies that the pi05, pi07 and pi07_paligemma low-level GPU integration tests build, the action round trip now comes back bfloat16. Their allclose(actions, round_trip, atol=1e-6) against float32 actions raised "Float did not match BFloat16", and could not hold at bfloat16 precision anyway. The three copies now share assert_action_normalization_round_trips in tests/utils.py. It pins that Normalize returns the stats' dtype and checks the round trip against a bound derived from that dtype's eps and the stats' mean. A CPU test runs it on the same fixture stats, so the gating suite covers the tolerance.
Contributor
There was a problem hiding this comment.
Reviewed c9a42f5: verified the helper's rounding-error bound (≤ u·(5|x|+3|mean|), inside the asserted 5u·(|x|+max|mean|); the shared std + eps denominator cancels exactly between Normalize and Unnormalize), the MEAN_STD/buffer_actions assumptions against all three GPU test fixtures, and the sweep claim (no other test round-trips the normalizer pair). No issues found.
Contributor
|
[claude-review] summary for commit c9a42f5 No blocking issues found. |
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 three GPU integration tests that have failed on
mainsince #559 (🐛 Bug). This PR changes tests only; the source code is unchanged.The failure. Three tests build a policy, cast it to bfloat16, and then check
torch.allclose(actions, unnormalize_outputs(normalize_targets(actions)), atol=1e-6)on float32torch.randnactions:test_pi05.py::TestPI05Integration::test_complete_pi05_pipeline_integrationtest_pi07_low_level.py::TestPI07LowLevelIntegration::test_complete_pi07_low_level_pipelinetest_pi07_paligemma_low_level.py::TestPI07PaligemmaLowLevelIntegration::test_complete_pi07_low_level_pipelinefix(serving): keep the state float32 for delta-action targets #559 made
Normalizecast a floating input to its stats' dtype first, so served inputs get training's arithmetic. The round trip therefore comes back bfloat16, andallcloseraisesRuntimeError: Float did not match BFloat16. Comparing in a common dtype is not enough either:atol=1e-6can never hold at bfloat16 precision;The fix. The three identical blocks now call one helper,
assert_action_normalization_round_trips, intests/utils.pynext toto_cuda_bf16. It asserts the two properties that hold after fix(serving): keep the state float32 for delta-action targets #559:normalize_targetsreturns the stats' dtype for a float32 input. This is fix(serving): keep the state float32 for delta-action targets #559's contract, now pinned rather than incidental.- mean,/ scale,* scaleand+ mean), each at mosteps / 2 · (|x| + |mean|), withepsread off the stats' dtype. A real inversion bug, such as a dropped or wrong mean or a wrong scale, misses by far more.Why the tests change and not the code. fix(serving): keep the state float32 for delta-action targets #559's CHANGELOG entry makes
Normalizecomputing in its stats' dtype intended behavior, and fix(serving): keep the state float32 for delta-action targets #559 leftUnnormalizeuntouched. What went stale is these tests' expectation of a float32-exact round trip on a bfloat16 policy.The gate now covers the tolerance. These GPU tests run only nightly. So a new CPU test,
test_action_normalization_round_trip_checkintests/policies/test_inference_state_precision.pynext to fix(serving): keep the state float32 for delta-action targets #559's ownNormalizedtype test, runs the helper on the same fixture stats in bfloat16 and float32 over 40 seeded draws. It also checks that the helper rejects an inverse whose mean is off by 1/16 (bfloat16) or 1e-3 (float32).No CHANGELOG entry, since CLAUDE.md exempts test-only PRs.
Sweep. On
main,grep -rn "normalize_targets(\|unnormalize_outputs(" tests/finds exactly these three round trips.test_policies.py::test_backward_compatibilityhas only a skip-marked case, and no other GPU test round-trips through the normalizers.How it was tested
main@ a048e55, all three tests fail at their round-trip line withRuntimeError: Float did not match BFloat16.pytest -m "gpu" -n 0 tests/policies/test_pi05.py tests/policies/test_pi07_low_level.py tests/policies/test_pi07_paligemma_low_level.pygives 11 passed and 0 failed. This includes the assertions after the round trip in the pi05 test (pad masks, position ids, attention masks), which had not run onmainsince fix(serving): keep the state float32 for delta-action targets #559.test_action_normalization_round_trip_checkpasses for bfloat16 and float32.cpu_test.ymlruns it: 3739 passed, 14 skipped. Pre-commit is clean.How to checkout & try? (for the reviewer)
pytest -m "gpu" -n 0 tests/policies/test_pi05.py tests/policies/test_pi07_low_level.py tests/policies/test_pi07_paligemma_low_level.pyChecklist
Note: Before submitting this PR, please read the contributor guideline.