Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an optimism parameter for rolling PRs together #23

Open
chris-morgan opened this issue Feb 26, 2014 · 9 comments
Open

Add an optimism parameter for rolling PRs together #23

chris-morgan opened this issue Feb 26, 2014 · 9 comments

Comments

@chris-morgan
Copy link

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

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

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

The idea is to merge multiple PRs together and build/test them all once.
On 29/03/2014 12:56 am, "Reid Draper" [email protected] 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//issues/23#issuecomment-38942208
.

@reiddraper
Copy link
Contributor

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

Right, but why?

@bharrisau
Copy link

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" [email protected] 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//issues/23#issuecomment-38969166
.

@reiddraper
Copy link
Contributor

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

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

@chris-morgan
Copy link
Author

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" [email protected] 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//issues/23#issuecomment-38970163
.

@chris-morgan
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants