Trigger Benchmark on comment & benchmark vs upstream

assignUser commented 2 years ago

Closes #111. Enables using touchstone with PR comments "/benchmark" and uses the upstream base branch on PRs from forks into the upstream repo, with an option to benchmark against the upstream branch even in PRs within a fork (e.g. for testing).

Needs some documentation.

lorenzwalthert commented 2 years ago

Thanks @assignUser for this work. 👍 I will look into it Next week. Also, did you habe a chance to test it with some repos? I created https://github.com/lorenzwalthert/touchstone.test once for this we can still use…

lorenzwalthert commented 2 years ago

I unfortunately am not too familiar with the programming languages used in this PR :D. However, if you add some docs, I can try it out on {styler}. In particular, we should document:

How to enable/disable automatic benchmarking vs /benchmark? Probably quite promiently, e.g. even in the README.md
Also, I am not sure I understand the new option force_upstream. What's the use case? So say I am in my fork, making a PR from patch-1 to master, but upstream has default branch main, what happens?

lorenzwalthert commented 2 years ago

in the mean time, I fixed the CI with #113 and rebased.

assignUser commented 2 years ago

How to enable:

add the changes in inst/touchstone-receive.yml into you existing file
comment out the pull_request part and un-comment the issue_comment line
comment "/benchmark" on a PR to run

Use case `force_upstream`:

In a situation as depicted below (awesome new gh feature btw :D), the issue with current touchstone is that touchstone would benchmark FEAT against HEAD~1 which could give misleading results as the upstream branch is already further at HEAD.

With the new changes on a PR into the Upstream repository touchstone will benchmark FEAT against HEAD, with force_upstream you can force this behavior for PR within your repository. This is of course only a sensible option if you PR against the default/same branch you would PR against in the upstream repo, if that diverges the setting should be left off.

graph TD
b-(( ))---a1(( ))
subgraph Fork-feature

a1(( ))---b1(( ))
b1(( ))---c1(( FEAT))

end

subgraph Fork-main

a-(( ))---b-(( ))
b-(( ))---c-(( ))
c-(( ))---d-(("HEAD~1"))
end

subgraph Upstream-main

a(( ))---b(( ))
b(( ))---c(( ))
c(( ))---d(( ))
d(( ))---f((HEAD))
end

lorenzwalthert commented 2 years ago

Thanks @assignUser 😄.

I assume most people just choose one mode, i.e. either every PR or manual trigger with /benchmark. So I think we can add an argument in use_touchstone() to govern which variant gets added to .github/workflows/. My suggestion: Have one source file in this package's inst/ with both in it, and mark the lines specific to one variant.
```
on: 
pull_request:
issue_comment: #VARIANT: AUTOCOMMENT
types: ["created", "edited"] #VARIANT: AUTOCOMMENT
```

jobs: prepare: runs-on: ubuntu-latest if: github.event_name == 'pull_request' || ( #VARIANT: MANUALCOMMENT github.event.issue.pull_request && #VARIANT: MANUALCOMMENT startsWith(github.event.comment.body, '/benchmark') #VARIANT: MANUALCOMMENT ) #VARIANT: MANUALCOMMENT


Then, Instead of copying the workflow file as we do [here](https://github.com/lorenzwalthert/touchstone/blob/main/R/use.R#L54), we `readLines()`, add comments to the lines we don't want active and then write it to the user's project.

* In the docs for `use_touchstone()`, we explain that switching between the modes can be done manually or just re-running `use_touchstone(force = TRUE)` and specify the option differently from the first run.

What do you think?

lorenzwalthert commented 2 years ago

@assignUser what do you think of my review comments?

assignUser commented 2 years ago

@lorenzwalthert sorry for the delay, yes I'll implement them soon(tm)

assignUser commented 2 years ago

It probably needs some more documentation but functionality should be all there. I added use_touchstone_workflows so the workflows can be updated without having to replace and roll back the script.R etc..

njtierney commented 2 years ago

Hi there @lorenzwalthert and @assignUser - I was wondering if I need to do anything to trigger a benchmark check? I'm hoping to do that here:

https://github.com/njtierney/conmat/pull/85

But I'm also not sure if I've just not got the right branch of touchstone setup?

lorenzwalthert commented 2 years ago

@njtierney to me, it seems up and running... Let's wait for the results 😀

lorenzwalthert commented 2 years ago

@njtierney in styler, we recently removed the token in the RSPM URL, as it timed out with the token. See https://github.com/r-lib/styler/blob/main/touchstone/config.json. If your run times out, maybe that's also something you could do in your default branch, then rebase your PR branch on that and see if it helps.

assignUser commented 1 year ago

I will rebase after #120 to fix the ci

assignUser commented 1 year ago

@lorenzwalthert rebased and ready for review :)

lorenzwalthert commented 1 year ago

LGTM, thanks a lot @assignUser.

lorenzwalthert / touchstone