Improve policy around rollups and priority

[Manishearth]

Ages ago we decided on a rollup policy that made sure higher priority PRs did not get shafted in the rollup process:

It arose out of frustration from compiler contributors that some PRs were getting deprioritized repeatedly in the rollup process.

The problem is, we now use p=1 to mark rollup=never PRs, so this policy isn't as helpful.

The main thing I want to propose is this: we consider 1 <= p <= 5 to be "skippable" priorities mostly set by people sheriffing the queue. If you want your PR to be high priority, use a number greater than 5. The policy above applies in the same way, except that rollup authors can ignore these priority numbers.

What do folks think about making that change and communicating it to the teams?

I have some larger policy changes I'd like to jot down around rollups, but I want to start with this.

cc @rust-lang/compiler @Mark-Simulacrum @Dylan-DPC @pietroalbini

[oli-obk]

If you want your PR to be high priority, use a number greater than 5.

bitrotty PRs are the thing where I mostly encounter priorities. Should these then have p=5 denoting them as don't run anything else before them except for rollups and actual high priority items like emergency fixes and whatnot?

[Dylan-DPC-zz]

Generally when bumping priorities i follow:
3+ => urgent prs, backports, toolchain and nominations that are preferable to be merged as soon as possible
1-2 => normally rollup=never prs and prs that have remained in the queue for a while i.e. in cases where you want to reorder the queue

[Manishearth]

@oli-obk absolutely, yes

anything that is urgent/bitrotty should be p>5 IMO

[ehuss]

Can we write down some guidelines for setting priority and post it on the forge? I sometimes tweak priorities based on my own unwritten rules, and I assume my rules are different from other people's rules. I think it would be good if we were all working with the same understanding.

[spastorino]

Some time ago I've also documented on forge an idea of giving some priority to P-critical and P-high fixes. If we were to document or re-define some stuff I think it's very important to give maximum possible priority at least to P-critical fixes if not also P-high.

[Manishearth]

@ehuss Yeah this would be great. I also plan to write a document about rollups for the forge

[nagisa]

I have encountered that in certain cases numbers are not the best or most self-documenting way to implement certain features. Priorities, now that they have more than just a single dimension of consideration, feels like one of those cases.

In particular, to me it seems like it would probably make more sense to go away with the one-dimensionality of priorities and use annotations that denote ordering more precisely. For instance one could have annotation like after=#12345 (runs after #12345 has got its bors time) for rollups.

That said, it would involve additional engineering and design effort, so I don’t have have much against just reshuffling of how we use priorities, just wanted to throw out my 2c.

[Manishearth]

I think numbers are the easiest way to communicate across a large organization. It's far easier to just say "hey i need this to land quickly, and I care this much about it" than it is to do the other thing in a way that consistently makes sense across orgs.

An after annotation is solving a different set of problems.

[Manishearth]

Number priorities are also incredibly useful from a sheriffing perspective. I did file an issue (rust-lang/homu#93) to add an annotation that would solve one of the main uses of priorities, but still

[Manishearth]

Thinking about it more; this policy is less necessary if we sort PRs by rollupishness

[Manishearth]

Opened a tooling PR that may obviate the need of this: rust-lang/homu#94

[Dylan-DPC-zz]

I have encountered that in certain cases numbers are not the best or most self-documenting way to implement certain features. Priorities, now that they have more than just a single dimension of consideration, feels like one of those cases.
In particular, to me it seems like it would probably make more sense to go away with the one-dimensionality of priorities and use annotations that denote ordering more precisely. For instance one could have annotation like after=#12345 (runs after #12345 has got its bors time) for rollups.

I agree with this, often I need to check why a PR is prioritised, and numbers don't convey much

[nikomatsakis]

I don't have a strong opinion about the best way to handle things, I trust y'all to hash it out, but I would request one thing: a forge page that lays out exactly what protocol I should follow.

I'll be honest that I can't really remember very well what I'm supposed to do w/ rollup today, for example. I know I should sometimes write rollup=never and I understand that some people write rollup=always, but I don't know how different that is from rollup, etc.

I think if there were some relatively simple directions for me to follow it would be very helpful, as well as criteria I should use to decide the rollup setting. (For example, it seems worth highlighting rollup=never for things that are likely to have perf impact.)

(Also, if there is a forge page, uh, sorry. 😁)

[Dylan-DPC-zz]

(Also, if there is a forge page, uh, sorry)

There isn't one that's generic to all teams. The only place it is mentioned from a reviewer respective in on the libs team

[Manishearth]

@nikomatsakis The docs are on https://buildbot2.rust-lang.org/homu/ , though I just noticed that they miss an important point, bors rollup is equivalent to bors rollup=always

[ehuss]

Mark wrote some guidance for rollups here: https://internals.rust-lang.org/t/rollup-never-always-guidance/11827

I think that would be good to capture in a page that also gives guidance on setting priorities (whether on the rollups page or somewhere else).

[Manishearth]

If rollup=iffy happens I'd update it with "use rollup=never for changes that should absolutely never be rolled up" and "rollup=iffy" for the categories he listed. We can make a forge page. I'd actually make two, one with guidance for rollupers.

[Manishearth]

As of now the queue autosorts rollup=never at the top, and also has rollup=iffy. The priority question is no longer required here. We should still document stuff.

[nikomatsakis]

I think the docs should live on forge... or at least have links from forge.

[nikomatsakis]

But also the docs that @Manishearth listed to are pretty brief, and they don't give (e.g.) much guidance as to what the heck rollup=iffy means or when I should use rollup=always vs rollup=never. The internals thread is pretty good for that, though!

[Manishearth]

I'm going to close this as it isn't necessary with the homu changes, but we should improve docs!

[ehuss]

@Manishearth will you open a PR on the forge to capture those docs? If not, I can probably try to start something.

[Manishearth]

@ehuss If you can start something that would be great!

[ehuss]

I posted a PR at rust-lang/rust-forge#402 to offer some guidance on rollup/priorities. Would appreciate if anyone following this issue could check it out and provide feedback.