Did we start reviewing PRs slower?

I’ve noticed that a couple of once-cell related PRs on rust-lang/rust seem to didn’t get any attention for quite some time, getting stuck in waiting-on-review even without initial review:

What happened there? I think historically we were faster with reviews, did something change?

3 Likes

I think this is mostly specific to the T-libs review queue. The 2 PRs you linked have assigned reviewers with 98 and 94 reviews currently assigned to them, despite both actively doing reviews. In general it's been the case for years that the demand for library reviews has been higher than supply of people willing to do library reviews on volunteer time with any sort of consistency – not for lack of people trying it, and then quickly quitting. As far as I know currently nobody is paid to do these reviews because (ᴀ) companies don't tend to see this as high enough leverage (in contrast to compiler work, for example, or in contrast to sending library PRs on paid time which obviously exacerbates things) and (ʙ) even the one reviewer who formerly had a paid arrangement burnt themself out over it.

21 Likes

To ask the obvious thing, are foundation folks aware of the problem?

6 Likes

FWIW @thomcc (not sure of their IRLO handle) and me also had a (private) chat about this recently. It's frustrating from a contributor perspective but I am sure it is just as frustrating for the team, and ultimately having more people in the reviewer pool is the only sustainable solution. I am not quite sure what to do in the mean time. I am particularly worried how this will look for first-time contributors when their PR goes without any feedback for more than a month.

For my own PRs, usually after about 2 weeks I re-roll the reviewer dice via r? libs to get a different team member assigned. That tends to help, since the review latency varies a lot across the people in that auto-review group.

3 Likes

And maybe the other obvious question is, is there a (somewhat) well-defined path for people who would potentially like to help out to become a libs team reviewer?

6 Likes

Anyone can help review any PR. You don't need to be on any team or have any special permission. Any help is hugely beneficial. And if there's someone consistently providing useful reviews, then they'd be a good candidate to offer r+ rights to.

7 Likes

Yeah, for sure. I try to stay on top of it, but it's hard, even though I am a fast reviewer. That said, a lot of the review backlog ends up being proposed new APIs, which now need to go through a separate process (although, then they get hit by the libs-api review backlog...)

As far as I know currently nobody is paid to do these reviews because (ᴀ) companies don't tend to see this as high enough leverage (in contrast to compiler work, for example, or in contrast to sending library PRs on paid time which obviously exacerbates things)

Yeah, this is a real bummer. For a while I tried to make a dent in the overall backlog (and I still manage to stay on top of my review queue), but it's definitely hard to manage when I'm doing it entirely in my spare time (any company wanting to reach out to me about this should feel free).

1 Like

Maybe companies can somehow be incentivised to pay people to review libs PRs?

1 Like

The most straightforward way to do this is to somehow prioritize PRs that come from people/organizations that are also active reviewers.

Then maybe the auto assign algorithm should take this latency variability into account? (But don't overshoot, or else it may lead to burning out the faster reviewers)

2 Likes

The canonical rust-lang tool could use the GitHub API to analyse the (open) PRs and provide some guidance where some help is needed.

Or maybe reviews should be assigned to a team (all team members) instead of an eagerly chosen member of the team?

4 Likes

While I'm otherwise taking this week off, I am still following IRLO!

I wouldn't mind reviewing PRs myself, and I believe I have the requisite knowledge for a number of them. I'm not really certain what the process is to become a reviewer, though.

Start reviewing PRs via Github's interfaces you do feel confident about. Eventually, you may be given powers to tell bors that your review is meaningful enough for merging.

I really do want to encourage people to add comments or a full review to any PRs that are in areas they have some experience with.

Open source is at its absolute best when the "many eyes" theory is actually practised, and visibly so.

6 Likes

Would it be feasible to add some stats on open PRs to the TWiR emails in the Calls for Participation section?

3 Likes

I have given an informal review for a few PRs before, but this has been limited to when people have explicitly asked for a review after delays and the assigned reviewer had not responded. However, I won't be spending my time actively reviewing an unknown number of pull requests unless there's an endgame in sight. It doesn't save time — an official review still has to be performed, and they should not be taking my review into account at all in doing so.

Performing reviews that have near zero effect on anything in the hopes of being noticed at some point is hardly a process. It is almost entirely a waste of time. I am happy to do so when it helps speed things up, but requiring someone else to review it is needless duplication. My name is presumably recognizable to people on the relevant teams, and I have submitted large patches to rustc that have needed minimal changes. That has to mean something.

5 Likes

I think you underestimate yourself. Getting feedback earlier is super useful for the PR author and helps the assigned reviewer if issues are already addressed before they get to it. Also you may spot something the assigned reviewer missed (and vice versa).

I would emphasize that it can be really discouraging for a new user to, seemingly, be ignored. So having someone provide more immediate feedback is tremendously reassuring.

8 Likes

You can reduce the number of cycles of "official review" that need to be performed which definitely lightens the load. Doc review, typo spotting, ergonomics feedback, testing coverage, etc. are all valuable from anyone with a working knowledge of Rust and/or English (in fact, doc feedback is usually better from an outsider because the "oh, that was obvious to me" syndrome is real). If I, as a reviewer of a project I work on, see typo cycles already being done it frees me up to look at more "important" details like (analogizing for Rust and its stdlib) soundness, semver compatibility, potential semver hazards from using the new feature, etc.

Of course, there's the worry of heading down the wrong path, but IRLO is better for "aiming" contributions on such a scale IMO.

2 Likes

I think that avoiding bystander effect by having someone on the hook for it specifically is better. Especially now that it's easy to re-roll for a new reviewer.

TBH, I think it's the "team feedback required" part that's more of a holdup for most libs reviews right now. And that's just fundamentally hard, because making a small API suggestion is always easier than deciding it's a good fit for core.

Especially since, in open source, it's much easier to just let something languish than say "no", (We have that problem in Lang, too.)

8 Likes