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.

Sign up for GitHub

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

Jump to bottom

CI: add benchmark workflow and script #250

Open

singh1203 wants to merge 7 commits into apache:main from singh1203:Benchmark-CI

Contributor

singh1203 commented Jan 15, 2025

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.


          ci: add benchmark workflow and script

f6bdce6

Signed-off-by: Saurabh Kumar Singh <[email protected]>

singh1203 marked this pull request as ready for review

January 15, 2025 14:26

singh1203 requested review from zeroshade, kou, assignUser and raulcd as code owners

January 15, 2025 14:26

zeroshade reviewed

View reviewed changes

Member

zeroshade left a comment

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

assignUser reviewed

View reviewed changes

.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


          refactor: (GH-apache#85) Changes in benchmark after review

8dc4aa8

Signed-off-by: Saurabh Kumar Singh <[email protected]>

singh1203 requested review from assignUser and zeroshade

January 16, 2025 11:13

singh1203 changed the title ~~ci: add benchmark workflow and script~~ CI: add benchmark workflow and script

singh1203 added 2 commits

January 21, 2025 18:15


          Merge branch 'apache:main' into Benchmark-CI

80dabba


          chore: Fix linting check

ceecbd2

Signed-off-by: Saurabh Kumar Singh <[email protected]>

Contributor Author

singh1203 commented Feb 1, 2025

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

kou reviewed

View reviewed changes

.github/workflows/benchmark.yml Outdated Show resolved Hide resolved

.github/workflows/benchmark.yml 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

.github/workflows/benchmark.yml Outdated

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

Member

kou Feb 1, 2025

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: |
    ...

Contributor Author

singh1203 Feb 1, 2025 •

edited

Loading

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

Member

zeroshade commented Feb 1, 2025

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 added 2 commits

February 1, 2025 14:06


          Merge branch 'apache:main' into Benchmark-CI

1ea6db4


          refactor: (GH-apache#85) Changes in benchmark after review-2

4dc951e

Signed-off-by: Saurabh Kumar Singh <[email protected]>

singh1203 force-pushed the Benchmark-CI branch from 94cabdd to 4dc951e Compare

February 1, 2025 10:08

singh1203 requested a review from kou

February 1, 2025 10:10

Contributor Author

singh1203 commented Feb 1, 2025

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

kou reviewed

View reviewed changes

.github/workflows/benchmark.yml Outdated Show resolved Hide resolved

.github/workflows/benchmark.yml Show resolved Hide resolved

singh1203 requested a review from kou

February 2, 2025 08:12

Contributor Author

singh1203 commented Feb 3, 2025

@kou IMO removing this

      - name: Install dependencies
        run: |
          apt-get update
          apt-get install -y build-essential git python3-pip

caused runner to fail here because of missing CA Certificates in the base Debian 12 environment.

singh1203 force-pushed the Benchmark-CI branch from 41cbba6 to 559a62d Compare

February 3, 2025 09:00

Member

kou commented Feb 3, 2025

Ah, could you try apt-get install -y ca-certificates then?

Contributor Author

singh1203 commented Feb 3, 2025 •

edited

Loading

Ah, could you try apt-get install -y ca-certificates then?

@kou Done

singh1203 force-pushed the Benchmark-CI branch 2 times, most recently from 6ed51f4 to 78da4c0 Compare

February 3, 2025 10:18

singh1203 force-pushed the Benchmark-CI branch from 78da4c0 to 1ce9cb0 Compare

February 3, 2025 16:38

This comment was marked as outdated.

Sign in to view


          refactor: (GH-apache#85) Changes in benchmark CI

4f7b37d

Signed-off-by: Saurabh Kumar Singh <[email protected]>

singh1203 force-pushed the Benchmark-CI branch from 1ce9cb0 to 4f7b37d Compare

February 4, 2025 06:51

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

Reviewers

raulcd Awaiting requested review from raulcd raulcd is a code owner

assignUser Awaiting requested review from assignUser assignUser is a code owner

zeroshade Awaiting requested review from zeroshade zeroshade is a code owner

kou Awaiting requested review from kou kou is a code owner

Labels

None yet