search

paritytech / bench-bot

ISC License
9 stars 19 forks source link

Handle updates to pull request's commits while the benchmark is running and after it's done #44

Open shawntabrizi opened 3 years ago

shawntabrizi commented 3 years ago

when you run the benchmark bot, you must be very careful not to push any changes, no matter how trivial to the branch, or else your benchmark results will not get comitted.

I believe this is because we are missing a simple pull / merge operation before we call commit.

Of course the bot should not try to handle any merge conflicts on its own, but should certainly be able to commit changes to anything that has a clean merge.

joao-paulo-parity commented 3 years ago

The merging of master should happen in prepare branch which is executed before the benchmark is ran

https://github.com/paritytech/bench-bot/blob/ee7bf897d75d90dae0e3d7fcd8d68c85edd4fefc/bench.js#L103-L104

You're suggesting it should also happen again before the push is made?

https://github.com/paritytech/bench-bot/blob/ee7bf897d75d90dae0e3d7fcd8d68c85edd4fefc/bench.js#L374

when you run the benchmark bot, you must be very careful not to push any changes, no matter how trivial to the branch, or else your benchmark results will not get comitted

Is it because the history gets desynchronized with the commits on the remote ref? Having the bot pull the commits just before pushing is an idea to counteract that.

One note about the merging of master: I thought the idea of doing that before the commit was so that the results are representative of how the code is going to perform after merging into master. If we pull commits after running the benchmark, then its result is not taking into account the new changes; thus another way of going about this would be to pull again and restart the benchmark if some change is detected to the HEAD sha of the pull request.

shawntabrizi commented 3 years ago

@joao-paulo-parity yeah you are right. We dont want to commit the benchmarks if there is some underlying logic that has changed. But probably we want to detect there was a latest commit (if that is possible), and stop the current benchmark, and report all that.

Not high priority, but just a little better experience i think.

joao-paulo-parity commented 3 years ago

when you run the benchmark bot, you must be very careful not to push any changes, no matter how trivial to the branch, or else your benchmark results will not get comitted

FWIW we should be reporting failures due to pushes:

https://github.com/paritytech/bench-bot/blob/11b9240228c8608062b1c1cf9fe8aec70634fedd/bench.js#L431

That is what's expected to happen if you were to push changes while the bot is running a benchmark because, at the end, the bot will not be able to do git push with the weights since the branch has become outdated.

The following part is not covered, though:

and stop the current benchmark

So when the pull request's commits gets updated, we should either:

  1. Stop the current benchmark and report this fact
  2. Restart the current benchmark with the new commits
athei commented 3 years ago

So I would be in favor of just stopping the benchmark when there are new commits on the PR branch. This would be the most flexible approach.