setup BenchmarkCI

[jw3126]

No description provided.

[codecov]

Codecov Report

Merging #148 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #148   +/-   ##
=======================================
  Coverage   91.59%   91.59%           
=======================================
  Files           9        9           
  Lines        1213     1213           
=======================================
  Hits         1111     1111           
  Misses        102      102

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46c0445...0f7b23a. Read the comment docs.

[jw3126]

@tkf sorry to bother you with this. BenchmarkCI gives me an error that Project.toml is dirty, see here:
https://github.com/JuliaImages/ImageFiltering.jl/pull/148/checks?check_run_id=403841198#step:6:124
Any idea how to fix this?

[jw3126]

What needs to be done, to get a nice report like this:
JuliaFolds/Transducers.jl#183 (comment)
Here is the log:
https://github.com/JuliaImages/ImageFiltering.jl/pull/148/checks?check_run_id=403945552#step:6:15

[tkf]

Hmm... I don't know GitHub API enough to explain what is going on. Maybe some kind of permission issue?

I wonder if you have to be a member to get the comment working, especially if you edit the workflow yml file in the same PR.

Can you open two PRs for https://github.com/tkf/BangBang.jl? One touches https://github.com/tkf/BangBang.jl/blob/master/.github/workflows/benchmark.yml and one does not?

[tkf]

OK, even the PR that does not touch the workflow file does not create a comment... https://github.com/tkf/BangBang.jl/pull/112/checks?check_run_id=403993952#step:6:15

[jw3126]

Ok that is unfortunate.

[tkf]

So it looks like the repository owner has to create a custom token to make the comment work in PRs from forks:

https://help.github.com/en/actions/automating-your-workflow-with-github-actions/authenticating-with-the-github_token#permissions-for-the-github_token

[tkf]

Some googling of the error message Resource not accessible by integration tells me that forks can't invoke APIs that require the write permission. I think it's just a limitation of the API. I think it might be possible to invoke the bot via explicit comment or something. But it's way beyond what I need ATM.

[johnnychen94]

Wow, this's a very nice job! Should I try to duplicate this PR to see if the comment thing works?

[tkf]

Yeah, let's see if it works.

[tkf]

Suggested change

	run: julia -e 'using BenchmarkCI; BenchmarkCI.judge(baseline = "HEAD")'
	run: julia -e 'using BenchmarkCI; BenchmarkCI.judge()'

baseline = "HEAD" should be removed before merging this.

[johnnychen94]

Good news is that #149 works

Github action has some limitations for PRs from other forks. For example, secrets are not passed to CI: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}. Hence if postjudge requires this secret to work, which I guess it does, then the functionality is expected to not work properly.

A workaround to this is:

• enable benchmark on push, and then
• create a comment to commit (on push only); to do this probably we need to pass an extra argument post_method to postjudge.

[tkf]

From my limited exploration into this, I think your conclusion correct, but there are minor details:

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

--- Using encrypted secrets in a workflow - Creating and using encrypted secrets - GitHub Help

But, the column "Access by forked repos" in the table shown in Permissions for the GITHUB_TOKEN - Authenticating with the GITHUB_TOKEN - GitHub Help only list "read." So the conclusion is the same: we can't post the comment via the pull_request event from forks.

I also agree that there has to be a workflow that is listening to something else (push, comment, cron, etc.) to make it work.

[jw3126]

Thanks a lot @tkf and @johnnychen94 for investigating this!

[johnnychen94]

I prefer to merge this when Benchmark.jl is registered in General so that we can add a compat section in benchmark/Project.toml (and make the workflow simpler)

I also agree that there has to be a workflow that is listening to something else (push, comment, cron, etc.) to make it work.

If this can be included in this PR, it's would be perfect. Even if it's not I think this PR is already a great proposal to be merged.

[johnnychen94]

From my first sight, it's not so intuitive where this file is used. I'm imagining something like this in the workflow

- julia -e 'using BenchmarkCI; BenchmarkCI.displayjudgement()'
+ julia --project=benchmark -e 'using BenchmarkCI; BenchmarkCI.displayjudgement()'

[tkf]

I prefer to merge this when Benchmark.jl is registered in General

Yeah, I think it makes sense. I'm probably going to register it as 0.1 (or maybe even as 0.0.1) as there are many things that can be improved. But since these works are required mainly for non-benchmark/Manifest.toml functionality that I don't use, I have to say I'm not super motivated to do these works ATM.

we can add a compat section in benchmark/Project.toml

I think it's actually better to do add"BenchmarkCI@0.1" or something like that in the workflow file. You don't need to use BenchmarkCI.jl inside the benchmark project; it is used from BenchmarkCI.jl. For example, BenchmarkCI.jl may depend on some versions of the packages that are incompatible with the dependencies of your package. We can safely separate environment in Julia to make it work even in such case. (PkgBenchmark.jl is the only exception that has to exist on "both sides" due to the way it is written.)

I also agree that there has to be a workflow that is listening to something else (push, comment, cron, etc.) to make it work.

If this can be included in this PR, it's would be perfect.

I don't think this can be done in this PR. I think you need a non-trivial piece of code to do this (interacting with GitHub API and Git repository). Also, I don't think I'm going to implement it soon as I don't need this ATM.

[tkf]

You can remove JULIA_LOAD_PATH now tkf#1 (comment). See #150.

[tkf]

FYI JuliaRegistries/General#8348

[jw3126]

Closing in favor of #150