Repository navigation
Proposal: a regression benchmark suite for comparing commits (results, speed, memory) #380
Replies: 5 comments
|
Very good suggestion, we definitely need this before we release all the current work in v1.1 – the exact Fresnel propagator change alone will break numerics (for the very good reason that it is simply more correct without real loss in performance), and it would be great to benchmark and be transparent about this. I think I need to get back home from my conference marathon and rest a bit before I can think on this level, I don't want to leave this up to Claude :) But I should be able to provide feedback and allow this to be started next week. Thank you! |
|
Paul, now that I am back and have had time to think about this properly: yes, let's do it, and thank you for framing it as a discussion before writing the code. The motivation is not hypothetical, and it is worth spelling out how bad the current situation actually is. The benchmarks/ directory is now about 4,500 lines across 13 scripts, and two of them — compare_benchmarks.py at 464 lines and the --compare mode inside benchmark_comparison.py at 368 lines — are independent implementations of the same comparison table, written weeks apart. Consolidating that would be worth doing even if we got nothing else out of it. The layering you propose is the right one, and I want to point out the two details that I think will decide whether this succeeds, because you have already got both of them right. The first is that the harness and the case definitions come from the invoking checkout while only the abtem package is swapped, so both refs really do run identical case code. The second is that a case whose API does not exist on one ref records "unsupported" instead of bringing down the run. Suites like this usually die because one of those two things was not thought through at the start. Before I answer the questions, there are two larger points I would like you to weigh, one about ordering and one about numerical precision. Ordering: what v1.1 needs first is a comparison against v1.0.10 What we concretely need from this suite in the next few months is a clear answer to: how does v1.1 differ numerically from v1.0.10, workload by workload, and which of those differences did we intend? The exact Fresnel propagator is the obvious example. It is now the default in abtem/multislice.py, it is simply more correct, it costs us essentially nothing in performance, and it will move the numbers in almost every case we run. I would very much like to be transparent about that in the release notes rather than have someone discover it for us in six months. So I would flip your milestones around: capture reference arrays at the v1.0.10 tag first, diff dev against them, and treat live branch-vs-branch A/B as the second mode built on the same compare layer. Most of the machinery is shared, and the reference mode is the one that pays off before v1.1 ships. Precision: I think the accuracy tier should run in float64 I would rather the accuracy tier ran at precision: float64 and compared bit-exactly. "Bit-identical across seventeen workloads" is then a statement with real content, and anything that is not bit-identical is a genuine algorithmic change that deserves a sentence in the changelog. The float32 path would get a separate, looser check of its own: does it still track its own float64 result about as closely as it did before? That question is meaningful, whereas "did float32 change in the seventh digit" is not. Answers to your six questions
Chunked potential building. The potential.slice-chunk-size default is "auto" and the auto value depends on available memory, so this needs both pinning in the preset and a case of its own. One practical correction about the CI tier: GPAW is not pip-installable for us. The abtem[gpaw] extra only pulls in hankel and sympy; GPAW itself comes from conda. So the GPAW core-loss cases cannot run on hosted runners. Use the built-in transition potentials for the CI tier and keep the GPAW cases for the local and standard tiers.
For the frozen-phonon cases, please also assert that the seed is consumed in the same way, not just that it is set. Otherwise a refactor of the sampling order will present itself as a change in physics.
The reference arrays, however, cannot live in the main repository, and they cannot live in the docs repository either. The main repository is already around 160 MB and the docs repository is over 500 MB, and reference data accumulates along three axes at once: per release tag, per case, per device. Carrying that in either repository means every clone pays for it forever, and I do not want to do that to contributors. We already have a cautionary example in-tree: test/data/silicon_diffraction_patterns.zarr is a single 9.9 MB blob, and the test that used it has been commented out since the 1.0 line (test/test_measure.py:808). It is dead weight in every clone and it is not even buying us a test. So I would repurpose abtem-benchmarks for this. It is still only about 1 MB, it is in the same organisation, and it already exists for benchmark-adjacent material, so it is a natural home. The harness would fetch a reference bundle for a given tag on demand and cache it locally. If the bundles turn out to grow faster than we expect, attaching them as release assets in that repository rather than committing them keeps the git history clean while leaving the URLs stable. A new dedicated repository would also be fine if you would rather keep the scaling studies undisturbed; I have no strong preference between those two, only that it is not the main repository. Two smaller asks on the in-tree part. Make it a properly importable package so that python -m abtem_bench actually works, and put it under ruff and mypy so it does not rot the way the one-off scripts did. And because the cases come from the invoking checkout, please record a hash of the case definitions in every result file — two reports are only comparable if the case code behind them was the same, and that is the kind of thing we will otherwise discover the hard way. Also, please do not delete the one-off scripts until the case each one encodes has actually been reproduced in the new harness. benchmark_potential_chunking.py is over 1,500 lines and a fair amount of that is accumulated tuning knowledge rather than a timing loop. It is worth mining before it is removed.
One last thing worth pinning Please go ahead with the skeleton. If you start from the reference-array mode and the three cases that would feed the v1.1 release notes, it becomes useful to us immediately rather than at the end of the fourth milestone. Thanks again, 🤖 Drafted with Claude Code, reviewed and edited by Toma. |
|
Thanks Toma, this is exactly the input needed. The design has been revised along your lines and the full version is in the abtem-benchmarks repository under Ordering: reference mode firstAgreed, and the milestones are flipped. M1 delivers One detail that makes the release-notes table attributable: v1.0.10 is the tip of Precision and tolerancesAgreed on both. The accuracy preset runs at The float32 path gets the separate check you describe: each case also runs at float32 in the same bundle, the metric is its distance from its own float64 result, and compare flags only when that distance grows relative to the reference bundle. The per-output comparison is the vector, not a boolean: bit-identical flag, max absolute difference over max of the reference, relative error restricted to elements above 1e-6 of the maximum, integrated intensity, and exact equality of shape, dtype and axes. Frozen-phonon cases emit the consumed random state as an output ( Drift policyAdopted as you describe: drift beyond tolerance fails the check and is cleared by an entry in CasesAll of your additions are in the matrix. Chunked potential building is its own case with explicit chunk sizes (1, 5, all) whose outputs must agree with each other, and Two more cases come from the issue history rather than from the API list. One correction to the CI point: abTEM has no built-in transition potentials. Both radial solvers in Where things live, and the two smaller asksHarness and cases in the main repository under One small proposal on naming: the repository is Reference bundles go to abtem-benchmarks as release assets ( Every manifest records a sha256 over the case sources and the harness version; compare refuses to pair bundles with different hashes unless told to, and says so in the report. The fully resolved On the legacy scripts: nothing is deleted until M4, and only per reproduced case. All 13 have been copied to abtem-benchmarks under Measurement details worth statingPeak host memory comes from CIAs agreed: label-triggered, quick tier, CPU, float64, PR head against its merge base in one job, bit-identical expected, MilestonesM0 (done): consolidation of design, legacy scripts and the reference-bundle home in abtem-benchmarks. M1: reference mode, harness core, the four v1.1 cases, self-check, v1.0.10 quick-tier CPU bundle, the dev versus v1.0.10 report with If the outline reads right, M1 starts now and the first thing to appear here will be the dev versus v1.0.10 table. 🤖 Written by Claude Code — Paul reviewed and posted it |
|
A follow-up on where the non-code parts should live, because rereading the design raised a doubt worth settling before M1 rather than after. With the harness and the cases in the main repository and the reference bundles delivered as release assets, the question is what Two things speak against putting the bundles on abTEM releases. Bundles are captured after the tag by a later harness and get re-captured whenever the case matrix changes, so one tag would accumulate several generations of multi-hundred-MB assets on the page users visit to download abTEM. And What the main repository cannot hold well is a results history. Once the suite runs weekly on our own hardware it produces per-commit speed, memory and self-check records, a few KB of JSON each, that do not map to releases and grow with every run. That series is what answers "when did this get slower" and what the required-check decision will be based on, and it is why asv keeps a separate results repository. The 2023 scaling-study notebooks already in Options, then:
Option 1 seems the better fit, but this is a maintainer's call. The consolidation done so far (design docs, legacy copies) is easy to move either way, so nothing is blocked; M1 only needs to know where 🤖 Written by Claude Code — Paul reviewed and posted it |
|
A second follow-up, on what the case matrix reaches below the workflow level, and on how a regression there actually gets found in the first place. #438, open now, is a good concrete test of the design, and it fails on both counts. It was found by investigating low GPU utilization on a production core-loss EELS scan running on a single A100: The kernel-level numbers in the PR show why that is batch-dependent rather than a flat cost: on the A100 the scan ran on, 5.1x at a small batch (10 rows) against 399.5x and 442.6x at the batch sizes a real That is the first gap: attribution. A regression in The natural next question is why not build that same time breakdown into the workflow cases directly, timing potential build, multislice and detection as named phases of Lacking anywhere better to put it, the fix's own test file shows the cost of that gap. This is not a one-off kernel, either. Proposal: keep the workflow cases exactly as designed, unperturbed, as the signal that something changed — and add two more mechanisms underneath them that answer the questions a workflow case cannot. The first is a profiling mode, not a case: The second is a Neither addition changes the earlier question about the two repositories: the profiling mode and the routine cases are both code, so they sit with the harness in Both additions revise the design as posted: the profiling mode is a new component with no analogue in it, and the routine tier changes a line in the non-goals list. That seemed worth raising here before either goes into the design document rather than folding them in unilaterally. If the split looks right, 🤖 Written by Claude Code — Paul reviewed and posted it |
Uh oh!
There was an error while loading. Please reload this page.
Hi all,
I would like to build a benchmark suite for abTEM and want to agree on what it is for before writing code. Short version: one command that runs a fixed set of standard simulations against two commits and reports, per case, whether the results changed, how the speed changed, and how the peak memory changed. Feedback on the goals and the case list is what I am after; the design details are collapsed at the end.
Why now
Every recent performance PR has shipped its own script and its own results table in its own format: #269, #289, #309, #346, #347, #366, and
benchmarks/on dev now holds 13 one-off scripts that each re-implement subprocess isolation, RSS/VRAM sampling, JSON output and a compare printer.Result changes between versions are currently found by users, not by us. In #261 a version-to-version difference in diffraction intensities turned out to be a convention change plus a slice-assignment bug introduced by a potential-building speedup, found months later and fixed in #262. In #281 a user asked why 1.0.9 timed differently from 1.0.8, and we had no reference to answer with.
We have no way to say "this commit is bit-identical to the previous one on these 17 workloads and 8 % faster on GPU" except by trusting whatever script the PR author wrote that week.
What it should achieve
What it is not
abTEM/abtem-benchmarksrepository (scaling studies from 2022/23) or the docs are better homes for it.What using it would look like
The report is a table with one row per case, device and tier, giving a result verdict (identical, within tolerance, drift, shape change, unsupported on one ref, error, OOM, timeout), the time ratio flagged against a measured noise floor, and the peak RSS/VRAM ratio. A self-check mode runs the same commit twice to measure that noise floor and to verify that CPU results are bit-reproducible under deterministic settings.
Proposed initial cases
Potential building (infinite and finite projection, CrystalPotential), HRTEM exit wave and image, SAED and CBED, HAADF scan, multi-detector scan (BF + ADF + segmented), 4D-STEM, flexible annular detector, thickness series, PRISM scan, frozen-phonon HAADF (fixed seed), core-loss image and PRISM core-loss (GPAW), Bloch-wave diffraction. Three sizes each: quick (CI, seconds), standard (minutes), large (GPU stress at the sizes the existing scripts use). Energy ensembles, tilt series, plasmons and magnetism would follow once the matrix is stable.
Questions where I want your input
benchmarks/in the main repo, since cases track the API of the code they test and CI needs them in-tree, with the one-off scripts consolidated into it and then removed. The dormantabtem-benchmarksrepo would stay as the home for scaling studies.Unless there are objections, I will start with a skeleton and three cases in about a week, so that the discussion can continue on a concrete PR.
Design details
Layers. Declarative cases (pure abtem API calls), a runner that executes each case in its own subprocess with time and memory meters, and a compare tool that works on a commit-independent result format (NumPy
.npzplus JSON sidecars: arrays, dtype, shape, axes, invariants). The harness and the cases always come from the invoking checkout; only theabtempackage is swapped per ref, so both refs run identical case code, and a case whose API is missing on one ref records "unsupported" instead of failing the run.Swapping refs. abtem is pure Python, so each ref is a git worktree put on
PYTHONPATHin a shared environment; an isolateduv venvper ref is the fallback for refs across the numpy 2 and zarr 3.1 pin changes. Refs are interleaved per case (A, B, A, B) so thermal drift hits both alike.Determinism. Cases pass explicit
gpts(neversampling, becausegrid.round-to-fast-fftcan change derived grids between commits), pinmax_batchand chunk sizes (the auto values depend on free memory), fix seeds, and the accuracy preset uses FFTW_ESTIMATE and one worker thread so CPU results are bit-reproducible. Auto-sized variants are reported but never flagged.Meters. Wall time with GPU synchronisation, cold-start time (JIT, FFTW planning, CuPy kernel compilation) recorded separately from warm repeats, peak RSS by sampling plus
ru_maxrss, peak VRAM from both the CuPy pool and the driver (to capture cuFFT workspace outside the pool).Why not asv. It has no array-result comparison (only scalar tracking), no GPU memory, its per-commit environment management fights uv and CuPy builds, and
--python=samecannot compare two commits. pytest-benchmark is in-process timing only. Both would still need the accuracy layer written by hand, so we would end up with two systems.Milestones. (1) skeleton, three cases, self-check validated; (2) full case matrix and consistency pairs; (3) isolated environments, timeouts and OOM classification, report polish; (4) remove the legacy scripts, add the CI check, docs page.
Best,
Paul
🤖 Drafted with Claude Code, reviewed and edited by Paul.
All reactions