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