Stream: t-compiler/cargo-bisect-rustc

Topic: Approach to rollup PR commit bisections


Chris Simpkins (Feb 20 2020 at 14:55, on Zulip):

This issue was raised by @pnkfelix during the t-compiler meeting today. The conversation begins here.

What is a good approach to bisect within rollup PR commits? It sounds like we do not have access to cached builds of rustc at the moment to bisect at a more granular level than the rollup PR commit level. The bisect-rustc tool therefore ends in some cases with a report suggesting that a rollup PR commit is the "regressed commit". This is valid, but not ideal. It would be helpful to drill down on more granular commit level changes within the rollup PR's during regression diagnostics.

lqd (Feb 20 2020 at 17:04, on Zulip):

I remember talking about this with simulacrum and IIRC we mentioned the possibility of maybe doing a try build per rolled-up PR, to have its CI artifacts available for bisection, which would at least help pinpointing the culprit PR. (At the time, this problem was maybe not as prevalent as it can be today)

Santiago Pastorino (Feb 20 2020 at 17:24, on Zulip):

the only way you have right now is to clone rustc and do the bisect locally in the repo

Santiago Pastorino (Feb 20 2020 at 17:24, on Zulip):

you would need to build rustc on every step so it will take a lot of time

pnkfelix (Feb 20 2020 at 17:44, on Zulip):

yeah. Part of the reason I suggested plugging in a script is that way you could do things like focus on stage 1 builds, and maybe even leverage incremental compile mode.

pnkfelix (Feb 20 2020 at 17:45, on Zulip):

but I suspect it would be better overall to do something like what @lqd suggested

Chris Simpkins (Feb 20 2020 at 18:38, on Zulip):

IIRC we mentioned the possibility of maybe doing a try build per rolled-up PR, to have their CI artifacts available for bisection, which would at least help pinpointing the culprit PR

@lqd @simulacrum A conversation that is worth continuing? I've gone through several bisect-rustc runs for open ICE issues and in my limited experience, it is common to come across a rollup PR commit as the regression point with this tool. IIRC, my last stage 1 build took 5+ mins. In the rollup PR that @pnkfelix mentioned during the meeting, there are 8 + 13 + 13 + 11 + 2 + 1 = 48 commits across the individual PR's in the rollup PR. At the individual PR level to even consider bisection over those commits, we have a O(n) search across each of the 6 individual PR I think? If that is a typical rollup PR size, we are talking about a significant amount of time to get to commit level granularity. And even individual PR level is significant if it involves rustc compiles for each test.

lqd (Feb 20 2020 at 18:44, on Zulip):

very much worth continuing imo indeed, esp with the new icebreakers cleanup crew, and the continued use of rollups, this will be more and more common I assume

pnkfelix (Feb 20 2020 at 18:47, on Zulip):

I did ask during the meeting if one can requrest try bulds on closed PR's. @simulacrum said one cannot currently do this. Should we consider adding that as a feature?

pnkfelix (Feb 20 2020 at 18:49, on Zulip):

(An alternative would be to add try build requests as part of the rollup process, either manually or have a bot do it. But I was figuring that it could make sense until a rollup has been identified before we request its components to be try-built.)

Santiago Pastorino (Feb 20 2020 at 18:49, on Zulip):

Chris Simpkins said:

IIRC we mentioned the possibility of maybe doing a try build per rolled-up PR, to have their CI artifacts available for bisection, which would at least help pinpointing the culprit PR

lqd simulacrum A conversation that is worth continuing? I've gone through several bisect-rustc runs for open ICE issues and in my limited experience, it is common to come across a rollup PR commit as the regression point with this tool. IIRC, my last stage 1 build took 5+ mins. In the rollup PR that pnkfelix mentioned during the meeting, there are 8 + 13 + 13 + 11 + 2 + 1 = 48 commits across the individual PR's in the rollup PR. To even consider bisection over those commits, we have a O(n) search across each of the 6 individual PR I think? If that is a typical rollup PR size, we are talking about a significant amount of time to get to commit level granularity. And even individual PR level is significant if it involves rustc compiles for each test.

I think you don't want commits, you want the PRs included in that rollup, so it will be less shas to cover

Santiago Pastorino (Feb 20 2020 at 18:49, on Zulip):

rollups do not include more than 10 (?) PRs

Santiago Pastorino (Feb 20 2020 at 18:50, on Zulip):

