Skip to content

ENH: Add performance benchmark CI builds#612

Open
thewtex wants to merge 2 commits intoInsightSoftwareConsortium:mainfrom
thewtex:ci-performance-builds
Open

ENH: Add performance benchmark CI builds#612
thewtex wants to merge 2 commits intoInsightSoftwareConsortium:mainfrom
thewtex:ci-performance-builds

Conversation

@thewtex
Copy link
Copy Markdown
Member

@thewtex thewtex commented Mar 21, 2019

No description provided.

@thewtex thewtex force-pushed the ci-performance-builds branch 8 times, most recently from 6a32231 to ce113c5 Compare March 22, 2019 21:29
@jhlegarreta
Copy link
Copy Markdown
Member

Speechless. Awesome.

@thewtex thewtex force-pushed the ci-performance-builds branch 5 times, most recently from 71f2cc5 to 111d88b Compare March 29, 2019 16:55
@thewtex thewtex force-pushed the ci-performance-builds branch 2 times, most recently from c0f2bbb to 289543a Compare April 8, 2019 22:22
@thewtex thewtex force-pushed the ci-performance-builds branch 4 times, most recently from fa20324 to 96b6114 Compare April 10, 2019 01:52
@thewtex thewtex marked this pull request as ready for review April 10, 2019 01:53
@thewtex thewtex force-pushed the ci-performance-builds branch from 96b6114 to b1d0d43 Compare April 11, 2019 20:39
@thewtex thewtex force-pushed the ci-performance-builds branch from b1d0d43 to 9cc56fa Compare April 23, 2019 22:26
@kwrobot-v1
Copy link
Copy Markdown

kwrobot-v1 Bot commented Apr 23, 2019

Errors:

  • Failed to run the checks: service error.

@thewtex
Copy link
Copy Markdown
Member Author

thewtex commented Apr 24, 2019

Errors:

* Failed to run the checks: `service error`.

CC @bradking @mathstuf

@thewtex thewtex force-pushed the ci-performance-builds branch from 9cc56fa to 1e53cfb Compare April 24, 2019 15:50
@mathstuf
Copy link
Copy Markdown
Contributor

Hmm. Seems we got a timeout on one of our queries.

Do: check

@thewtex thewtex force-pushed the ci-performance-builds branch 3 times, most recently from 0948b8f to 698efc6 Compare April 25, 2019 13:54
@github-actions github-actions Bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Jun 1, 2024
@jhlegarreta jhlegarreta force-pushed the ci-performance-builds branch from 8e4c0f3 to c6b6b60 Compare June 1, 2024 17:53
@jhlegarreta
Copy link
Copy Markdown
Member

jhlegarreta commented Jun 1, 2024

Updated macOS version from 10.13:
c93e0c6#diff-c8d4e43125ff7fbe0d808694e0b2ab37de80808b1d9a54fed631330117d3cd4bR90

to macOS 11, and Python from 3.7 to 3.9:
c93e0c6#diff-c8d4e43125ff7fbe0d808694e0b2ab37de80808b1d9a54fed631330117d3cd4bR104

Preferred to avoid style changes to the existing parts, e.g.:
c93e0c6#diff-c8d4e43125ff7fbe0d808694e0b2ab37de80808b1d9a54fed631330117d3cd4bR8

Not sure if this will be enough to have this working.

If Azure allows to re-use workflows, cleanest would be to relocate the performance benchmarking part to a separate file so that the macOS build part can report its status regardless of the benchmarking part.

@jhlegarreta
Copy link
Copy Markdown
Member

So the benchmarks add 22 minutes;
https://dev.azure.com/itkrobotmacos/ITK.macOS/_build/results?buildId=11489&view=logs&j=68e22afd-607b-5133-9236-337c135f0832

If this is deemed excessive, they could be triggered e.g. once or twice a week.

The next thing will be how to display those reports on a website.

@thewtex
Copy link
Copy Markdown
Member Author

thewtex commented Jun 3, 2024

I started an ASV port on the ITKPerformanceBenchmarking repository. Eventually we should be able to run asv compare similar to what @yarikoptic did with datalad.

@yarikoptic
Copy link
Copy Markdown

FWIW we ran into some minor issues with recent asv and our setups, just in case you do too -- might want to restrict for now to < 0.6.2 and make sure that you have asv[virtualenv] in your setup*, as we did in https://github.com/con/fscacher/pull/91/files#diff-fa602a8a75dc9dcc92261bac5f533c2a85e34fcceaff63b3a3a81d9acde2fc52R49 which is yet another project of ours which does similar benchmarking on CI

@hjmjohnson hjmjohnson modified the milestones: ITK 6.0.0, ITK 6.0 Beta 1 Jan 24, 2025
@hjmjohnson hjmjohnson marked this pull request as draft February 15, 2026 21:13
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request Apr 19, 2026
Add .github/workflows/perf-benchmark.yml, an opt-in GHA workflow
that runs the ASV-based performance regression harness hosted at
InsightSoftwareConsortium/ITKPerformanceBenchmarking on pull
requests labelled 'performance' (or via manual dispatch).

The workflow:

  - Computes BASE = merge-base(PR, origin/master) and builds ITK at
    both BASE and PR HEAD with Module_PerformanceBenchmarking=ON,
    BUILD_EXAMPLES=ON, BUILD_TESTING=ON (required for the
    ExternalData fixture fetch), and BUILD_SHARED_LIBS=OFF. The two
    builds share a ccache so the second build is mostly incremental.

  - Builds only the benchmark executables plus ITKBenchmarksData,
    avoiding a full ITK test suite build.

  - Checks out InsightSoftwareConsortium/ITKPerformanceBenchmarking
    at the PERF_HARNESS_REF env var (defaults to the asv-native-
    harness branch while that work is landing upstream; flip to a
    merged SHA afterwards).

  - Creates the 'itk-repo' symlink required by the harness's
    asv.conf.json, installs the itk_perf_shim Python package, and
    runs 'asv run --set-commit-hash <SHA>' against each build so
    result labels track ITK history.

  - 'asv compare --factor=1.10 --split' produces a markdown diff;
    posted as a sticky PR comment and uploaded as an artifact along
    with the 'asv publish' HTML tree.

Opt-in via the 'performance' label keeps the two-ITK-build cost
off the default PR path. No changes to any existing workflow or
Azure Pipelines file.

