update Rollups process

[matthiaskrgr]

cc @compiler-errors @jieyouxu @GuillaumeGomez
Rendered

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) some time within the next two weeks.

[GuillaumeGomez]

I'm not sure including "iffy" PRs is a good idea in rollups. I personally tend to never includ them.

[compiler-errors]

That's kinda the whole reason we have "iffy". If you never include them, then they may as well be marked "never".

[GuillaumeGomez]

Fair point.

[jieyouxu]

I personally consider iffy PRs to be the whole point of rollups, tbh. Some PRs are clearly rollup=never if you are almost certain they will fail or if they have perf implications, but some of the iffy PRs are for the "it can fail, but we're not too sure" cases: it's great if it passes full CI, in which case we saved a bunch of time. It's also great if it fails full CI, in which case the failure time is amortized.

[jieyouxu]

I adjusted this to say

You can include one or two iffy PRs to amortize the cost of testing full CI.

[GuillaumeGomez]

Apart from my comment, looks good to me. Thanks for the doc improvement!

[compiler-errors]

That's kinda the whole reason we have "iffy". If you never include them, then they may as well be marked "never".

[compiler-errors]

Suggested change

	Don't try to make mega-rollups (15-20 PRs that merge half or more of the entire queue all at once) to keep the number of perf or bug regressions per merge as low as possible and keep potential regressions [bisectable].
	Limit the size of rollups, even if the queue is backed up -- large rollups run the risk of failing or merge conflicts, and smaller rollups keep potential regressions [bisectable]. On average, rollups are <N> PRs large, often varying from <N - M> to <N + M> depending on the number of `rollup=always` PRs that can be included.

[compiler-errors]

choose some value for N and M.

[Noratrieb]

note that thanks to unrolled builds, bisection can be done within a roll-up and cargo bisect rustc does that

[jieyouxu]

note that thanks to unrolled builds, bisection can be done within a roll-up and cargo bisect rustc does that

But I think there is some time limit for rollup artifacts right? Something like 6 months?

[steffahn]

There is¹ a time limit for all artifacts; for old regressions we only get nightly granularity AFAIR

Footnotes

1. or at least was, my source on this is only personal experience in bisection of old bugs, no idea where the current limits can be found out for certain ↩

[jieyouxu]

Adopted this wording. I picked ~7 as the average size, with a usual bound of [5, 10] PRs.

Limit the size of rollups, even if the queue is backed up. Large rollups run the risk of failing
or merge conflicts, and smaller rollups keep potential regressions [bisectable]. On average, rollups
are 7 PRs large, often varying from 5 to 10 depending on the number of rollup=always PRs that can
be included.

[tgross35]

I have been thinking that what the existing documentation says (rollups always p=5) might be better than p=pr_count that is usually done. There are a lot of cases where 7 PRs get rolled up with p=7 and sit in the queue for a number of hours, only to be leapfrogged by a p=8 rollup that contains much newer PRs. Seems to have a tendency to break FIFO.

[matthiaskrgr]

Yes that's why I added the "Rollups should not overlap" rule.

[jieyouxu]

we can just ping the contributors who are assigning rollup p>5 (I've done this a couple of times myself) and just point them to p=5 being the good default and having that respect queue order. It helps to elaborate on the reasoning for choice of p=5 and not arbitrary p=N where N is the number of PRs rolled up.

If we update the docs here and then still notice contributors assigning p > 5 to rollups, we can just ping them and point them to the update docs here.

[jieyouxu]

(Thanks for the edits)

Just wanted to say, we should make these rollup advice as accessible and useful to onboard other contributors w/ r+ perms who may want to help to do rollups too.

[jieyouxu]

(Somewhat unrelated, but also I feel like the p=??? advice is at times too vague to be useful, at least when I read some of the forge docs related to p=??? advice)

[jieyouxu]

Yeah, I feel like the tool/subtree updates should receive same priority as rollups and all pinned at p=5 exactly, good point.

[matthiaskrgr]

mmh imo it makes sense to prioritise subtree/module updates over rollups since doing a tool sync is often more complicated than rebasing PRs

[jieyouxu]

Just, they should receive a consistent p between themselves, but otherwise it feels whatever to me.

[jieyouxu]

Adjusted this to say sth like

Tools and subtree syncs should use p=5, like rollups, so they interleave between rollups, as tools and subtree syncs are typically more tricky to fix than rebasing PRs.

[matthiaskrgr]

I'm not sure anymore if there is an easy way to generalize 😅

some subtrees are mostly self-contained and risk free to rollup, other (like clippy) are prone to creating merge conflicts and changes in tests, some, like cargo may also have test failures which the pre merge ci cannot catch.

some subtrees are always rollup=never'd by the author so they cannot be rolled up

[jieyouxu]

@matthiaskrgr sorry to also ping you here, do you intend to update this PR, or do you mind if I take over?

[apiraino]

I think it's fine by now if you @jieyouxu push this forward (when you have time, ofc). Let's not forget to co-author the patch.

@rustbot assign @jieyouxu

[jieyouxu]

r? @Mark-Simulacrum (release)

[matthiaskrgr]

ah I kinda forgot about this

I'll have another look this week.

@jieyouxu my general vibe is that I would rather avoid using --author/ committing something in someone elses name etc, and would rather have one commit by person A and another commit by person B that may or may not change/revert some content of the previous commit/ builds on top of it, just so that its clear what changed and who is the author of what. :)