it's still a lot given the time it takes to build the compiler but it's not 42 :P

Chris Simpkins (Feb 20 2020 at 18:56, on Zulip):

it's still a lot given the time it takes to build the compiler but it's not 42 :P

I understand. I guess I am coming at this from the vantage of cleanup crew who try to offset some of the time investment required for diagnostics. You will need a kind soul to spend 30 - 60 mins building rustc for each issue where a rollup PR is identified by a previous run of cargo-bisect-rustc that took several minutes WITH cached builds. :slight_smile:

Santiago Pastorino (Feb 20 2020 at 18:59, on Zulip):

yep, it's not great :)

Chris Simpkins (Feb 20 2020 at 19:01, on Zulip):

can requrest try bulds on closed PR's

@pnkfelix Can you explain what a "try" build is?

pnkfelix (Feb 20 2020 at 19:01, on Zulip):

@Chris Simpkins sure.

pnkfelix (Feb 20 2020 at 19:02, on Zulip):

https://forge.rust-lang.org/infra/docs/rustc-ci.html#try-builds

Chris Simpkins (Feb 20 2020 at 19:02, on Zulip):

ty!

pnkfelix (Feb 20 2020 at 19:02, on Zulip):

that doc is a little lacking in details though

pnkfelix (Feb 20 2020 at 19:03, on Zulip):

basically, its a way to tell the CI, for a given Pull Request, "hey, make a full build off of this."

pnkfelix (Feb 20 2020 at 19:04, on Zulip):

we automatically run the CI in a more limited mode on every PR

pnkfelix (Feb 20 2020 at 19:04, on Zulip):

but a try build has more stuff in it

pnkfelix (Feb 20 2020 at 19:05, on Zulip):

you can request a try build by writing a comment "@bors try"

pnkfelix (Feb 20 2020 at 19:05, on Zulip):

see for example https://forge.rust-lang.org/infra/service-infrastructure.html?highlight=#perf--rust-timer

pnkfelix (Feb 20 2020 at 19:06, on Zulip):

which illustrates a way you can get data onto the perf.r-l.o website without committing to merging the PR to master

pnkfelix (Feb 20 2020 at 19:06, on Zulip):

by 1. requesting a try build and 2. scheduling that try build to have its own perf run.

pnkfelix (Feb 20 2020 at 19:07, on Zulip):

So the reason that try builds are relevant to this conversation is that, if I understand correctly, the CI keeps the artifacts from try builds around the same way it keeps around the artifacts from nightlies and each PR that bors merges.

pnkfelix (Feb 20 2020 at 19:08, on Zulip):

and therefore, we could leverage try when doing rollups to request that the CI produce the necessary builds that cargo-bisect-rustc would need to automatically download and run the builds.

Chris Simpkins (Feb 20 2020 at 19:49, on Zulip):

Got it. Now the comment that you cannot perform try builds on closed PR makes sense to me. It would be nice to avoid the need to execute try builds + caching on every roll up PR. Thanks for the detailed explanation

simulacrum (Feb 20 2020 at 20:53, on Zulip):

note that a try build is way less than a full CI run

simulacrum (Feb 20 2020 at 20:54, on Zulip):

a full CI run is like 60 builders at around 3 hours each, whereas try is around the same time but just 2 builders.

simulacrum (Feb 20 2020 at 20:54, on Zulip):

I think try on closed PRs is a bit hard as try by default rebases the PR onto master, so it's a bit unclear that you'd get the artifacts you want from that :)

simulacrum (Feb 20 2020 at 20:55, on Zulip):

also, bors today breaks on try after r+ so we added error handling and just flat out forbid it

pnkfelix (Feb 20 2020 at 20:56, on Zulip):

oh wow it breaks after r+?

pnkfelix (Feb 20 2020 at 20:56, on Zulip):

what if there's a subsequent r- and followon commits?

simulacrum (Feb 20 2020 at 20:57, on Zulip):

it's fine so long as you don't have the PR approved and try to try it

pnkfelix (Feb 20 2020 at 20:57, on Zulip):

also: just to correct my own thinking: How does the try build compare to the automatic build the CI does? (Travis Azure Github Actions) ?

simulacrum (Feb 20 2020 at 20:57, on Zulip):

Azure for now :)

simulacrum (Feb 20 2020 at 20:57, on Zulip):

a full CI run is like 60 builders at around 3 hours each, whereas try is around the same time but just 2 builders.

