-
Notifications
You must be signed in to change notification settings - Fork 49
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.
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
setup BenchmarkCI #148
setup BenchmarkCI #148
Conversation
Codecov Report
@@ 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.
|
@tkf sorry to bother you with this. BenchmarkCI gives me an error that Project.toml is dirty, see here: |
Co-Authored-By: Takafumi Arakaki <aka.tkf@gmail.com>
Co-Authored-By: Takafumi Arakaki <aka.tkf@gmail.com>
Co-Authored-By: Takafumi Arakaki <aka.tkf@gmail.com>
What needs to be done, to get a nice report like this: |
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? |
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 |
Ok that is unfortunate. |
|
Some googling of the error message |
Wow, this's a very nice job! Should I try to duplicate this PR to see if the comment thing works? |
Yeah, let's see if it works. |
- name: Install dependencies | ||
run: julia -e 'using Pkg; pkg"add PkgBenchmark https://github.com/tkf/BenchmarkCI.jl"' | ||
- name: Run benchmarks | ||
run: julia -e 'using BenchmarkCI; BenchmarkCI.judge(baseline = "HEAD")' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run: julia -e 'using BenchmarkCI; BenchmarkCI.judge(baseline = "HEAD")' | |
run: julia -e 'using BenchmarkCI; BenchmarkCI.judge()' |
baseline = "HEAD"
should be removed before merging this.
Good news is that #149 works Github action has some limitations for PRs from other forks. For example, secrets are not passed to CI: A workaround to this is:
|
From my limited exploration into this, I think your conclusion correct, but there are minor details:
But, the column "Access by forked repos" in the table shown in Permissions for the I also agree that there has to be a workflow that is listening to something else (push, comment, cron, etc.) to make it work. |
Co-Authored-By: Takafumi Arakaki <aka.tkf@gmail.com>
Co-Authored-By: Takafumi Arakaki <aka.tkf@gmail.com>
Thanks a lot @tkf and @johnnychen94 for investigating this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -0,0 +1,4 @@ | |||
[deps] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()'
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-
I think it's actually better to do
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. |
You can remove |
Closing in favor of #150 |
* setup BenchmarkCI * fix benchmarks * remove Manifest.toml from benchmark * fix * fix * Update .github/workflows/benchmark.yml Co-Authored-By: Takafumi Arakaki <aka.tkf@gmail.com> * Update .github/workflows/benchmark.yml Co-Authored-By: Takafumi Arakaki <aka.tkf@gmail.com> * Update .github/workflows/benchmark.yml Co-Authored-By: Takafumi Arakaki <aka.tkf@gmail.com> * Update benchmark/Project.toml Co-Authored-By: Takafumi Arakaki <aka.tkf@gmail.com> * Update .github/workflows/benchmark.yml Co-Authored-By: Takafumi Arakaki <aka.tkf@gmail.com> * Remove JULIA_LOAD_PATH from .github/workflows/benchmark.yml * Use BenchmarkCI@0.1 * Update .github/workflows/benchmark.yml Co-Authored-By: Jan Weidner <jw3126@gmail.com> Co-authored-by: Jan Weidner <jw3126@gmail.com>
No description provided.