ASV Benchmarks Integration#310
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an ASV benchmark suite for mkl_fft (root API + NumPy/SciPy interfaces + memory) along with configuration, docs, and ignore rules to support running and storing benchmark artifacts in CI.
Changes:
- Introduces ASV benchmark modules covering 1-D, 2-D, and N-D FFTs across multiple dtypes and shapes (including Hermitian variants for SciPy).
- Adds ASV configuration (
asv.conf.json) and documentation for local/CI execution. - Adds benchmark package initialization that pins thread counts via environment variables, plus
.gitignoreentries for ASV artifacts.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
benchmarks/benchmarks/bench_scipy_fft.py |
Adds full-coverage ASV benchmarks for mkl_fft.interfaces.scipy_fft including Hermitian 2-D/N-D. |
benchmarks/benchmarks/bench_numpy_fft.py |
Adds full-coverage ASV benchmarks for mkl_fft.interfaces.numpy_fft including Hermitian 1-D. |
benchmarks/benchmarks/bench_memory.py |
Adds ASV peak-RSS benchmarks for core mkl_fft transforms across 1-D/2-D/3-D. |
benchmarks/benchmarks/bench_fftnd.py |
Adds 2-D and N-D root-API benchmarks including non-square and non-power-of-two cases. |
benchmarks/benchmarks/bench_fft1d.py |
Adds 1-D root-API benchmarks for power-of-two and non-power-of-two sizes (C2C and R2C/C2R). |
benchmarks/benchmarks/__init__.py |
Adds thread pinning logic via MKL_NUM_THREADS/OMP_NUM_THREADS/OPENBLAS_NUM_THREADS. |
benchmarks/asv.conf.json |
Adds ASV project configuration (dirs, branches, timeouts, regression thresholds). |
benchmarks/README.md |
Documents benchmark structure, coverage, threading rationale, and local/CI run commands. |
.gitignore |
Ignores ASV artifact directories. |
ndgrigorian
left a comment
ndgrigorian
left a comment
There was a problem hiding this comment.
no other comments from my side, thank you @vchamarthi
|
@ndgrigorian Thanks for the review, can you please merge it? i do not have enough permissions to merge. |
I'll merge tomorrow, after I give Anton a chance to take a quick look. |
| else: | ||
| half_shape = shape[:-1] + (shape[-1] // 2 + 1,) | ||
| self.x_real = rng.standard_normal(shape).astype(dtype) | ||
| self.x_complex = ( |
There was a problem hiding this comment.
That does not enforce Hermitian symmetry but is used for hfft benchmarks.
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class BenchC2C1D(BenchC2C): |
There was a problem hiding this comment.
Would it make sense to combine benchmarks for numpy_fft and scipy_fft by adding module to the parameter list as scipy did?
The benchmark code seems similar for both numpy_fft and scipy_fft use cases.
|
|
||
| def setup(self, shape, dtype): | ||
| rng = np.random.default_rng(_RNG_SEED) | ||
| self.x = _make_input(rng, shape, dtype) |
There was a problem hiding this comment.
I wonder if we need any warmup iteration. At the first run there will be time spent to create a DFTI descriptor handle. Each next run will reuse the same descriptor.
I wonder if it might be needed to consider that and to add a warmup iteration to exclude the first run from the measurements.
| @@ -0,0 +1,21 @@ | |||
| """ASV benchmarks for mkl_fft""" | |||
|
|
|||
| import os | |||
There was a problem hiding this comment.
Would it make sense to add one more optional dependency declaration to pyproject.toml? Like: benchmark = ["asv>=0.6", "psutil"] or similar?
| this suite catches regressions in that path. | ||
| """ | ||
|
|
||
| params = [_SIZES_NONPOW2, ["float64", "complex128", "complex64"]] |
There was a problem hiding this comment.
Why we don't use _DTYPES_REDUCED or _DTYPES_ALL here?
| .asv/ | ||
| benchmarks/.asv/ |
There was a problem hiding this comment.
The first pattern already covers the second one
| .asv/ | |
| benchmarks/.asv/ | |
| .asv/ |
| "default_benchmark_timeout": 500, | ||
| "regressions_thresholds": { | ||
| ".*": 0.3 | ||
| } |
There was a problem hiding this comment.
Do we need also to configure environment_type?
Adds a complete ASV benchmark suite for mkl_fft and wires it into the internal Jenkins CI pipeline.
Benchmarks added (
benchmarks/benchmarks/):bench_fft1d.py— 1-D C2C and R2C/C2R, power-of-two and non-power-of-twobench_fftnd.py— 2-D and N-D C2C and R2C/C2R, square, non-square, non-power-of-twobench_numpy_fft.py— full coverage of mkl_fft.interfaces.numpy_fftbench_scipy_fft.py— full coverage of mkl_fft.interfaces.scipy_fft including Hermitian 2-D/N-Dbench_memory.py— peak RSS for 1-D, 2-D, and 3-D transforms__init__.py— pins MKL_NUM_THREADS=4 on machines with ≥4 physical cores for consistent cross-machine results (A hack to keep the results consistent on random nodes until CI finds a stable benchmarking machine)Config (
benchmarks/asv.conf.json)Docs (
benchmarks/README.md): structure, coverage, threading rationale, local run commands, CI flow.