edit: do you have a diff from mine to your changes? since you force pushed, the diff is all messed up :/

[jieyouxu]

edit: do you have a diff from mine to your changes? since you force pushed, the diff is all messed up :/

Sorry, I don't, I think I unintentionally squashed...

[matthiaskrgr]

Hm, I'm afraid something got messed up. when I try to reconstruct the diff between us, I can only see whitespace change, I also did not find the quote you mentioned here
#775 (comment)

edit: nevermind, I think I got it :D

[compiler-errors]

I object to this line you added. Please remove it.

Never have we ever used try jobs for this purpose, so I'm tempted to say you're just creating new procedures to justify closing other people's rollups.

[compiler-errors]

if you wanna soften in the tone to encourage people to test rollups independently if they are at a high risk to merge, that's great, but you're asking for people to do extra work because of your personal preference of rollup size.

[matthiaskrgr]

I have made (what I deem) "high-risk" rollups tested via try jobs in the past just to weed out failures and I found it pretty useful because then I don't unnecessarily block the queue.
What exactly are you concerned about?

[compiler-errors]

Yeah, which is why I said:

if you wanna soften in the tone to encourage people to test rollups independently if they are at a high risk to merge, that's great, but you're asking for people to do extra work because of your personal preference of rollup size.

What I'm afraid of is you codifying justification to close other people's rollups because you think they're too large and pointing at this justification you added because of it.

Again, I think you can just discourage people from making large PRs unless they know what they're getting themselves into (and perhaps mention that it's usually unnecessary), and point them to try jobs if they really want to get things testing.

If you want to go the extra mile, it would be nice if you asked people who are working on bors2 for better automation to test "dist-like jobs" for this exact scenario, instead of just saying that the user needs to do it manually. Even better if we got CI to detect rollups and run a larger test suite on them so that they get weeded out before the next auto merge is done.

[compiler-errors]

One more good idea if you really care about overlapping rollups not being made, at least not accidentally or without justification, is asking the bors2 folks if they could detect and mark PRs that are currently part of rollups that are currently mergeable + accepted. Then people could make disjoint rollups more easily; right now it's a pretty large ask to do. Paired with CI-level testing it would be very beneficial to weed out bad PRs and improve our merge rate in general.

[compiler-errors]

This doesn't really make sense. I think there are plenty of cases where it's valid to make a rollup that's larger than the ones that are queued and give it higher priority.

I think the tone should be softened to urge the rollup maker to consider rollups that are already queued, but any hard suggestions here I'm afraid are just going to be used to justify closing people's rollups for no good reason.

[matthiaskrgr]

To me it sounds a bit like you're saying "I want to be able invalidate other peoples rollups at any time" (by overwriting them with my larger rollup) but at the same time I "I don't want to have my own rollup closed for doing so".

In what situations do you deem it critical to override someones rollup?
Why not just wait it out until it is merged and then make another rollup of your own?

I think the rollup-oneupping has created a lot of unnecessary friction in the past, so its best to just avoid it altogether 🙂

[compiler-errors]

To me it sounds a bit like you're saying "I want to be able invalidate other peoples rollups at any time" (by overwriting them with my larger rollup) but at the same time I "I don't want to have my own rollup closed for doing so".

First of all, claiming that this is "invalidating" other people's rollups or calling this "oneupping" makes it seem like this is a competition or race or something, which it definitely is not 😸

What I'm saying is that these should be guidelines for well-informed contributors to understand how to use rollups to help things get merged, not strict rules that must be followed to the tee. After all, I think it's very useful to give individuals discretion that is weighed against different things like what's queued and how busy the week is going and factors like that.

In what situations do you deem it critical to override someones rollup?

Well, I think it's a bit excessive to call it "critical", but there are (as of writing this comment I think) 23 rollup=always PRs in the queue right now. I think testing all of them in parallel with a couple of rollup=maybe PRs outweighs the benefit of testing a 8 PR rollup regardless of what it contains. That's what I mean by discretion above.

Of course there is always a risk that large rollups fail to merge, but I think it's unnecessary to have to wait for other smaller rollups to merge first just because they were created first. If they conflict with the larger rollup and that one does successfully merge, then great! We all win, since we're all just trying to get our PRs merged after all 😃 And if it doesn't, then we fall back to the smaller rollup.

I'm very much of the opinion that this is a non-problem because this is obviously not something that is happening all the time.