Repository navigation
Conversation
…CONNECTIVITY BECOMES MULTITHREADED
asv preimports the benchmark suite before it runs any setup_cache (asv/runner.py, spawner.preimport() ahead of the run loop), so on a cold cache bench_connectivity's import-time preload_topologies is what fills it -- serially, in the forkserver parent, before a single benchmark starts. CachedFixtures.setup_cache then finds everything already built, and prime(workers=...) never runs on the path it was written for. Filling it from the CLI first puts those reads back in the parallel prime. Worth a second or two on the GitHub runners, which only see the oQU grids; worth rather more on a machine that can reach the four dyamond grids on campaign storage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Main now carries squash-merged versions of work this branch had unsquashed (cached benchmark I/O UXARRAY#1700, lazy/cached neighborhood kernels UXARRAY#1708/UXARRAY#1768, asv env installs UXARRAY#1776, connectivity peakmem benchmarks UXARRAY#1661). Conflicts resolved in main's favor except where the branch adds sharding: - asv-benchmarking-pr.yml: keep the sharded setup/shard/merge jobs; adopt UXARRAY#1776's asv env cache key (ci/environment.yml + asv.conf.json, no restore-keys) in both jobs; drop a duplicated CPU topology step. - asv-benchmarking.yml: take main's unguarded fixture priming; apply the same env cache key fix. - asv.conf.json: take main's matrix and comments; keep the branch's BLAS thread pins in env_nobuild. - .gitignore: keep the per-shard config/results entries. - bench_connectivity.py: accept deletion; superseded by connectivity.py. - neighbors.py, _fixtures.py, _warmup.py, mpas_ocean.py, geometry_samebody*: take main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cmd/bench_io is stale: its content landed on main as UXARRAY#1700 and has since been superseded there (UXARRAY#1768 neighborhood kernels, UXARRAY#1776 asv env). This branch already carries main, so every conflict resolves to the branch's side and the tree is unchanged; the merge only records bench_io's history so PR #2 against it is mergeable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
asv continuous is the same interleaved run plus a comparison, and exits 1 when the comparison finds a regression -- the status it also uses for a broken run -- so any slower benchmark failed its shard (shards 0 and 1 in run 37510066776). Shards now only measure; the merge job compares. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The main merge left a duplicate warm_in_parent import and a docstring-only diff against main; both files now match main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Text only: module docstrings cut to purpose, key constraints and usage; comments to one or two lines of why. Fixes two that had drifted from the code (a stale stderr claim in _threads.resolve, a contradictory matrix note in asv.conf.hpc.json) and a comment above the wheels cache that described the durations cache. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
NeighborhoodReduce ran at r=15, where a 120km face has ~600 neighbors and time_dataset_reduce alone took over a minute per round -- the class that bounded the slowest shard. It now runs at 5, shared with NeighborhoodDask as NEIGHBORHOOD_RADIUS. NeighborhoodBuild swept r over 1, 5 and 15 for every benchmark. The timings now skip 15 (1.3s per call at 120km) and the track_* benchmarks skip 1, where peakmem and nbytes barely move (362k vs 384k at 480km). asv reads params from the method first, so this stays one class and the kernels still compile once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Same shards, caches and pre-builds; about 630 fewer lines. - _merge: union rows and max durations. Every shard's benchmarks.json already lists the whole suite (asv saves all it discovers), and the conflict, ordering and column-realignment paths were unreachable. - _partition: group by class. No setup_cache outside _fixtures spans classes, so the union-find never merged more. --shard now implies --asv-args; the uncalled --bench-args and --config-out are gone, and the report lists the heaviest benchmarks in place of the workflow's inline script. - _machine: write asv's detected specs under the pinned name directly, replacing `asv machine --yes` plus a rename. - Drop the unused _threads.py and asv.conf.hpc.json. The latter had drifted from asv.conf.json (no conda_environment_file), so HPC runs built a different env than CI. stage.pbs now always sets NUMBA_NUM_THREADS (default 8; numba's default counts SMT siblings) and passes -a timeout=1800. - local.sh pins each shard to whole physical cores in socket order, so concurrent shards no longer share SMT siblings on nodes that number them N and N + cores. - PR workflow: YAML anchors for the steps repeated across jobs, one SHARDS setting, and a shorter baseline resolution. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cmdupuis3
added
benchmarking
cmdupuis3
marked this pull request as ready for review
October 7, 2026 23:35
ASV BenchmarkingBenchmark Comparison ResultsBenchmarks that have stayed the same:
Benchmarks that have got worse:
|
…ns from main asv_runner re-runs setup before every sample, about six times per benchmark process. NeighborhoodReduce.time_reduce timed a 5ms kernel at 120km but took 27s, and NeighborhoodDask.time_mean 49s for 1-40ms calls, almost all setup. That made NeighborhoodReduce alone larger than a quarter of the suite, which capped a 4-shard split at 3.2x. - NeighborhoodReduce and NeighborhoodDask build their fixture, warmup and neighborhood once per process. The timed calls only read them. The grid caches a single ball tree and time_dataset_reduce leaves it on edges, so setup restores the face tree each sample, as a fresh setup did. - main's benchmark workflow saves the commit it just ran under the asv-results cache key. PR caches are scoped to their PR, so every PR's first run had no durations and split by count (this branch's first run: shards of 3:11, 4:48, 2:17 and 9:49). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sevans711
reviewed
Oct 8, 2026
Sevans711
left a comment
Sevans711
left a comment
Collaborator
There was a problem hiding this comment.
Great overall, I'm excited for very fast ASV benchmark runs! I read through all the changes and they look pretty reasonable to me. Because this is all for benchmarks, not anything user-facing, I didn't take the time to spin up a local or HPC environment to test it directly for myself. It seems to work, and I think that should be good enough? And, if there's some subtle bug that gets missed here, it could always be fixed in a follow-up PR, without users ever being affected directly.
I think there's only one important thing to fix before merging, and that would be the roughly 11% to 30% increases to track_peakmem for many benchmarks here. I would hope to see no changes to peakmem usage, to avoid re-introducing something like #1605. Any guesses on what is causing this?
Beyond that, I also have a few clarifying questions, just so I can understand this a bit better:
- Where do the sharding timing weights come from when you make a new github action ASV run? I can understand from the code that if you run things locally you are just using the previous job's weights to decide how to group jobs into shards. Are these being cached somewhere on github? What happens if the cache doesn't exist?
- Can you clarify, are the changes in
mpas_ocean.pyrelated to the sharding at all, or are they a completely separate improvement (i.e. is this part of the original issue or an expansion of PR scope)? I don't necessarily have any concerns about those changes, just trying to understand the motivation for them a bit better.
Collaborator
Author
|
@Sevans711 The memory issues are ongoing, I think this issue is something about
|
There was a problem hiding this comment.
I would recommend fusing/melding commits together so that the history of changes are more tractable.
My understanding of the changes in this PR,
The benchmark test suite run using asv (Airspeed Velocity) in github CI workflow takes up to 20-30 mins to run. This PR shards (breaks up) the benchmark test suite into multiple independent jobs, containing batches of tests, that run in parallel. Running these test shards/batches in parallel reduces the overall time taken to run the benchmark. However since the project does not use custom testing frameworks that support sharding the mechanism of sharding/batching tests is implemented manually.
The PR contains the following changes,
- A helper script, _machine.py, that captures the "machine/node name" from the environment (or platform)
- Helper scripts for sharding/partitioning the tests into batches and gathering the test results
** Script _partition.py : Reads benchmark.json (asv output, one for each run) from result dirs to calculate mean "duration" (wallclock time for benchmark) for each benchmark. The benchmarks are binned greedily (sort benchmarks using duration, longest first, and add to bin with the lowest load) into separate shards/bins (number of bins is a parameter to script). The asv config file (parameter to script) is updated with the shard information.
** Script _merge.py : Since the shard workflow jobs are run in separate directories (and the results are in these directories) this script consolidates/merges the results (benchmarks.json) and machine files (machine.json) in each result directory into a separate result directory (benchmarks.json in this directory contains results from all shard jobs). - Since all shard jobs need to use the same input files (oQU*.nc), asv files (wheels) these files are cached
- Misc fixes
** Adding env for OMP, MKL, BLAS
** Updates to MPAS neighborhood tests
| env: | ||
| BASE: ${{ needs.setup.outputs.base }} | ||
| run: | | ||
| set -ex |
There was a problem hiding this comment.
Is it possible to use pre-built actions here (actions/upload-artifact/merge@v4)?
| merged = {} | ||
| for shard_dir in map(Path, shard_dirs): | ||
| for path in sorted(shard_dir.rglob("*.json")): | ||
| rel = path.relative_to(shard_dir) |
There was a problem hiding this comment.
Looks like the function is merging files with the same name across directories. Does the function also merge json files other than benchmarks.json ("results" and "duration" - referred later - seem to be just in the benchmarks.json file)?
Renaming rel ("relative file path/name") to something like "fname" makes the intent more clear
| for path in sorted(shard_dir.rglob("*.json")): | ||
| rel = path.relative_to(shard_dir) | ||
| data = json.loads(path.read_text()) | ||
| if rel not in merged or path.name in _WHOLE_FILES: |
There was a problem hiding this comment.
Do you need the extra check in _WHOLE_FILES?
| for path in sorted(shard_dir.rglob("*.json")): | ||
| rel = path.relative_to(shard_dir) | ||
| data = json.loads(path.read_text()) | ||
| if rel not in merged or path.name in _WHOLE_FILES: |
There was a problem hiding this comment.
Adding a comment that you are trying to find the first file to append would be useful. You might also want to instead do append vs initialize based on whether the merged[fname] is set or not (make the intent more clear)
|
|
||
|
|
||
| def load_weights(results_dirs): | ||
| """Mean recorded duration per benchmark, in seconds. |
There was a problem hiding this comment.
Why not use median (to get rid of outliers) here too?
| for results_dir in results_dirs: | ||
| for path in sorted(Path(results_dir).glob("*/*.json")): | ||
| data = json.loads(path.read_text()) | ||
| columns = data.get("result_columns") or [] |
There was a problem hiding this comment.
Looks like you want to read the "duration" for each "benchmark name" in "results". Avoiding accessing "result_columns" might simplify this code
| class_cost = {owner: sum(cost[name] for name in names) for owner, names in classes.items()} | ||
|
|
||
| shards = [[] for _ in range(n_shards)] | ||
| loads = [0.0] * n_shards |
There was a problem hiding this comment.
Looks like you are binning greedily a sorted (longest duration in the front) list of benchmarks. Having a list of tuples, (benchmark_name, benchmark_cost), and iterating over that list would make the intent more clear. Also renaming classes/owner (or adding more comments) would help.
| params = DatasetBenchmark.params + [['mean', 'median']] | ||
|
|
||
| radius = 15.0 | ||
| radius = NEIGHBORHOOD_RADIUS |
There was a problem hiding this comment.
Was this intentional (NEIGHBORHOOD_RADIUS is 5 now)?
Collaborator
Author
There was a problem hiding this comment.
Yes, the radius parameter scales the algorithm cost as O(N^2), so these neighborhood algorithms are very expensive in general. I chose radii that will hopefully show the scaling of the radius parameter on the grids we use, without needlessly bloating the compute on the shard the neighborhood filter benchmarks land on.
| if: always() | ||
| with: | ||
| name: asv-benchmark-results-${{ runner.os }} | ||
| name: asv-benchmark-results-Linux |
|
As the number of benchmark runs (previous/older) increase you might also want to investigate alternatives to scanning results from all previous runs to calculate mean benchmark runtimes (when binning the tests to shard jobs). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1814
Overview
This PR reworks the ASV CI workflow to split the benchmarking suite into separate jobs, allowing for parallel benchmarking runs.
Basically, all the benchmarks are scored and are roughly load-balanced based on the number of threads available. Each job runs independently, so there is some overhead with redundant benchmark cache builds. This work can also support larger HPC-scale jobs, but I'm deferring the actual HPC scripts to another PR.
Current timings show benchmark suite wall time usually around 6-8 minutes, versus 20-30 minutes for the current baseline.
PR Checklist
General
Testing & Benchmarking
Documentation and Examples
docs/api.rst; internal (private) function names start with an underscore (_)AI Disclosure
AI Usage: Claude Opus 5, Opus 5.5