Repository navigation
test-backend-ops: space TOPK_MOE's logits, so the ids comparison cannot turn on a last bit (engine #1259) - #238
Conversation
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ede5de8c7.
The fix is the right one and the arithmetic holds up:
row[i] = (i - n/2) * 2^-9 exactly representable (small integer x 2^-9), range +/-0.25 for 256 experts
adjacent values differ by 2^-9 -> adjacent probabilities ~1e-3 apart, which is the 1e-3 the comment claims
So an id can only differ if a producer is genuinely wrong, rather than because two backends rounded the 6th and 7th expert to the same
float. That is the correct way to deflake an exact-index assertion: separate the values, do not loosen the comparison.
Three things I checked because they are where a fix like this usually goes wrong:
- Coverage is preserved:
test_case::initialize_tensors(ctx)runs first, so every other tensor keeps the uniform fill it had, and only
thelogitstensor is overwritten. Theif (logits == nullptr) return;guard means a case without one is untouched. - The write is the house convention:
ggml_backend_tensor_set(logits, …)is exactly whatinit_tensor_uniformitself does (it is how
every other case sets tensor data at this point in the lifecycle), so this does not depend on the tensor being host-resident when it is
not.<random>and<algorithm>were already included. - Determinism:
std::mt19937 rng(0x1259u + n)makes the permutation a function ofnalone, andstd::shuffle's result is
implementation-defined — which is fine here, because both backends read the same tensor within a run; the test does not need a specific
permutation, only well-separated values.
One residual, not a defect: the seed depends only on n, so two cases with the same width get the same permutation. Harmless today (nothing
depends on the rows being decorrelated between cases), but if a later case ever relies on that, the seed should take the op and type too.
Also worth keeping the (engine #1259) attribution in the comment: it is the row that flaked at a3e8d42c9, and someone reading a future
failure needs to know the flake was already diagnosed as a tie-break rather than start looking for a real index bug.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ede5de8c790a58cc318c52cf552d3a742e5d312f
I built test-backend-ops at this head and ran the op it changes, three times: 320/320 TOPK_MOE tests passed each run, 1/1 backends, on the CPU backend.
The fix is the right kind for a flaky test: it removes the coin, not the comparison. The row asserts an exact index set, and with random logits the 6th and 7th expert values can be within a few ULPs (~0.1 % per case with 256 experts over 6 routes, ~33 % across a full-suite run) — so two producers that compute those probabilities in different places can legitimately disagree on the order while the gathered weight moves by ~1e-7, inside the tolerance. Spacing the logits at exactly representable values 2^-9 apart makes adjacent probabilities differ by ~1e-3, i.e. three orders of magnitude above the ULP noise, so an id can now only differ if the producer is genuinely wrong. That is strictly better than loosening the tolerance: the assertion keeps its strength and loses only its dependence on rounding.
Two details show it was written by someone who has been bitten by this:
- it calls
test_case::initialize_tensors(ctx)first, "so every other tensor keeps the coverage it had" — the override adds spacing for the logits instead of replacing the base initialisation, which is exactly how a targeted fixture change goes wrong; - the justification is quantitative and dated (the
a3e8d42c9pin, engine ggml-org#1259's flake), so the next person does not "simplify" the spacing back to random floats.
Not run: the HRX backend arm, which is where the two producers actually diverge (this box's build226 is GGML_HRX=OFF), and the full suite. What I could take — that the test compiles, that the op's 320 cases pass on CPU across three runs, and that the spacing argument is arithmetic rather than a guess — is green.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at ede5de8c790a58cc318c52cf552d3a742e5d312f
The diagnosis is right (near-ties at the k-th boundary, not exact ties), and spacing the logits is the right kind of fix: nothing skipped, waived or widened. But it only covers half the cases, the bias_probs=0 ones.
Why: with bias_probs=1, build_graph selects on selection_probs = ggml_add(probs, exp_probs_b) (ggml_argsort_top_k(ctx, selection_probs, …)). exp_probs_b is filled by test_case::initialize_tensors from the base class's uniform [-1, 1] fill, and this PR deliberately keeps that fill for every tensor except logits. After spacing, adjacent logits are 2^-9 apart, so the spaced probs differ by about 1e-3 relative, but in absolute terms that's tiny next to a uniform bias. The ordering of selection_probs is therefore set by random bias values again, which brings back exactly the near-tie window the PR closes.
Measured on this test's own shapes, simulating the k-th-boundary gap (n=256, k=6, sqrt_softplus, relative gap ≤ 4e-7, numpy float32, 200,000 draws per arm):
| fill | per-case near-tie |
|---|---|
| original random logits | 0.0180 % |
| original + random bias | 0.0035 % |
| spaced logits (this PR), no bias | 0.0000 % |
| spaced logits (this PR) + random bias | 0.0095 % |
So the bias_probs=1 rows (half the matrix at every size, including [256,22]) still flake at a measurable rate, and "an id can only differ if a producer is genuinely wrong" isn't yet true for them. My baseline rate is lower than your 0.12 % (my threshold or setup probably differs from yours), but the comparison between the arms is what matters here.
The fix is the same move, applied to the bias. In the same override, give exp_probs_b (keep a pointer to it, as with logits) values on a spaced grid too, e.g. (perm(i) - n/2) * 2^-7 shuffled with a different seed. Then selection_probs = spaced bias + small spaced probs keeps an adjacent gap of at least about 2^-7 minus the probs' spread. Or make the bias grid coarse enough that the probs can't reorder it. Then rerun your duplicated-vs-spaced A/B with bias_probs=1 in the filter, the way you did for the bias_probs=0 row.
Optional: (i - n/2) / 512 shrinks the logit range from the original [-1, 1] to about [-0.25, 0.25] at n=256, so softmax probabilities are nearly uniform, which is a narrower test of the gating numerics than before. A 2^-7 step keeps n=256 at [-1, 1) and is still exactly representable. Your call.
What I ran: read the diff (one commit on 1bit/hrx-vulkan-patched); read test_topk_moe::build_graph for the bias_probs path; ran the simulation above. No build or device run.
…ot turn on a last bit (engine ggml-org#1259) `test_topk_moe` compares the expert ids it selected with the CPU reference's, as an exact index set. With random logits in [-1,1] and 256 experts over 6 routes, the 6th and 7th probabilities are within a few ULPs often enough to matter, and then the two producers legitimately disagree: the CPU gathers `probs` from one softmax and sorts it, the HRX path computes the same quantity in another place, and a last-bit difference flips their order. The ids differ, the weights differ by ~1e-7 (inside this test's nmse tolerance), so the report is "[VIEW] ids only, tiny ERR" - which is exactly what the row at pin a3e8d42 looked like: `TOPK_MOE(ne=[256,22,1,1],n_expert_used=6,with_norm=1,bias_probs=0,gating_func=3)`, `[VIEW] ERR = 2.5e-4`, ~2 full-suite runs in 9, and 12/12 clean when the case is run alone. Measured, not asserted (simulation of this test's own data, numpy float32, 4000 draws per point): exact tie at the k-th boundary, gating_func=3 0.05 % per case -> 0.16 % per full-suite run (cannot explain it) adjacent gap <= 4e-7 relative, gating_func=3 0.12 % per case -> 33 % per full-suite run (matches 2/9) adjacent gap <= 4e-7 relative, softmax 0.10 % per case -> 27 % per full-suite run So the first explanation I wrote for this row - "exact ties, and the reference's tie order is undefined because `ggml_compute_forward_argsort_f32` sorts with an unstable, value-only comparator" - is arithmetically too rare to be the cause (it stays true, it is just not what happens here), and this commit does not rest on it. The fill keeps every other tensor as the base class fills it and gives the logits exactly representable values 2^-9 apart, shuffled per row so the order is not monotone. Adjacent probabilities then differ by ~1e-3, three orders above the few-ULP window, so an id can only differ if a producer is genuinely wrong - which is what the comparison is for. Nothing is skipped, waived or tolerance-widened. Refs ggml-org#1259
ede5de8 to
4240f15
Compare
|
Rebased onto the current The measurements in the body stand unchanged — this is test data only, so the base commit cannot affect it, and the filtered A/B (duplicated values → 8/16 fail on |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4240f158d — my READY at ede5de8c7 carries.
The test file is byte-identical (tests/test-backend-ops.cpp same blob at both heads), so the substantive review stands: exact 2^-9 spacing,
±0.25 range, coverage preserved by the base fill, the same ggml_backend_tensor_set convention init_tensor_uniform uses, deterministic per
n.
The only thing that moved is the base: the delta is graph-program-cache.{cpp,h} (+81/−6), which is #235's fold arriving as this branch's own
commit because the branch was cut before it merged — the same benign stale base I noted on #239. A rebase onto the current branch tip drops it
and leaves the single test file.
|
Adding the spacing invariant this fix depends on, as a check a reviewer can reproduce in seconds without a GPU (numpy, exact float32):
(Property is permutation-independent, so it does not depend on the per-row shuffle's seed.) |
|
Full-suite run with this build, for the record (queued when the PR merged): 4575/4575 passed, |
Refs engine ggml-org#1259
The problem
test_topk_moecompares the expert ids it selected with the CPU reference's as an exact index set, on random logits in[-1,1]. With 256 experts over 6 routes that comparison can turn on a last bit, and then the two producers legitimately disagree — the CPU gathersprobsfrom one softmax and sorts it, the HRX path computes the same quantity elsewhere, and a few-ULP difference flips their order. The weights move by ~1e-7 (inside this test'snmsetolerance), so the report is exactly the row that flaked at pina3e8d42c9:Why it is near-ties and not ties
I first wrote this up as "exact ties, and the reference's tie order is undefined because
ggml_compute_forward_argsort_f32sorts with an unstable, value-only comparator". That fact is true but it is not the cause, and the arithmetic says so — simulation of this test's own data (numpy float32, 4000 draws per point):gating_func=3gating_func=3The change
test_topk_moe::initialize_tensorskeeps the base class's fill for every other tensor and gives the logits exactly representable values2^-9apart, shuffled per row so the column order is not monotone. Adjacent probabilities then differ by ~1e-3 — three orders above the few-ULP window — so an id can only differ if a producer is genuinely wrong, which is what the test is for. Nothing is skipped, waived, or tolerance-widened.Measured
Same case, same filter (
-o TOPK_MOE -p "ne=\[256,22,1,1\],n_expert_used=6,with_norm=1"), two builds that differ only in that fill, one gpu-hold claim (~/lb/aval/tattn-measure-both-20261108T020208Z):[VIEW],ERR = 70.52^-9(this PR)[VIEW]0So the comparison is demonstrably sensitive to value proximity and demonstrably insensitive once the values are separated — which is the property the test needs, independent of which backend is under test.