Skip to content
This repository has been archived by the owner on Jun 30, 2022. It is now read-only.

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

Open
shawntabrizi opened this issue Jul 1, 2021 · 4 comments

Comments

@shawntabrizi
Copy link
Member

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
Copy link
Contributor

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

bench-bot/bench.js

Lines 103 to 104 in ee7bf89

// Fetch and merge master
var { error, stderr } = benchContext.runTask(`git pull origin ${baseBranch}`, `Merging branch ${baseBranch}`);

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

benchContext.runTask(`git push pr HEAD`);

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
Copy link
Member Author

@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
Copy link
Contributor

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:

extraInfo = `NOTE: Failed to push commits to repository: ${error.toString().replace(token, "{secret}", "g")}`

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

@joao-paulo-parity joao-paulo-parity changed the title bench bot should handle basic merge before commit Handle updates to pull request's commits while the benchmark is running and after it's done Sep 9, 2021
@athei
Copy link
Member

athei commented Oct 22, 2021

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.

@Vovke Vovke added duplicate This issue or pull request already exists and removed duplicate This issue or pull request already exists labels Apr 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants