Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: set up a CI job for automated benchmarking (Linux, Python 3.9) #129

Open
wants to merge 1 commit into
base: meson
Choose a base branch
from

Conversation

HarshCasper
Copy link

Description

This PR:

  • Implements automated benchmarking on CI
  • Uploads ASV-generated results, HTML files and logs as artifacts
  • Adds build caching using ccache for faster builds
  • Allows benchmark tests to be triggered using specific PR labels (Commented out as of now)

Copy link
Owner

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

This looks good, thanks @HarshCasper. Some localized comments. The benchmarks are quite slow, I'm wondering if it would be a good idea to merge them running a subset of all benchmarks to start with, to keep the time down. Maybe just the stats or sparse ones?

name: Benchmark CI

on:
pull_request:
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we want to run this job on PRs - it takes too long.

Copy link
Owner

Choose a reason for hiding this comment

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

Same for branches. I think the best would be a scheduled job, say once every 3 days.

Copy link
Author

@HarshCasper HarshCasper Feb 2, 2022

Choose a reason for hiding this comment

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

My idea here was to run this on specific PRs with a specific label, like run-benchmarks. It would be quite useful for significant PRs, especially around releases. Please let me know your thoughts, otherwise, I would just remove this.

Copy link
Owner

Choose a reason for hiding this comment

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

I like that idea. That's even better than regularly scheduled runs, very user-friendly.

Can you make the label ci/run-benchmarks? Then we can later extend the pattern with ci/other-job-to-run. PyTorch has this too, and it's quite nice.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I will make this change.

- master
# types: [labeled]

# workflow_dispatch:
Copy link
Owner

Choose a reason for hiding this comment

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

can be uncommented?

jobs:
benchmark:
name: Benchmark
# if: ${{ github.event.label.name == 'run_benchmark' && github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch' }}
Copy link
Owner

Choose a reason for hiding this comment

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

Uncomment, and add the protections for not running on a fork (see the linux_meson job)?

- name: Install Python packages
run: |
python -m pip install numpy setuptools wheel cython asv pytest pytest-xdist pybind11 pytest-xdist mpmath gmpy2 pythran ninja
python -m pip install git+https://github.com/mesonbuild/meson.git@master
Copy link
Owner

Choose a reason for hiding this comment

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

This can now be replaced by python -m pip install meson==0.61.1


- name: Setup build and install scipy
run: |
python dev.py -j 2 --build-only --werror
Copy link
Owner

Choose a reason for hiding this comment

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

Let's remove --werror here; it is best-practice to have that only in a single CI job (at least on the same platform).

pushd benchmarks

set -x
asv machine --yes
Copy link
Owner

Choose a reason for hiding this comment

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

This block could use a few comments on what the goal is; does it run a comparison, use baseline data, or ... ?

echo "Contender: ${GITHUB_SHA} (${{ github.event.pull_request.head.label }})"

ASV_OPTIONS="--split --show-stderr --factor $ASV_FACTOR"
git fetch origin meson
Copy link
Owner

Choose a reason for hiding this comment

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

ah, this is the branch to compare against

Copy link
Author

Choose a reason for hiding this comment

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

Should this be changed?

Copy link
Owner

Choose a reason for hiding this comment

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

No, you can leave it like this, I am keeping meson up to date and am trying out various Meson-specific build changes there. Just adding comments so this whole asv block becomes easier to understand would be great.

@rgommers rgommers force-pushed the meson branch 2 times, most recently from 0704d6a to 7deaf3e Compare February 3, 2022 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants