Skip to content

[ROCm] Fix flaky forward-tolerance in backward adagrad ROCm test - #165

Open
avbokovoy wants to merge 2 commits into
mainfrom
abokovoi/relax-fp16-tolerances-in-bwd-test
Open

[ROCm] Fix flaky forward-tolerance in backward adagrad ROCm test#165
avbokovoy wants to merge 2 commits into
mainfrom
abokovoi/relax-fp16-tolerances-in-bwd-test

Conversation

@avbokovoy

Copy link
Copy Markdown

Problem

test_backward_adagrad_rocm_fallback_kernel flakily failed with a forward output mismatch (e.g. fp32 weights + bf16 output, [127, 960]). The forward-check tolerance derived rtol/atol from weights_precision only, so fp32 weights forced the tight rtol=1e-4, tol=1e-2 even when the output was bf16 — below one bf16 ULP at the output magnitudes (~16–22). A correct kernel that bounded a single element the other way (exactly 1 ULP) tripped the assertion.

Fix

In backward_adagrad_common.py, only use the tight fp32 tolerance when both weights and output are fp32; otherwise use the relaxed 1e-2. Repro loop goes 16/40 -> 0/40 failures; the fp32/fp32 tight path is unchanged.

@avbokovoy avbokovoy self-assigned this Aug 18, 2026

@aryaman-gupta aryaman-gupta left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

aryaman-gupta added a commit that referenced this pull request Aug 28, 2026
This hits the same intermittent forward-tolerance failure already skipped for
test_backward_adagrad_rocm_fallback_kernel:

  AssertionError: Forward output mismatch: VBE=False pooling_mode=0,
                  weight_precision=fp32 output_dtype=bf16

Both go through execute_backward_adagrad, which derives rtol from
weights_precision alone (backward_adagrad_common.py:395), so fp32 weights impose
an fp32 tolerance on a bf16 output. It reproduces in roughly half of runs, which
is why one compiler passed it and the other did not.

#165 fixes the tolerance itself; this skip is a stopgap and should be
reverted with it.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@aryaman-gupta

aryaman-gupta commented Aug 28, 2026

Copy link
Copy Markdown

This also fixes test_backward_adagrad_fp32_pmSUM, test_backward_adagrad_fp32_pmMEAN and test_backward_adagrad_fp32_pmNONE, not just test_backward_adagrad_rocm_fallback_kernel.

fp32_pmSUM hit the same assertion on the MI350 runner during the CI migration work:

AssertionError: Forward output mismatch: VBE=False pooling_mode=0,
                weight_precision=fp32 output_dtype=bf16
backward_adagrad_common.py:395

All three take weights_precision=SparseType.FP32 and draw output_dtype from the shared strategy, which includes BF16 on GPU (backward_adagrad_common.py:84), so all three reach the rtol line this PR fixes. Only fp32_pmSUM has actually failed so far; the other two are equally exposed and simply have not drawn a losing combination yet. Consistent with your 16/40 flake rate — in one CI run fp32_pmSUM passed under gcc and failed under clang. The CPU variants are unaffected, since test_st_cpu restricts output_dtype to FP32/FP16.

The firefight PR needs to land first, so all three are skipped there for now:

When this merges, please remember to remove all four skips in backward_adagrad_test.pyrocm_fallback_kernel plus the three fp32_* ones above.

aryaman-gupta added a commit that referenced this pull request Aug 31, 2026
fp32_pmMEAN and fp32_pmNONE are exposed to the same forward-tolerance failure as
fp32_pmSUM. All three take weights_precision=SparseType.FP32 and draw
output_dtype from the shared strategy, which includes BF16 on GPU
(backward_adagrad_common.py:84), and all three reach the same assertion at
backward_adagrad_common.py:395 where rtol is derived from weights_precision
alone.

Only fp32_pmSUM had actually failed, but the failure is intermittent, so the
other two are equally exposed and simply have not drawn a losing combination
yet. The CPU variants are unaffected: test_st_cpu restricts output_dtype to
FP32 and FP16.

#165 fixes the tolerance; all three skips should be reverted with it.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@avbokovoy
avbokovoy force-pushed the abokovoi/relax-fp16-tolerances-in-bwd-test branch from 6bf6c26 to 5acd1be Compare September 2, 2026 12:19
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