Add benchmarking workflow using ASV #74

light2802 · 2023-06-15T17:39:27Z

📝 Description

Type: 🚀 feature | 🚦 testing | 🎢 infrastructure
Add benchmarking workflow for gauging stardis performance using ASV. The workflow can push the benchmarking results to another repo to be viewed later. The workflow compares the performance for the last five commits for now, but this can be changed later.

Only one benchmarking test has been added, which runs the run_stardis function. Next PRs will add code for benchmarking other internal parts, but that requires some change in stardis code.

📌 Resources

ASV resources: https://asv.readthedocs.io/en/stable/using.html

🚦 Testing

How did you test these changes?

Testing pipeline
The workflow is successfully tested to run on my local repo

☑️ Checklist

I requested two reviewers for this pull request
I updated the documentation according to my changes
I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

benchmarks/h_minus_cross_section_wbr.dat

stardis_env3.yml

.github/workflows/benchmarks.yml

asv.conf.json

atharva-2001

Hi @light2802 this looks good. I have a few minor comments. Can you address them so we can get this merged?

.github/workflows/benchmarks.yml

atharva-2001 · 2023-07-21T09:00:08Z

.github/workflows/benchmarks.yml

+      - name: Create results html on dest repo
+        if: github.event_name != 'pull_request_target'
+        continue-on-error: true
+        uses: cpina/github-action-push-to-another-repository@main
+        env:
+          API_TOKEN_GITHUB: ${{ secrets.BOT_TOKEN }}
+        with:
+          source-directory: ".asv/html"
+          destination-github-username: "tardis-sn"
+          destination-repository-name: "stardis-benchmarks"
+          user.email: tardis.sn.bot@gmail.com
+          target-branch: main
+
+      - name: Push misc data to dest repo
+        if: github.event_name != 'pull_request_target'
+        continue-on-error: true
+        uses: cpina/github-action-push-to-another-repository@main
+        env:
+          API_TOKEN_GITHUB: ${{ secrets.BOT_TOKEN }}
+        with:
+          source-directory: ".asv/results"
+          destination-github-username: "tardis-sn"
+          destination-repository-name: "stardis-benchmarks"
+          user.email: tardis.sn.bot@gmail.com
+          target-branch: main


I don't think you need to do this separately- once the env files are deleted, all that remains is HTML and the results directory right? You should be able to send it all at once. I don't think having the name "misc" is a good idea.

I am pushing the html to the root directory as it will automatically deploy the webpage using github pages and we can see the results instantaneously

I don't find this organised. I like the approach of not needing a GitHub Actions workflow, however.
But is that the only reason? Can you let me know why you're pushing it to the root directory?
But the workflow I used is pretty simple. I don't think it should be named "misc". I think it should be named something like "asv-results" at least.

Either way, the approaches should be identical for STARDIS and TARDIS benchmarks.

GitHub pages builds the site from the root directory so I want the .asv/html in root. As for the .asv/results I don't mind naming it to something else

But if someone wants to find results from the benchmarks repo they won't be able to find it easily right?

You also see the results instantly with the workflow I made- I think it looks cleaner, right? Don't you think so?

Oh so you made another workflow there. Yeah that's fine too

Done. Thanks

.github/workflows/benchmarks.yml

codecov · 2023-07-21T09:47:48Z

Codecov Report

Merging #74 (9d17653) into main (fb5f079) will not change coverage.
The diff coverage is n/a.

❗ Current head 9d17653 differs from pull request most recent head 08084d6. Consider uploading reports for the commit 08084d6 to get more accurate results

@@           Coverage Diff           @@
##             main      #74   +/-   ##
=======================================
  Coverage   34.97%   34.97%           
=======================================
  Files          17       17           
  Lines         629      629           
=======================================
  Hits          220      220           
  Misses        409      409

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

* Bechmark using ASV * Delete env files only if generated * Fix posting comment * Try addding token as env var * Put env var at correct place * fix benchmark comment bug * Fix typo * Add compare steps names and fix checkout step * Push asv results to benchmarks results repo as it is * Fix workflow name --------- Co-authored-by: Joshua Shields <54691495+jvshields@users.noreply.github.com>

light2802 force-pushed the main branch 5 times, most recently from 2f176a8 to 6312074 Compare June 15, 2023 19:45

atharva-2001 self-requested a review June 16, 2023 14:21

light2802 force-pushed the main branch from 6312074 to 4530857 Compare June 16, 2023 15:34

light2802 marked this pull request as ready for review June 16, 2023 15:35

light2802 force-pushed the main branch from 4530857 to cf28884 Compare June 16, 2023 15:39

light2802 changed the title ~~[WIP] Add benchmarking workflow using ASV~~ Add benchmarking workflow using ASV Jun 16, 2023

light2802 force-pushed the main branch from cf28884 to 3bb3776 Compare June 16, 2023 15:44

jvshields self-requested a review June 16, 2023 15:52

isaacgsmith requested changes Jun 16, 2023

View reviewed changes

benchmarks/h_minus_cross_section_wbr.dat Outdated Show resolved Hide resolved

stardis_env3.yml Outdated Show resolved Hide resolved

stardis_env3.yml Outdated Show resolved Hide resolved

light2802 force-pushed the main branch 5 times, most recently from acf7941 to af7baa3 Compare June 20, 2023 16:15

wkerzendorf reviewed Jun 20, 2023

View reviewed changes

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

wkerzendorf reviewed Jun 20, 2023

View reviewed changes

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

wkerzendorf reviewed Jun 20, 2023

View reviewed changes

asv.conf.json Outdated Show resolved Hide resolved

light2802 force-pushed the main branch 9 times, most recently from e814485 to 7a8ed99 Compare June 20, 2023 18:09

light2802 requested a review from atharva-2001 July 12, 2023 05:36

light2802 added the benchmarks Trigger benchmarks to run on this pr label Jul 12, 2023

light2802 mentioned this pull request Jul 12, 2023

Benchmark internal STARDIS functions #85

Merged

6 tasks

light2802 requested a review from wkerzendorf July 12, 2023 15:55

jvshields previously approved these changes Jul 13, 2023

View reviewed changes

sonachitchyan requested a review from isaacgsmith July 13, 2023 17:03

atharva-2001 requested changes Jul 21, 2023

View reviewed changes

light2802 dismissed jvshields’s stale review via bc5750c July 21, 2023 09:45

light2802 force-pushed the main branch from bc5750c to 4106df6 Compare July 21, 2023 09:46

light2802 requested a review from atharva-2001 July 21, 2023 09:47

light2802 added 10 commits July 24, 2023 16:58

Bechmark using ASV

ec06e23

Delete env files only if generated

2ce15be

Fix posting comment

4361964

Try addding token as env var

2cc24ae

Put env var at correct place

18d16a5

fix benchmark comment bug

ef9d654

Fix typo

3111664

Add compare steps names and fix checkout step

6fa7c39

Push asv results to benchmarks results repo as it is

48223de

Fix workflow name

dc618fa

light2802 force-pushed the main branch from 34f193b to dc618fa Compare July 24, 2023 11:31

atharva-2001 approved these changes Aug 1, 2023

View reviewed changes

jvshields approved these changes Aug 1, 2023

View reviewed changes

Merge branch 'main' into main

08084d6

jvshields requested review from isaacgsmith and removed request for isaacgsmith August 1, 2023 15:55

isaacgsmith approved these changes Aug 1, 2023

View reviewed changes

isaacgsmith merged commit 1d9daf7 into tardis-sn:main Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add benchmarking workflow using ASV #74

Add benchmarking workflow using ASV #74

light2802 commented Jun 15, 2023 •

edited

Loading

atharva-2001 left a comment

atharva-2001 Jul 21, 2023

light2802 Jul 21, 2023

atharva-2001 Jul 21, 2023

light2802 Jul 21, 2023

atharva-2001 Jul 24, 2023

atharva-2001 Jul 24, 2023

light2802 Jul 24, 2023

light2802 Jul 24, 2023

codecov bot commented Jul 21, 2023 •

edited

Loading

Add benchmarking workflow using ASV #74

Add benchmarking workflow using ASV #74

Conversation

light2802 commented Jun 15, 2023 • edited Loading

📝 Description

📌 Resources

🚦 Testing

☑️ Checklist

atharva-2001 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 21, 2023 • edited Loading

Codecov Report

light2802 commented Jun 15, 2023 •

edited

Loading

codecov bot commented Jul 21, 2023 •

edited

Loading