search

graydon / bors

Integration robot for buildbot and github
424 stars 49 forks source link

Add an optimism parameter for rolling PRs together #23

Open chris-morgan opened 10 years ago

chris-morgan commented 10 years ago

Often PRs come through where one can be highly confident, or even certain, that the PR will not cause test failure. (e.g. documentation that is not tested, typos in comments, Vim syntax files for Rust.)

At the simplest level, optimism would just be a boolean flag: that if a developer is optimistic that a patch will land successfully, it can merge multiple optimistic patches at once.

A slightly higher level of implementation for this feature would be a parameter optimism=X, for values of X between 0 and 1 (inclusive).

optimism=1 means "there is no way this can genuinely go wrong" which would also mean that you could merge it with a non-optimistic patch and impute no failure to this patch in case of failure, and so immediately put it back in the approved queue rather than the bad queue. If it fails three times (arbitrary number), mark it as bad, just in case the optimism flag was erroneous. @retry isn't hard in such cases.

You could let optimism=0.99 merge only with other optimistic patches.

After that, you could get into moderately advanced statistics, if you wanted—probability that a patch will land, and then let bors calculate potential time savings by merging multiple patches at once (win if they land; lose if they don't, and then it can take a guess at which broke it and try a smaller set of patches). Artificial intelligence! Machine learning! Fun!

But for the moment, basic optimism checking would be a good feature to have.

bharrisau commented 10 years ago

Pretty straightforward until you get to reconstructing which PRs are being merged from the test HEAD.

Easiest way to go is checking if pulls[-1].is_optimistic(), and then running a different function (instead of try_advance) on pulls[state == approved and is_optimistic()] that merges as much as possible from oldest to newest. Then I got stuck on the reconstruction part.

reiddraper commented 10 years ago

What's the driving factor here? Why not simply test these patches even though you're 'optimistic' they'll pass? Are you constrained by build-resources? If you want, you can always manually merge things into the repository and skip bors altogether.

bharrisau commented 10 years ago

The idea is to merge multiple PRs together and build/test them all once. On 29/03/2014 12:56 am, "Reid Draper" notifications@github.com wrote:

What's the driving factor here? Why not simply test these patches even though you're 'optimistic' they'll pass? Are you constrained by build-resources? If you want, you can always manually merge things into the repository and skip bors altogether.

Reply to this email directly or view it on GitHubhttps://github.com/graydon/bors/issues/23#issuecomment-38942208 .

reiddraper commented 10 years ago

The idea is to merge multiple PRs together and build/test them all once.

Right, but why?

bharrisau commented 10 years ago

The idea was that one PR takes a very long time. If you end up with 5 PRs just fixing spelling mistakes in bors it would be handy to have bors merge them as one and save some time in the build queue. On 29/03/2014 5:19 am, "Reid Draper" notifications@github.com wrote:

The idea is to merge multiple PRs together and build/test them all once.

Right, but why?

Reply to this email directly or view it on GitHubhttps://github.com/graydon/bors/issues/23#issuecomment-38969166 .

reiddraper commented 10 years ago

The above suggestions seem rather complex. But I suppose I don't really care too much as long as the feature is either hidden behind a flag, or the repository owner gets to decide whether the PR is given different treatment, and not the pull-request author.

bharrisau commented 10 years ago

I believe the idea was for the reviewer to say r+ optimistic=1

chris-morgan commented 10 years ago

Jut like reviewer and priority (e.g. r=brson p=1) it's up to the reviewer to specify it. The PR author has no direct say in the matter.

The justification is that bors gets, from time to time, significant backlog, leading to much longer approval cycles and it taking a long time to catch up. Also that a lot of power is being wasted on things that simply don't need testing, though I dare say the cost savings will be fairly small. But think of the environment! ;-) On Mar 29, 2014 8:29 AM, "Reid Draper" notifications@github.com wrote:

The above suggestions seem rather complex. But I suppose I don't really care too much as long as the feature is either hidden behind a flag, or the repository owner gets to decide whether the PR is given different treatment, and not the pull-request author.

Reply to this email directly or view it on GitHubhttps://github.com/graydon/bors/issues/23#issuecomment-38970163 .

chris-morgan commented 9 years ago

I suppose #34 would be considered to supersede this idea?