github action to update benchmarks #106

JamesKyburz · 2019-10-06T14:32:15Z

Remove run directory (was added previously by mistake from benchmarks run)
Added run to gitignore
Fixed column error in markdown table generation (one too many --:)
Github Action to run benchmarks and update README.md
Allow benchmarks to work from CI without the need for interactive prompts

This PR is in response to #104

StarpTech · 2019-10-06T15:38:51Z

.github/workflows/benchmarks.yml

+        node-version: 10
+    - name: benchmarks
+      run: |
+        npm install


This is error prone and hard to manage. Put it in a separate file and let it be part of the linter.

@StarpTech I have removed the inline node code to run the benchmarks, this required a fix to benchmark-bench.js to avoid the prompts.

My idea is to move the entire code in a file and just run e.g scripts/bench.js

StarpTech · 2019-10-06T15:43:19Z

For more realistic benchmark and better isolation I would run each benchmark in a separate ci job. You could generate that file very easily.

Each job runs in a fresh instance of the virtual environment specified by runs-on.

https://help.github.com/en/articles/workflow-syntax-for-github-actions#jobs

JamesKyburz · 2019-10-07T08:56:35Z

For more realistic benchmark and better isolation I would run each benchmark in a separate ci job. You could generate that file very easily.

Each job runs in a fresh instance of the virtual environment specified by runs-on.

https://help.github.com/en/articles/workflow-syntax-for-github-actions#jobs

If the benchmarks were split into seperate jobs there would be no easy way to get the results of the entire run.

StarpTech · 2019-10-07T09:24:10Z

If the benchmarks were split into seperate jobs there would be no easy way to get the results of the entire run.

That's possible but definitly out of scope here. I'm fine 👍

StarpTech · 2019-10-07T09:25:55Z

benchmark-bench.js

+      filter: Number
+    }
+  ])
+  : (async () => {


async is not needed.

Is was that or Promise.resolve. Do you want me to change the inline function to a resolved promise?

If async is an option would you mind to rewrite it to simply use async/await without ternarys?

JamesKyburz · 2019-10-09T07:31:39Z

@StarpTech I replaced the inline async function with new Promise. I am not sure if this is better, however I feel changing to async/await should be another pull request.

StarpTech · 2019-10-09T08:04:03Z

.github/workflows/benchmarks.yml

+        node-version: 10
+    - name: benchmarks
+      run: |
+        npm install


My idea is to move the entire code in a file and just run e.g scripts/bench.js

JamesKyburz · 2019-10-09T11:38:10Z

@StarpTech Splitting the inline script in benchmarks.yml to a seperate file breaks the secrets interpolation stuff (${{ secrets }})

also it won't help with linting as it is a mix of bash and inline node.

StarpTech · 2019-10-09T13:42:08Z

@JamesKyburz I got it. That's my suggestion to the benchmark-bench.js https://gist.github.com/StarpTech/958edf3920e13b08671998bf18683114

JamesKyburz · 2019-10-14T06:04:45Z

@StarpTech async/await is now used.

StarpTech

LGTM

Eomm · 2019-11-03T15:04:01Z

.github/workflows/benchmarks.yml

+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v1
+    - name: Use Node.js ${{ matrix.node-version }}


Suggested change

- name: Use Node.js ${{ matrix.node-version }}

- name: Use Node.js 12

Eomm · 2019-11-03T15:04:23Z

.github/workflows/benchmarks.yml

+    - name: Use Node.js ${{ matrix.node-version }}
+      uses: actions/setup-node@v1
+      with:
+        node-version: 10


I would use the last LTS node

Suggested change

node-version: 10

node-version: 12

Agreed, I used 10 because it was the LTS version when the PR was submitted.

JamesKyburz · 2019-11-04T08:43:42Z

updated to use node 12.

mcollina · 2019-11-11T04:13:44Z

.github/workflows/benchmarks.yml

+      run: |
+        npm install
+        npm start y 100 10 40
+        npm start y 100 10 40


Why are there run twice?

Sorry that was my interpretation of README.md two rounds; one to warm-up, one to measure ;)

I gather that I should only run them once, and this is taken care of by autocannon?

I have updated the PR to only run the benchmarks once.

.github/workflows/benchmarks.yml

mcollina

LGTM

JamesKyburz · 2020-01-03T14:36:07Z

The secrets GITHUB_USERNAME and GITHUB_REPO_TOKEN need to be added before the git hub action will work. They give the action access to create the scheduled commit message.

Can we merge this?

Eomm · 2020-01-03T20:50:28Z

I don't have powa here 😅

Kindly ping to @mcollina

mcollina · 2020-01-05T16:57:09Z

Is GITHUB_TOKEN https://help.github.com/en/actions/automating-your-workflow-with-github-actions/authenticating-with-the-github_token provided by github action? I would prefer to have this not depending on a token provided by secrets.

I think GITHUB_USERNAME can be hardcoded, as the URL of this repo is fixed.

JamesKyburz · 2020-01-05T21:25:42Z

@mcollina Yes that's the correct documentation link. The token has to be created & then added as a secret. The token value is needed to create the git commit message in the github action.

jsumners · 2020-01-05T21:26:01Z

Is the GitHub Actions default runner environment consistent enough for this task? If we are going to automate benchmark running I would think it'd be desirable that the runner system be consistent between every run.

JamesKyburz · 2020-01-05T21:39:30Z

@jsumners I am sure that it's a better solution than we currently have and should hopefully be consistent enough. The same virtual hardware is used each time.

mcollina · 2020-01-06T12:00:05Z

The doc states (at the very beginning):

GitHub automatically creates a GITHUB_TOKEN secret to use in your workflow. You can use the GITHUB_TOKEN to authenticate in a workflow run.

Why should this created manually?

The reason I’m asking is that setting this up will require create a dummy/bot user with write permission to this repo. If possible, I try to minimize yet another account to manage.

JamesKyburz · 2020-01-06T14:16:48Z

@mcollina I forgot that when I first tested this I read the documentation where it mentioned the github token had read only access in a forked repository (where I was testing from :))

Good news!

The default token works, I have just verified the change so we need no extra secrets, so ready for merge :)

mcollina · 2020-01-06T19:00:52Z

@delvedor @jsumners any objections?

jsumners · 2020-01-06T19:29:45Z

None.

JamesKyburz · 2020-01-13T20:36:36Z

FYI I noticed that PR #108 has made similar changes to benchmark-bench.js to also allow it it work non interactively.

mcollina · 2020-01-20T12:28:41Z

Landed, let's see how it goes.

JamesKyburz · 2020-01-20T14:29:04Z

@mcollina nice!

JamesKyburz · 2020-02-01T13:17:12Z

@mcollina The github action ran this morning and updated the readme :)

mcollina · 2020-02-01T15:45:10Z

🎉 Il 1 feb 2020, 14:17 +0100, James Kyburz <[email protected]>, ha scritto:

…

@mcollina The github action ran this morning and updated the readme :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

Eomm · 2020-02-01T22:54:30Z

.gitignore

@@ -56,4 +56,3 @@ yarn.lock
 package-lock.json

 # benchmark results
-results


I notice this right now: why we removed from gitignore? I don't find an answer

StarpTech suggested changes Oct 6, 2019

View reviewed changes

StarpTech suggested changes Oct 7, 2019

View reviewed changes

JamesKyburz force-pushed the feature/github-action-benchmarks branch from 2f16052 to 330848b Compare October 8, 2019 05:58

StarpTech suggested changes Oct 9, 2019

View reviewed changes

JamesKyburz force-pushed the feature/github-action-benchmarks branch from 16d39be to 815a4a1 Compare October 14, 2019 06:02

StarpTech approved these changes Oct 15, 2019

View reviewed changes

JamesKyburz force-pushed the feature/github-action-benchmarks branch from 815a4a1 to c28cca8 Compare October 29, 2019 13:00

Eomm reviewed Nov 3, 2019

View reviewed changes

Eomm approved these changes Nov 10, 2019

View reviewed changes

mcollina reviewed Nov 11, 2019

View reviewed changes

JamesKyburz added 2 commits November 11, 2019 07:44

github action to update benchmarks

9dc8d90

update to node 12 lts version

2713d58

JamesKyburz force-pushed the feature/github-action-benchmarks branch from 79475c0 to 2713d58 Compare November 11, 2019 06:48

mcollina approved these changes Nov 12, 2019

View reviewed changes

update github action to work without extra secrets

94c8745

mcollina merged commit 8a3b704 into fastify:master Jan 20, 2020

mcollina mentioned this pull request Jan 20, 2020

bench: allow users to run benchmarks non-interactively #108

Open

4 tasks

Eomm reviewed Feb 1, 2020

View reviewed changes

Eomm mentioned this pull request Mar 20, 2020

update ci to node 12 #107

Closed

tinawng mentioned this pull request Sep 18, 2020

Add: results folder into .gitignore #123

Merged

	- name: Use Node.js ${{ matrix.node-version }}
	- name: Use Node.js 12

github action to update benchmarks #106

github action to update benchmarks #106

Conversation

JamesKyburz commented Oct 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarpTech commented Oct 6, 2019 • edited Loading

JamesKyburz commented Oct 7, 2019

StarpTech commented Oct 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesKyburz commented Oct 9, 2019

Choose a reason for hiding this comment

JamesKyburz commented Oct 9, 2019

StarpTech commented Oct 9, 2019

JamesKyburz commented Oct 14, 2019

StarpTech 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

JamesKyburz commented Nov 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

JamesKyburz commented Jan 3, 2020

Eomm commented Jan 3, 2020

mcollina commented Jan 5, 2020

JamesKyburz commented Jan 5, 2020

jsumners commented Jan 5, 2020

JamesKyburz commented Jan 5, 2020

mcollina commented Jan 6, 2020 • edited Loading

JamesKyburz commented Jan 6, 2020 • edited Loading

mcollina commented Jan 6, 2020

jsumners commented Jan 6, 2020

JamesKyburz commented Jan 13, 2020

mcollina commented Jan 20, 2020

JamesKyburz commented Jan 20, 2020

JamesKyburz commented Feb 1, 2020

mcollina commented Feb 1, 2020 via email

Choose a reason for hiding this comment

JamesKyburz commented Oct 6, 2019 •

edited

Loading

StarpTech commented Oct 6, 2019 •

edited

Loading

mcollina commented Jan 6, 2020 •

edited

Loading

JamesKyburz commented Jan 6, 2020 •

edited

Loading