pnkfelix (Feb 20 2020 at 20:57, on Zulip):

I'm not speaking clearly

pnkfelix (Feb 20 2020 at 20:58, on Zulip):

I meant in comparison to the one that automatically happens on every PR that is opened

pnkfelix (Feb 20 2020 at 20:58, on Zulip):

is the only difference that the try build does a rebase onto master?

pnkfelix (Feb 20 2020 at 20:58, on Zulip):

I had thought that the try build still includes "more" stuff than the automagic build

simulacrum (Feb 20 2020 at 20:59, on Zulip):

ah, so the automatic one is basically a "test" build on some loosely "random" commit (IIRC, we use the default logic, so this is a merge commit with master at the time of the last push to the branch), whereas a try build is rebase onto master + dist build

simulacrum (Feb 20 2020 at 20:59, on Zulip):

from an infra perspective, the difference is that a try build has credentials to upload to S3, PR builds cannot for security reasons.

pnkfelix (Feb 20 2020 at 20:59, on Zulip):

ah. and also a "test" build has less stuff in it than a "dist" build, right?

pnkfelix (Feb 20 2020 at 21:00, on Zulip):

like you're referring to the difference between x.py test vs x.py dist, right?

simulacrum (Feb 20 2020 at 21:00, on Zulip):

basically, yes

simulacrum (Feb 20 2020 at 21:01, on Zulip):

I think we might not build tools in these dist builders today? not sure

simulacrum (Feb 20 2020 at 21:01, on Zulip):

hm, no I think we do

simulacrum (Feb 20 2020 at 21:01, on Zulip):

but yeah PR CI is more limited

pnkfelix (Feb 20 2020 at 21:12, on Zulip):

hmm. I guess one problem with the idea of using the try builds to generate the desired artifacts is that the try builds only make things for, what, just Linux?

pnkfelix (Feb 20 2020 at 21:12, on Zulip):

(at least, that's a problem for someone trying to use cargo-bisect-rustc on a Mac.)

simulacrum (Feb 20 2020 at 21:12, on Zulip):

just linux yeah

simulacrum (Feb 20 2020 at 21:13, on Zulip):

in theory docker or something

simulacrum (Feb 20 2020 at 21:13, on Zulip):

but I think for intra-rollup bisections requiring a linux computer is not too bad

simulacrum (Feb 20 2020 at 21:13, on Zulip):

maybe I'm wrong :)

pnkfelix (Feb 20 2020 at 21:14, on Zulip):

I don't mind it if it means we can get something rolling quickly.

simulacrum (Feb 20 2020 at 21:15, on Zulip):

I personally think that we might have enough capacity to just pre-run try as soon as something is r+ed

simulacrum (Feb 20 2020 at 21:15, on Zulip):

we basically need to fix the bug I think

simulacrum (Feb 20 2020 at 21:16, on Zulip):

(bors bug, that is)

pnkfelix (Feb 20 2020 at 21:16, on Zulip):

like run try on every r+ of a PR ?

simulacrum (Feb 20 2020 at 21:16, on Zulip):

hm, well, maybe not every

simulacrum (Feb 20 2020 at 21:16, on Zulip):

I guess ideally we'd restrict to just rollups

simulacrum (Feb 20 2020 at 21:17, on Zulip):

but we only know that after the fact...

pnkfelix (Feb 20 2020 at 21:17, on Zulip):

maybe every PR not marked with rollup=never

pnkfelix (Feb 20 2020 at 21:18, on Zulip):

heh. that would make for some great workflow docs. "Yeah we always include rollup=never because otherwise our CI $$$ is too high.")

simulacrum (Feb 20 2020 at 21:18, on Zulip):

One alternative -- maybe worth considering -- is that it's actually likely fairly fast to build PRs included in a rollup on decent hardware

simulacrum (Feb 20 2020 at 21:18, on Zulip):

or, rather, I take that back

pnkfelix (Feb 20 2020 at 21:18, on Zulip):

you mean locally?

simulacrum (Feb 20 2020 at 21:19, on Zulip):

yeah

pnkfelix (Feb 20 2020 at 21:19, on Zulip):

like checkout at the start commit, and leverage incremental compiles to get each one?

simulacrum (Feb 20 2020 at 21:19, on Zulip):

basically, yeah

simulacrum (Feb 20 2020 at 21:19, on Zulip):

