update Rollups process #775

matthiaskrgr · 2024-11-03T19:07:11Z

cc @compiler-errors @jieyouxu @GuillaumeGomez
Rendered

rustbot · 2024-11-03T19:07:15Z

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 · 2024-11-03T19:11:31Z

src/release/rollups.md


 ## Selecting Pull Requests

-The queue is sorted by rollup status. In general, a good rollup includes one or two `iffy` PRs (if available), a bunch of `maybe` (unmarked) PRs, and a large pile of `always` PRs. A rollup should never include `rollup=never` PRs.
+The queue is sorted by rollup status. In general, a good rollup contains a bunch of `maybe` (unmarked) PRs, and a large pile of `always` PRs. You can include one or two `iffy` PRs if you are confident that they will pass. 


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

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

Fair point.

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.

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!

src/release/rollups.md

compiler-errors · 2024-11-03T19:12:52Z

src/release/rollups.md


 ## Selecting Pull Requests

-The queue is sorted by rollup status. In general, a good rollup includes one or two `iffy` PRs (if available), a bunch of `maybe` (unmarked) PRs, and a large pile of `always` PRs. A rollup should never include `rollup=never` PRs.
+The queue is sorted by rollup status. In general, a good rollup contains a bunch of `maybe` (unmarked) PRs, and a large pile of `always` PRs. You can include one or two `iffy` PRs if you are confident that they will pass. 


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

compiler-errors · 2024-11-03T19:30:37Z

src/release/rollups.md


-The actual absolute size of the rollup can depend based on experience, people new to making rollups might start with including 1 `iffy`, 4 `maybe`s, and 5 `always`s, but more experienced people might even make a rollup of 1-2 `iffy`s, 8 `maybe`s, and 10 `always`s! Massive rollups are rarely needed, but as your intuition grows you'll get better at judging risk when including PRs in a rollup.
+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].


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.

choose some value for N and M.

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

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?

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

Footnotes

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 ↩

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 · 2024-11-03T23:37:16Z

src/release/rollups.md

    ```
    @bors r+ rollup=never p=5
    ````
-
+   where 5 is the number of PRs contained in your rollup.


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.

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

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.

(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.

(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)

Maybe we should have something like
tool/subtree updates p=5 and rollups p=5 as default, so that we still have some headroom to 4 or 3 things if needed

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

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

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

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.

jieyouxu · 2025-02-12T11:13:56Z

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

apiraino · 2025-06-20T09:15:02Z

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 · 2025-06-22T03:05:25Z

r? @Mark-Simulacrum (release)

Co-authored-by: Matthias Krüger <matthias.krueger@famsik.de> Co-authored-by: Jieyou Xu <jieyouxu@outlook.com>

rustbot assigned Mark-Simulacrum Nov 3, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2024

GuillaumeGomez reviewed Nov 3, 2024

View reviewed changes

compiler-errors reviewed Nov 3, 2024

View reviewed changes

tgross35 reviewed Nov 3, 2024

View reviewed changes

rustbot assigned jieyouxu and unassigned Mark-Simulacrum Jun 20, 2025

rustbot assigned Mark-Simulacrum and unassigned jieyouxu Jun 22, 2025

Update rollup process

ae43065

Co-authored-by: Matthias Krüger <matthias.krueger@famsik.de> Co-authored-by: Jieyou Xu <jieyouxu@outlook.com>

jieyouxu force-pushed the uproll branch from dc98abc to ae43065 Compare June 22, 2025 03:09


		The actual absolute size of the rollup can depend based on experience, people new to making rollups might start with including 1 `iffy`, 4 `maybe`s, and 5 `always`s, but more experienced people might even make a rollup of 1-2 `iffy`s, 8 `maybe`s, and 10 `always`s! Massive rollups are rarely needed, but as your intuition grows you'll get better at judging risk when including PRs in a rollup.
		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].

	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.

update Rollups process #775

Are you sure you want to change the base?

update Rollups process #775

Uh oh!

Conversation

matthiaskrgr commented Nov 3, 2024

Uh oh!

rustbot commented Nov 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Noratrieb Nov 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steffahn Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthiaskrgr Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Nov 4, 2024 • edited by oli-obk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Feb 12, 2025

Uh oh!

apiraino commented Jun 20, 2025

Uh oh!

jieyouxu commented Jun 22, 2025

Uh oh!

Uh oh!

jieyouxu Nov 4, 2024 •

edited

Loading

Noratrieb Nov 3, 2024 •

edited

Loading

steffahn Feb 17, 2025 •

edited

Loading

matthiaskrgr Nov 4, 2024 •

edited

Loading

jieyouxu Nov 4, 2024 •

edited by oli-obk

Loading