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: add benchmark workflow and script #250

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

singh1203
Copy link
Contributor

Fixes: #85

What changes are included in this PR?

Added Benchmark CI using Debian as runtime environment

Are these changes tested?

Yes, I have tested the changes locally.

Signed-off-by: Saurabh Kumar Singh <[email protected]>
@singh1203 singh1203 marked this pull request as ready for review January 15, 2025 14:26
Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

Just a few nitpicks but we also need to add the CONBENCH_EMAIL and CONBENCH_PASS secrets to this repo

ci/scripts/bench.sh Outdated Show resolved Hide resolved
ci/scripts/bench_adapt.py Outdated Show resolved Hide resolved
ci/scripts/bench.sh Outdated Show resolved Hide resolved
.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
@singh1203 singh1203 changed the title ci: add benchmark workflow and script CI: add benchmark workflow and script Jan 16, 2025
@singh1203
Copy link
Contributor Author

Hey @zeroshade, it would be good if you could share your thoughts here.
Thank you!
cc - @assignUser

.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
.github/workflows/benchmark.yml Show resolved Hide resolved
.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
ci/scripts/bench_adapt.py Show resolved Hide resolved
Comment on lines 53 to 64
- name: Run Benchmarks
if: >-
success() && github.event_name == 'push' && github.repository == 'apache/arrow-go' && github.ref_name == 'main'
env:
CONBENCH_URL: https://conbench.ursa.dev
CONBENCH_EMAIL: ${{ secrets.CONBENCH_EMAIL }}
CONBENCH_PASSWORD: ${{ secrets.CONBENCH_PASS }}
CONBENCH_REF: ${{ github.ref_name }}
CONBENCH_MACHINE_INFO_NAME: ${{ matrix.arch }}-debian-12
run: |
python3 -m pip install benchadapt@git+https://github.com/conbench/conbench.git@main#subdirectory=benchadapt/python
python3 ci/scripts/go_bench_adapt.py
Copy link
Member

Choose a reason for hiding this comment

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

How about running only ci/scripts/bench.sh --json on pull request (and push on fork too?) for testing?

- name: Run Benchmarks
  if: github.event_name == 'pull_request'
  run: |
    ci/scripts/bench.sh --json
- name: Run Benchamrks
  if: >-
    success() && github.event_name == 'push' && github.repository == 'apache/arrow-go' && github.ref_name == 'main'
  env:
    ...
  run: |
    ...

If it's possible, the following may be better:

- name: Run benchmarks
  run: |
    ci/scripts/bench.sh --json
- name: Upload results
  if: github.event_name == 'push' && github.repository == 'apache/arrow-go' && github.ref_name == 'main'
  env:
    ...
  run: |
    ...

Copy link
Contributor Author

@singh1203 singh1203 Feb 1, 2025

Choose a reason for hiding this comment

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

Well, I went with:

- name: Run benchmarks
  run: |
    ci/scripts/bench.sh --json
- name: Upload results
  if: github.event_name == 'push' && github.repository == 'apache/arrow-go' && github.ref_name == 'main'
  env:
    ...
  run: |
    ...

IMO it makes more sense to have a trigger push event on the main branch and the previous review from @assignUser here suggests that benchmark dumps are quite large and no pull request based triggering.

.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
@zeroshade
Copy link
Member

Hey @singh1203 ! I'm at FOSDEM25 this weekend, I'll try to give this a look but I might not get to it until Monday/Tuesday.

@singh1203
Copy link
Contributor Author

@kou Please let me know if I missed something, guidance will be appreciated.
Thank you!

Comment on lines +20 to +24
push:
branches: [main]
paths:
- ".github/workflows/benchmark.yml"
- "ci/scripts/bench_adapt.py"
Copy link
Member

Choose a reason for hiding this comment

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

If we have paths for push, this workflow isn't executed for most commits on main. Because most commits don't change these files.

How about using paths only for pull_request?

Suggested change
push:
branches: [main]
paths:
- ".github/workflows/benchmark.yml"
- "ci/scripts/bench_adapt.py"
push:
branches: [main]
pull_request:
paths:
- ".github/workflows/benchmark.yml"
- "ci/scripts/bench_adapt.py"

If we have pull_request.paths, we can test this workflow only in PRs that change these files such as this PR.

go-version: ${{ matrix.go }}
cache: true
cache-dependency-path: go.sum
- name: Run Benchmarks
Copy link
Member

Choose a reason for hiding this comment

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

ci/scripts/bench_adapt.py will run ci/scripts/bench.sh. So we don't need this with push.

Suggested change
- name: Run Benchmarks
- name: Run Benchmarks
if: github.event_name != 'push'

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.

Add benchmark CI: Debian
4 participants