Repository navigation
fix(ci): make the default grad_norm gate a 4x sanity band - #4094
Conversation
The two P2P MTP tests (#3985) landed after small-batch GRPO tests were moved to shadow grad_norm, so they still enforce it. They log train/grad_norm as exactly 0 on steps where every GRPO group's rewards are uniform: in their stored history, GLM-4.7-Flash step 0 is 0.0 in one run and 0.164 in the other, and Qwen3.5-35B-A3B step 1 is 0.148 and 0.0. The default band cannot hold both, so the step would fail at random once ten baselines accumulate. Keep the declarations as shadow checks, like their sibling tests.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
train/grad_norm jitters between runs and is exactly 0 on steps where every GRPO group's rewards are uniform, so the old default band (0.5x up, 0.8x down, 0.1 floor) failed healthy runs. Twenty tests had opted out with enforce=False, but a failing shadow check still untrusts the run: 480 of their 490 untrusted stored runs carry such a failure, so their baselines for every metric starved. The P2P MTP tests (#3985) still enforced it. The gate only needs to catch an order-of-magnitude jump. The default now allows up to 10x the trusted mean (1.0 when that mean is near zero) and any value down to 0, and every grad_norm declaration is enforced again, which also reverts the previous commit's per-test opt-out. Replaying all stored grad_norm history with every run as a baseline, the old band fails 262 of 3347 comparisons with at least ten baselines and the new band fails none.
guapisolo
changed the title
The default grad_norm band passes a value up to 10x the trusted mean or the mean plus 1.0, whichever is higher, so anything up to 1.0 passes. The comment and docs said "1.0 when the mean is near zero"; spell out the rule and why it is wide: exact-zero steps also drag the averaged baseline down. The test now covers a small mean, a mean above 1.0, and an all-zero history, and fails under a 2x band as well as under the old one.
A 4x cap still passes every stored grad_norm value. Replaying the nightly history in time order with trust re-derived per run, grad_norm fails 0 times and its largest value reaches 0.80 of the limit (0.977 against a 0.22 mean, under the mean + 1.0 floor); the largest jump above 1.0, a kimi_k3 level shift from 1.55 to 2.84, uses 46% of it. A 4x jump in a test whose grad_norm sits above 1.0 now fails. The test boundaries move with the cap and fail under the old band, a 2x band and a 10x band.
|
@claude review always |
A failing check keeps its run out of trusted history even with enforce=False, which is how the twenty grad_norm overrides starved their tests' baselines. Say so where enforce=False is described, and point zero-flip coordinates at a wider band instead, as the grad_norm default now does.
guapisolo
changed the title
There was a problem hiding this comment.
Looks good — the widened train/grad_norm sanity band is correctly implemented and applied consistently.
What was reviewed: the GATE_DEFAULTS["train/grad_norm"] band math (hi = ref + max(3·ref, 1.0), lo = ref − max(1.0·ref, 0) = 0) checked against evaluate_constraint and the new test_grad_norm_default_is_a_sanity_band cases — all pass/fail boundaries match; confirmed every register_ci_gate(metric_key="train/grad_norm", ...) call across the repo (not just the 20 files in this diff) now uses the bare form, with no stray leftover enforce=False/allowlist_reason override; and checked the doc/comment updates in 03-metric-history-gate.md and replay.py for consistency with the new "widen the band" guidance.
Extended reasoning...
The change touches CI metric-gate config (tests/ci/metric_history/register.py, replay.py, docs) plus 20 mechanically-identical e2e test files removing a per-test override; it has no auth/injection/data-exposure surface. This is a second look at the PR — four commits landed after my prior (COMMENTED) review, refining the band and docs — and independent math verification plus the new unit test and consistency check across the whole repo found no issues, with no outstanding third-party objections and no CODEOWNERS covering these paths.
Summary
Make the default grad_norm gate a 4x sanity band and enforce it on every test again.
Symptom & Reproduction
train/grad_normband fails healthy runs. Replaying stored history with every run as a baseline, 262 of 3347 comparisons with ten or more baselines fail.enforce=False, yet a failing shadow check still untrusts the run; 480 of their 490 untrusted runs carry one, starving every metric's baseline.evaluate_coordinatewith ten baselines at 0.164 and a current 0.0 returns FAIL (outside [0.0328, 0.264]).Root Cause
train/grad_normis exactly 0 on steps where every GRPO group's rewards are uniform.train/grad_normalso jitters between runs; nonzero values span 0.03 to 4.1.GATE_DEFAULTS["train/grad_norm"]band (0.5x up, 0.8x down) cannot hold that spread.Fix
The gate is only a sanity check, and the averaged baseline is itself pulled down by zero steps, so the default band is now
rel_up3.0,abs_floor_up1.0,rel_down1.0: a value passes up to 4x the trusted mean or the mean plus 1.0, whichever is higher (anything up to 1.0 passes), and down to 0. The 20 per-testenforce=Falseoverrides go away, so every grad_norm declaration is enforced; the P2P tests need no change. Other metrics keep their bands. History is keyed by metric and step, so stored grad_norm values stay baselines. The metric-history doc stops recommendingenforce=Falsefor zero-flip coordinates, since a failing check still untrusts the run.Verification
test_grad_norm_default_is_a_sanity_band: means 0.2 / 2.0 / 0 accept 1.1 / 7.9 / 0.9, reject 1.3 / 8.1 / 1.1, and always accept 0; it fails under the old band, a 2x band or a 10x band.tests/ci/test: 1347 passed; pre-commit passed.Review Focus
GATE_DEFAULTS["train/grad_norm"]: 4x up or mean + 1.0, no lower bound above 0.