epinowcast
Change benchmarking to be called by a comment
At the moment we run benchmarking on our CI with every commit. This leads to quite a bit of comment spam and also wasted computing as we don't always need benchmarks. @sbfnk updated benchmarking in EpiNow2 to only run when a benchmark comment is included in a commit for the same reasons.
On the other hand, benchmarking is very useful in some cases and having it automatically ticking over has identified a number of problem areas before they have truly bitten.
I think we should update so that benchmarking is optional and called by either a commit message or a comment in a PR. The reason for the second feature is that often benchmarks are helpful for a reviewer and it would be nice to be able to trigger them without needing to either make a commit yourself or getting the contributor to add something to their commit messages.
Sounds obviously preferable to me? I do get the desire for completely automated use, but this seems automated enough. About the only nice-to-have tweak would be to require a final benchmark pre merge.
Aside: need to document how / why to trigger benchmarking in CONTRIBUTING.
About the only nice-to-have tweak would be to require a final benchmark pre merge.
Agreed with this part. It can be sometimes difficult to predict that a given change will impact performance. Detecting unexpected changes is one nice benefit of this workflow. It would be a shame to lose it by only running it when the submitter or maintainer assumes it can have an impact.
Also agreed it doesn't necessarily need to run on every push though.
. It can be sometimes difficult to predict that a given change will impact performance.
totally agree and this is the nice bit of the current setup which makes me think its worth the waste/spam.
About the only nice-to-have tweak would be to require a final benchmark pre merge.
This seems like it would be a great half-way house but I have no clue how to implement.
You can make this check a required check (through a branch protection rule) with a manual trigger. Happy to jump on a quick zoom call any time to point out the specific boxes to check in the settings.
It will not be fully automated in the sense that a specific comment will still be required to merge but there will be a helpful visual reminder that this needs to be done before merging.
You can make this check a required check (through a branch protection rule) with a manual trigger.
Ah yes good idea. I know how to do that so no worries but I had no idea that would work with something that needed to be called manually.
@sbfnk we likely want to do this for EpiNow2 as well.
So if we start using a merge queue then we could have benchmarking be an automated part of that and not happen automatically in a PR. One downside is that we wouldn't get the nice comment but that doesn't seem like a major issue. Another is that if for any reason we wanted to merge code that introduced a benchmark slow down (for example a new feature in preprocessing) then we would have to somehow skip the merge queue requirement which I currently don't know how to do.
Ah it looks like you can still bypass merge queue requirements so its doable (but only by skipping all requirements)