that might be pretty slow still, not sure

simulacrum (Feb 20 2020 at 21:19, on Zulip):

I guess it depends on the rollup

pnkfelix (Feb 20 2020 at 21:19, on Zulip):

that then puts us back at the problem we didn't want to try to solve: having cargo-bisect-rustc do local builds

simulacrum (Feb 20 2020 at 21:19, on Zulip):

true

simulacrum (Feb 20 2020 at 21:20, on Zulip):

I think if we had docs for how to generate a PR post-facto on which we could run try builds that might be good too

pnkfelix (Feb 20 2020 at 21:20, on Zulip):

by the way, who has merge permissions on cargo-bisect-rustc ?

simulacrum (Feb 20 2020 at 21:20, on Zulip):

uh, I do, not sure if @Santiago Pastorino does, probably should

simulacrum (Feb 20 2020 at 21:20, on Zulip):

really anyone can merge :)

pnkfelix (Feb 20 2020 at 21:21, on Zulip):

maybe I can figure this out

pnkfelix (Feb 20 2020 at 21:21, on Zulip):

anyone ? I didn't see a merge button when I looked at a PR recently

simulacrum (Feb 20 2020 at 21:21, on Zulip):

well, I meant in a hypothetical sense

simulacrum (Feb 20 2020 at 21:21, on Zulip):

like, t-compiler knows enough

pnkfelix (Feb 20 2020 at 21:21, on Zulip):

oh oh I see

simulacrum (Feb 20 2020 at 21:22, on Zulip):

added t-compiler I think

pnkfelix (Feb 20 2020 at 21:23, on Zulip):

okay now I see the button, thanks

Chris Simpkins (Feb 25 2020 at 17:31, on Zulip):

I think if we had docs for how to generate a PR post-facto on which we could run try builds that might be good too

@simulacrum Would the workflow then be that a new PR is pushed when a roll up PR commit is identified as a regression, then once the try build is complete the bisect process can continue on the commits in that PR? What docs are needed?

simulacrum (Feb 25 2020 at 17:33, on Zulip):

Well, yes, but making that PR is relatively hard today, in that you need to move master back (a bunch of revert commits) and then gradually re-apply the rollup, running try each time. Note that the matter may not even work on each commit!

pnkfelix (Feb 25 2020 at 18:08, on Zulip):

what about doing a try build on the original PR's linked from the rollup? Those should all build, right?

pnkfelix (Feb 25 2020 at 18:09, on Zulip):

its not quite the same, since bugs can get injected in the process of merging, I admit ...

pnkfelix (Feb 25 2020 at 18:09, on Zulip):

but it would probably be effective in diagnosing the injection in 99% of the cases?

Santiago Pastorino (Feb 25 2020 at 18:13, on Zulip):

my understanding is that means that the PR would be merged on top of a completely different master to the one it was merged and by definition it would never apply cleanly

Santiago Pastorino (Feb 25 2020 at 18:14, on Zulip):

so you would need the master you had at the time the PR was merged

Santiago Pastorino (Feb 25 2020 at 18:14, on Zulip):

I guess this is why @simulacrum is saying that you need to move master back

pnkfelix (Feb 25 2020 at 18:14, on Zulip):

oh right, I forgot again that try merges to current master (and misinterpreted @simulacrum 's comment)

pnkfelix (Feb 25 2020 at 18:15, on Zulip):

but then ... is there a way we could ask try to do a build based on the master that the PR was connected to? I mean, this sounds straight-forward in principle...

Santiago Pastorino (Feb 25 2020 at 18:16, on Zulip):

yeah, one could fork a new branch from the commit that was on top at the time and merge there

Santiago Pastorino (Feb 25 2020 at 18:16, on Zulip):

unsure how the process work and if that is complicated for some reason

Santiago Pastorino (Feb 25 2020 at 18:17, on Zulip):

it won't be the current try

Santiago Pastorino (Feb 25 2020 at 18:17, on Zulip):

let's call it try-back

Santiago Pastorino (Feb 25 2020 at 18:18, on Zulip):

but would be nice if one could request that once cargo-bisect-rustc finds a regression in a rollup

Santiago Pastorino (Feb 25 2020 at 18:19, on Zulip):

so you do a "try-back" #PR1 #PR2 #PR3 and you get try-back-PR1, try-back-PR2, branches and the artifacts

Last update: Nov 25 2020 at 02:00UTC