JuliaImages / ImageFiltering.jl

Julia implementations of multidimensional array convolution and nonlinear stencil operations
Other
99 stars 49 forks source link

setup BenchmarkCI #148

Closed jw3126 closed 4 years ago

codecov[bot] commented 4 years ago

Codecov Report

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

Impacted file tree graph

@@           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 commented 4 years ago

@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 commented 4 years ago

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

tkf commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Ok that is unfortunate.

tkf commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

tkf commented 4 years ago

Yeah, let's see if it works.

johnnychen94 commented 4 years ago

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:

tkf commented 4 years ago

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 commented 4 years ago

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

tkf commented 4 years ago

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 commented 4 years ago

You can remove JULIA_LOAD_PATH now https://github.com/tkf/ImageFiltering.jl/pull/1#issuecomment-577966400. See #150.

tkf commented 4 years ago

FYI https://github.com/JuliaRegistries/General/pull/8348

jw3126 commented 4 years ago

Closing in favor of #150