Replaces the intent of the long-stalled thewtex/ITK:ci-performance-
builds branch (InsightSoftwareConsortium#612), which proposed
an Azure Pipelines job targetting the retired macos-11 image and
the legacy evaluate-itk-performance.py driver.
@hjmjohnson hjmjohnson force-pushed the ci-performance-builds branch from c6b6b60 to fbbbf29 Compare April 22, 2026 00:01
@github-actions github-actions Bot added area:Remotes Issues affecting the Remote module and removed type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Apr 22, 2026
@hjmjohnson hjmjohnson force-pushed the ci-performance-builds branch from fbbbf29 to cfa9a3f Compare April 22, 2026 00:07
@hjmjohnson hjmjohnson changed the title WIP: ENH: Add performance benchmark CI builds ENH: Add performance benchmark CI builds Apr 22, 2026
@hjmjohnson hjmjohnson force-pushed the ci-performance-builds branch from cfa9a3f to c4e39a6 Compare April 23, 2026 00:51
@github-actions github-actions Bot added the type:Enhancement Improvement of existing methods or implementation label Apr 23, 2026
@hjmjohnson
Copy link
Copy Markdown
Member

@thewtex — the rebase + origin/masterorigin/main workaround in .github/workflows/perf-benchmark.yml (commit c4e39a64) got the asv base vs head check green (11m59s). All Azure C++ pipelines pass on the fresh tip c4e39a64; remaining pending are the usual long-runners (Python-wrapping + Pixi-Cxx).

To remove the in-workflow patch permanently:

  1. Merge InsightSoftwareConsortium/ITKPerformanceBenchmarking#113 (2-line fix to asv.conf.json + README-asv.md).
  2. Bump Modules/Remote/PerformanceBenchmarking.remote.cmake GIT_TAG here to that merge SHA.
  3. Delete the python -c sed block from the Stage ASV harness step.

Adds .github/workflows/perf-benchmark.yml — a GitHub Actions workflow
that builds ITK twice (merge-base and PR HEAD) with the
PerformanceBenchmarking remote module enabled, then drives the ASV
harness from InsightSoftwareConsortium/ITKPerformanceBenchmarking#111
against each build to produce an asv compare report.

Bumps the PerformanceBenchmarking remote module pin from
7950c1d76095033edbf7601a925cdacc2fe717f9 to
41bf1b9cfbaa1b146e1b3e5d3ad3571b2203ce1e (master HEAD, post-InsightSoftwareConsortium#111).
The new pin carries:

  * asv.conf.json, benchmarks/{core,filtering,segmentation}.py,
    python/itk_perf_shim/ (the ASV harness).
  * Hardened examples/Core/itkCopyIterationBenchmark.cxx that no
    longer crashes on VectorImage under ITK >= 1d87efa (the VLV
    null-data regression; see
    InsightSoftwareConsortium#6087 for the parallel ITK-side
    defensive null-guard).

Supersedes the 2019 Azure Pipelines macos-11 approach; macos-11 is
retired and the older evaluate-itk-performance.py driver has been
replaced by the ASV-native harness.

For the very first PR (this one) the merge-base lacks the new pin,
so its benchmark run is best-effort and the asv compare step is
wrapped with continue-on-error. The workflow becomes fully
useful for regression detection after this PR lands.
ITKPerformanceBenchmarking#113 merged as b24324f922, making the in-
workflow python/json sed-patch that the earlier revision of this PR
carried unnecessary.  Bumping the GIT_TAG to pick up the upstream fix
and reverting to thewtex's original, unpatched perf-benchmark.yml.
@hjmjohnson hjmjohnson force-pushed the ci-performance-builds branch from c4e39a6 to 2359970 Compare April 23, 2026 02:28
@hjmjohnson
Copy link
Copy Markdown
Member

@thewtex — ITKPerformanceBenchmarking#113 merged (b24324f922). Force-pushed tip 2359970c24 that (1) bumps Modules/Remote/PerformanceBenchmarking.remote.cmake GIT_TAG to the merge SHA and (2) drops the python -c sed-patch from the Stage ASV harness step — the workflow is now back to your original minimal form.

Net diff vs the pre-rebase tip cfa9a3ff0d:

  • Modules/Remote/PerformanceBenchmarking.remote.cmake +1/-1 (pin bump)
  • rebased onto current upstream/main (was 11 commits behind, now 0)

Fresh CI kicked off; should settle similarly to the previous run (asv was 11m59s last round).

@hjmjohnson hjmjohnson marked this pull request as ready for review April 23, 2026 02:40
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 23, 2026

Greptile Summary

This PR adds a new GitHub Actions workflow (perf-benchmark.yml) that runs ASV (airspeed velocity) benchmarks comparing the PR HEAD against its merge-base, and bumps the PerformanceBenchmarking remote module pin to a commit that introduces the required ASV harness.

  • P1: The workflow_dispatch trigger causes the "Resolve merge-base and HEAD SHAs" step to fail — github.base_ref is empty outside of pull_request events, making git fetch origin "" fatal.
  • P2: CMakeVersion: "4.0.1" and ExternalDataVersion: 5.4.5 are declared in env but never referenced; if CMake 4.x is genuinely required, an installation step is missing.

Confidence Score: 4/5

Safe to merge after fixing the workflow_dispatch / github.base_ref breakage.

One P1 defect: manually triggered runs fail immediately due to an empty github.base_ref. The cmake pin bump is clean and the overall workflow design is sound.

.github/workflows/perf-benchmark.yml — the workflow_dispatch + github.base_ref interaction and the unused env vars.

Important Files Changed

Filename Overview
.github/workflows/perf-benchmark.yml New CI workflow running ASV benchmarks on base vs HEAD; broken for workflow_dispatch due to empty github.base_ref, and has unused CMakeVersion/ExternalDataVersion env vars.
Modules/Remote/PerformanceBenchmarking.remote.cmake Single-line GIT_TAG bump to pin the commit that introduced the ASV harness (asv.conf.json) required by the new workflow.

Sequence Diagram

sequenceDiagram
    participant GHA as GitHub Actions
    participant Base as itk-base (merge-base)
    participant Head as itk-head (PR HEAD)
    participant ASV as ASV Harness

    GHA->>Base: checkout @ merge-base
    GHA->>Head: checkout @ PR HEAD (full history)
    GHA->>Base: cmake configure + build benchmark targets
    GHA->>Head: cmake configure + build benchmark targets
    Note over Head: PerformanceBenchmarking module populated with asv.conf.json
    GHA->>ASV: pip install harness (from itk-head)
    GHA->>ASV: asv machine --yes
    GHA->>ASV: asv run --quick @ merge-base SHA
    Note over ASV: continue-on-error: true
    GHA->>ASV: asv run --quick @ HEAD SHA
    GHA->>ASV: asv compare merge-base HEAD
    ASV-->>GHA: asv-compare.txt + results artifact
Loading

Reviews (1): Last reviewed commit: "COMP: Bump PerformanceBenchmarking pin t..." | Re-trigger Greptile

Comment on lines +56 to +62
HEAD_SHA=$(git rev-parse HEAD)
echo "merge_base=${MERGE_BASE}" >> "$GITHUB_OUTPUT"
echo "head=${HEAD_SHA}" >> "$GITHUB_OUTPUT"
echo "merge_base=${MERGE_BASE} head=${HEAD_SHA}"

- name: Checkout ITK at merge-base (shallow)
uses: actions/checkout@v4
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 github.base_ref is empty for workflow_dispatch

When the workflow is triggered manually via workflow_dispatch, github.base_ref is an empty string. Line 58 expands to git fetch origin "", which fails immediately with fatal: couldn't find remote ref. The entire asv-compare job becomes broken for manual runs.

You either need to guard this block behind a pull_request-only condition or supply a fallback (e.g. ${{ github.base_ref || 'main' }}):

Suggested change
HEAD_SHA=$(git rev-parse HEAD)
echo "merge_base=${MERGE_BASE}" >> "$GITHUB_OUTPUT"
echo "head=${HEAD_SHA}" >> "$GITHUB_OUTPUT"
echo "merge_base=${MERGE_BASE} head=${HEAD_SHA}"
- name: Checkout ITK at merge-base (shallow)
uses: actions/checkout@v4
git fetch origin "${{ github.base_ref || 'main' }}"
MERGE_BASE=$(git merge-base HEAD "origin/${{ github.base_ref || 'main' }}")

Comment on lines +21 to +22
CMakeVersion: "4.0.1"
# ASV-wired targets — see Modules/Remote/PerformanceBenchmarking's python/itk_perf_shim/registry.py.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Declared env vars are never consumed

ExternalDataVersion: 5.4.5 and CMakeVersion: "4.0.1" are defined at the workflow level but referenced nowhere in any run step. CMakeVersion is particularly suspect: Ubuntu 24.04's default apt ships CMake 3.28, not 4.0.1. If CMake 4.x is required (e.g. for FetchContent changes in 4.0), a step to install it via pip install cmake==4.0.1 or the jwlawson/actions-setup-cmake action is missing. If they're not needed, removing them avoids confusion.

Comment on lines +131 to +140
continue-on-error: true
env:
ITK_BENCHMARK_BIN: ${{ runner.temp }}/ITK-build-base/bin
ITK_BENCHMARK_DATA: ${{ runner.temp }}/ITK-build-base/ExternalData/Modules/Remote/PerformanceBenchmarking/examples/Data/Input
ITK_BENCHMARK_SCRATCH: ${{ runner.temp }}/asv-scratch-base
run: |
mkdir -p "${ITK_BENCHMARK_SCRATCH}"
asv run --machine gha-ubuntu-24.04 \
--set-commit-hash "${{ steps.sha.outputs.merge_base }}" --quick

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 --quick produces single-shot, noisy measurements

asv run --quick executes each benchmark exactly once. With no statistical averaging, the compare output will reflect OS scheduling noise as much as real code differences, making it hard to distinguish genuine regressions from measurement jitter. Consider omitting --quick (or replacing with --min-runs 3) so ASV collects a small sample and reports median-based results, which is the standard for reliable CI benchmarking.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Remotes Issues affecting the Remote module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants