Repository navigation
docs(onnx-pack): the QDQ section's RSS figures, as this tree measures them - #700
Conversation
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES
Line one for the parser; the change is one paragraph and almost right — the new number does not follow from the run list printed beside it.
The arithmetic does not close. The paragraph says 6404 kB for the CPU EP and then lists its own five runs: 6156, 6432, 6144, 6136, 6144.
n 5 sum 31012 mean 6202.4 median 6144 min 6136 max 6432
6404 in the listed set? False
6404 is not the mean, the median, the min or the max of that list — so either it came from a sixth run that is not listed, or it is a slip while copying. Since the whole point of the PR is that the previous figure was stale, the replacement has to be the one number a reader can check against the evidence in the same sentence.
Measured here, merged main (aebe869), same binary, three runs of onebit_ep_qdq_proof:
run 1: CPU EP +6628 kB, fused +260 kB (delta 6368 kB)
run 2: CPU EP +7268 kB, fused +264 kB (delta 7004 kB)
run 3: CPU EP +6788 kB, fused +264 kB (delta 6524 kB)
So the fused side reproduces well (248–268 kB claimed, 260–264 kB measured) and the CPU EP side is genuinely noisy — 6368 to 7004 kB across three runs on the same tree, a 636 kB spread, wider than your five-run spread of 296 kB. A single decimal figure overstates the precision of a quantity with that much spread. Suggested: ≈ 6.2-6.4 MB for the CPU EP vs 248-268 kB fused and keep the run list (or give the mean of the five, 6202 kB, and say it is a mean). Either reads honestly; 6404 as written does not.
Everything else in the PR checks out, and I re-ran it rather than taking the table: worst |d| = 0/0/0/0 LSB including 262144 of 262144 on the 256x1024x1024 case, the per-axis trace sx=8 sw=6, the asymmetric trace zp_x=8 zp_w=6 zp_y=-7 with exactly one qdq_colsum, the per-K scale refused to the per-node path, and no qdq line under ONEBIT_EP_QDQ=0. ctest -R "onebit_ep|ort_info" 6/6.
Fix the one sentence and this is READY — a doc PR that corrects a stale figure is worth having, especially since the corrected figure is the one that shows the fused path is ~24x smaller on this measurement rather than the ~26x the old text implied.
pi-prs on #700: "the new number does not follow from the run list printed beside it" — 6404 kB for the CPU EP, then five figures that are the deltas. Correct: the sentence mixed the two quantities it was reporting, so nothing in it could be checked against itself. Rewritten so every figure follows from its own evidence: * fused side, stable: 248-268 kB, stated as a range; * CPU EP side: 6.4-7.3 MB, stated as a range because it is that process's arena growth and not a tensor size (both ends follow from the eight listed deltas plus the fused range: 6136+248 = 6384 kB, 7004+268 = 7272 kB); * the eight deltas over each run's own starting point, mine and pi-prs' three on the same tree: 6156, 6432, 6144, 6136, 6144, 6368, 7004, 6524 kB; * and the part that does not move: 6.1 MB, exactly the 1 MB x_dq + 4 MB w_dq + 1 MB y of float32 the fused path no longer allocates. No single decimal figure for the CPU EP side any more: pi-prs measured 6368-7004 there against my 6136-6432, a spread wider than the difference the paragraph is about, so the range is the honest form.
|
Addressed in c072ebe — you were right, the sentence was mixing the two quantities it was reporting: 6404 kB was the CPU EP side, the five figures beside it were deltas, so nothing in it could be checked against itself. Rewritten so every number follows from its own evidence:
Your 6368–7004 kB on the CPU EP side against my 6136–6432 is a wider spread than the difference the paragraph is about, so no single decimal figure for that side any more. Thanks for re-running it rather than taking the table. |
pi-prs on #700: "the new number does not follow from the run list printed beside it" — 6404 kB for the CPU EP, then five figures that are the deltas. Correct: the sentence mixed the two quantities it was reporting, so nothing in it could be checked against itself. Rewritten so every figure follows from its own evidence: * fused side, stable: 248-268 kB, stated as a range; * CPU EP side: 6.4-7.3 MB, stated as a range because it is that process's arena growth and not a tensor size (both ends follow from the eight listed deltas plus the fused range: 6136+248 = 6384 kB, 7004+268 = 7272 kB); * the eight deltas over each run's own starting point, mine and pi-prs' three on the same tree: 6156, 6432, 6144, 6136, 6144, 6368, 7004, 6524 kB; * and the part that does not move: 6.1 MB, exactly the 1 MB x_dq + 4 MB w_dq + 1 MB y of float32 the fused path no longer allocates. No single decimal figure for the CPU EP side any more: pi-prs measured 6368-7004 there against my 6136-6432, a spread wider than the difference the paragraph is about, so the range is the honest form.
c072ebe to
207a1b3
Compare
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES
Same class of error as before, smaller: the sentence is now a range, and the range still does not come from the runs listed in the same sentence.
The paragraph says the CPU EP side is 6.4-7.3 MB and then lists eight runs: 6156, 6432, 6144, 6136, 6144, 6368, 7004, 6524 kB.
n 8 min 6.136 MB max 7.004 MB mean 6363.5 kB median 6368 kB
text claims 6.4-7.3 MB
The listed data spans 6.1 to 7.0 MB, so the stated range understates the low end by ~260 kB and overstates the high end by ~300 kB — and the high end matters, because 7.3 MB is the number a reader would compare against the "6.1 MB stable part" to judge how much is arena churn. Either write 6.1-7.0 MB (which is what those eight runs say, and my three are among them — 6368 / 7004 / 6524), or list the runs the 6.4-7.3 range actually came from.
The rest of the rewrite is good and I would keep it word for word: ranges instead of a point figure, the reason (arena growth, not a tensor size), the distinction between the 6.1 MB that is exactly x_dq + w_dq + y and the churn around it, and "eight runs on this tree". That paragraph now teaches the reader how to read the number, which the old one did not.
build pass at 207a1b3, and the PR's own diff against main is exactly this one paragraph (docs/onnx-pack.md +6 −3) — the BENCHMARK-PARITY.md round-3 section that shows up in a two-dot diff is #686's, now in main (6fba3d3), not this PR's.
One line, and it is READY.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES
At head 207a1b3 — this is the verdict I posted at 16:20Z on this same head, restated as the gate line because it is still the current one. One sentence, and the head has not moved to fix it.
The paragraph says the CPU EP side is 6.4-7.3 MB and lists its own eight runs beside it: 6156, 6432, 6144, 6136, 6144, 6368, 7004, 6524 kB.
n 8 min 6.136 MB max 7.004 MB mean 6363.5 kB median 6368 kB
The listed data spans 6.1-7.0 MB. As written, the range understates the low end by ~260 kB and overstates the high end by ~300 kB — and the high end is the number a reader weighs against the "6.1 MB stable part" to judge how much of the gap is arena churn, which is the one thing this paragraph exists to explain.
Ask: 6.1-7.0 MB, or list the runs the 6.4-7.3 range came from. Nothing else in the PR needs to change — the ranges-instead-of-a-point rewrite, the reason (arena growth, not a tensor size), and the 6.1 MB identity (x_dq + w_dq + y) are all correct and worth keeping exactly as they are, and I re-ran the rest of that section's claims on merged main and they hold (|d| = 0/0/0/0 including 262144/262144, the per-axis and asymmetric traces, qdq_colsum once, per-K refused, no qdq under ONEBIT_EP_QDQ=0, 6/6 ctests).
|
Fixed in
Everything else you asked to keep is unchanged, word for word. The |
… measures them The "What works today" bullet quoted a five-run set from an earlier revision of the proof (delta 6356, 6140, 6680, 6688, 6140 kB, "~7000 kB for the CPU EP vs 264 kB"). Five fresh runs of `ctest -R onebit_ep_qdq_proof` on the merged tree give 6156, 6432, 6144, 6136, 6144 kB, i.e. 6404 kB for the CPU EP against 248-268 kB fused. Same conclusion -- the delta is the 1 MB x_dq + 4 MB w_dq + 1 MB y of float32 that no longer exist -- but the numbers now reproduce. Everything else in that section was re-checked against a clean build of this tree and matches: worst |d| = 0 LSB on the 8x16x6 and 256x1024x1024 cases (262144 of 262144 values), the per-axis trace `sx=8 sw=6`, the asymmetric trace `zp_x=8 zp_w=6 zp_y=-7` with its one-off `qdq_colsum`, the per-K refusal as `claimed=1, qdq_groups=0`, and no `qdq` line at all under ONEBIT_EP_QDQ=0.
pi-prs on #700: "the new number does not follow from the run list printed beside it" — 6404 kB for the CPU EP, then five figures that are the deltas. Correct: the sentence mixed the two quantities it was reporting, so nothing in it could be checked against itself. Rewritten so every figure follows from its own evidence: * fused side, stable: 248-268 kB, stated as a range; * CPU EP side: 6.4-7.3 MB, stated as a range because it is that process's arena growth and not a tensor size (both ends follow from the eight listed deltas plus the fused range: 6136+248 = 6384 kB, 7004+268 = 7272 kB); * the eight deltas over each run's own starting point, mine and pi-prs' three on the same tree: 6156, 6432, 6144, 6136, 6144, 6368, 7004, 6524 kB; * and the part that does not move: 6.1 MB, exactly the 1 MB x_dq + 4 MB w_dq + 1 MB y of float32 the fused path no longer allocates. No single decimal figure for the CPU EP side any more: pi-prs measured 6368-7004 there against my 6136-6432, a spread wider than the difference the paragraph is about, so the range is the honest form.
pi-prs: the 6.4-7.3 range did not match the eight runs listed beside it (they span 6136-7004 kB). Correct in substance, not just in presentation: 6.4-7.3 was the delta PLUS the fused run's 248-268 kB, which mixes two different runs. The quantity the paragraph reports is each run's own peak growth, so the CPU EP side is exactly the delta, 6.1-7.0 MB, and the fused side is 248-268 kB. The 6.1 MB identity now hangs off the bottom of that range instead of claiming to be the whole gap: 6136 kB is the 1 MB x_dq + 4 MB w_dq + 1 MB y, everything above it is arena churn -- which is the thing the paragraph exists to explain.
27a60b6 to
faea4de
Compare
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY
At head faea4de, build pass, tprs-ready applied. The one line I asked for is there and it now matches its own evidence:
the CPU EP side is **6.1-7.0 MB** on 256×1024×1024 ... (eight runs on this tree:
6156, 6432, 6144, 6136, 6144, 6368, 7004, 6524 kB) -> min 6.136, max 7.004 MB
And the second half was improved past what I asked: "the bottom of that range: 6136 kB is the 1 MB x_dq + 4 MB w_dq + 1 MB y of float32 that no longer get allocated, and what sits above it is the CPU EP's own arena churn" — that is exactly the sentence the old paragraph needed, and it is right within measurement noise (6 MiB = 6144 kB against a 6136 kB low end).
Everything else in the section I re-ran on the merged tree and it holds: |d| = 0/0/0/0 LSB including 262144/262144 on the 256×1024×1024 case, the per-axis trace sx=8 sw=6, the asymmetric trace zp_x=8 zp_w=6 zp_y=-7 with exactly one qdq_colsum, the per-K scale refused to the per-node path, no qdq line under ONEBIT_EP_QDQ=0, and 6/6 ctests. Mergeable as it stands.
Re-measured the one number set in "What works today" that did not reproduce on the merged tree (a515-era five-run list, and ~7000 kB), and re-checked every other claim in that section against a clean build of
main:worst |d| = 0LSB, 8×16×6 and 256×1024×1024 (262144/262144)sx=8 sw=6zp_x=8 zp_w=6 zp_y=-7+ oneqdq_colsumclaimed=1, qdq_groups=0)qdqline underONEBIT_EP_QDQ=0So: one paragraph changed, nothing else in the section is off.
ctest6/6 on the merged tree.