Assign folks to urgently review the oldest 20% of PRs during the community meeting

[sarayourfriend]

The Openverse monorepo had over 35 open PRs (excluding drafts) in need of review last week. The number is down to 28 as of writing this, so already significantly improved, but mostly because some folks went through and intentionally reviewed some older PRs.

We need to institute some ongoing practice to distribute PR reviews for the oldest open PRs during community meetings. I propose the following:

During the community meeting, the oldest 20% of open, undrafted, and unblocked PRs in need of reviews should be distributed to volunteers (synchronously and asynchronously) to urgently review within the next two days.

AFKs should be kept in mind, but reviews should be distributed equitably, potentially even at random. Later this week I'll write a script that automatically does this. It should be easy to write one that:

1. Requests the list of relevant PRs from the GitHub API
2. Chooses the oldest 20%
3. Assigns reviews in round-robin using a random order

The script should also have configuration options to ignore certain people if they're AFK or otherwise unable to review PRs at the moment.

[krysal]

We're already assigning reviewers to PRs so I don't see how assigning more in a second batch will help here. What are the reasons the assigned persons have not reviewed it? Are they focused on other projects or doing non-visible work outside GitHub? Was the number of PRs reduced due to also creation being paused while existing ones were reviewed?

The issue seems to be somewhere else. I don't have a good answer, but perhaps to suggest a limit at which one should refrain from creating more PRs until merging or closing the currently open ones, like 10 PRs maybe? Each one represents a stream of work with considerations to evaluate by reviewers: check added automated tests, dependencies, migrations, a11y, etc; and the work can get staled even by authors when they're handling so many.

[zackkrida]

This is now down to 21 open PRs, excluding drafts.

My sense is that we need a bit of a cultural shift amongst the team. The net amount of work are doing does not need increase. Instead, if we focused on reviewing open code before writing new code, we could benefit by having fewer open work streams at once, and less context shifting.

As far as how we facilitate this context shift, we need to find ways to incentivize folks doing code review earlier and more regularly. Some things that might work could be:

• Open PR limits, like Krystle mentioned. I don't like that this might block folks from doing valuable work or work that they are excited about, though.
• Find ways to make open PRs louder, perhaps:
  • Change priority labels based on how long something has been open (i.e, any PR open longer than 10 business days becomes critical)
  • Shorten the window for PR review reminders
  • Strengthen our "sprints" concept to be more meaningful. Deeply discuss issue assignments for the next two weeks in each community meeting, hold each other more accountable for unhandled issues.
• Something like the described approach above, but perhaps more manual? That script sounds like it could get complicated to write and maintain, and that it's potentially "treating a symptom" (albeit a significant and critical one) rather than addressing an underlying problem as @krysal alluded to

[sarayourfriend]

Pinging @krysal here so she see's my clarifications as well.

Change priority labels based on how long something has been open (i.e, any PR open longer than 10 business days becomes critical)

My suggestion would do this but shorten the window to 5 business days (the interval of our community meetings). This matches the urgency of low priority PRs, so is not an unfair expectation or precedence.

I do not think PR limits are a good idea, at least not absolute PR limits on the entire project. 10 PRs would be less than 2 PRs per code-oriented maintainer. On the other hand, I think that people should absolutely start taking into consideration when looking for the next thing they can do whether that should be reviewing open PRs, even ones not assigned to them.

Additionally, I would like to contextualise "20 percent". If we have 40 open PRs, 20 percent of that is 8. That is still less than 2 PRs to urgently review per code-oriented maintainer (assuming no one is AFK). The burden of this change would not be significant.

The benefit, on the other hand, is immense, in my opinion. Right now, if a PR you have open is lagging in reviews, you need to ping for reviews. If you have to do this repeatedly in a week or often throughout a time period, you will inevitably begin to wonder if your PRs are being ignored or frustrated by the constant need to ping people for review reminders. Implementing this change in the community meeting would prevent people from needing to feel like they're constantly pinging. PRs would naturally come up in the 20% described, but wouldn't require individual authors to do the pinging. It would happen in a batch, during the meeting, and be distributed randomly. This has an additional advantage (other than shifting the burden) of making the pings automatically random. Right now, we need to either ping the people assigned or decide to make a general ping. This is also a cognitive and emotional burden (is my PR important enough to warrant a team-wide ping?). Shifting this to the community meeting, making it random (and encouraging people to swap for PRs they can review more easily/expertly, if wanted) removes that decision as well.

In short, the approach I describe would (a) not increase the burden of work, (b) catch the oldest PRs open at any time, which can sometimes easily fall through the cracks, and (c) remove the emotional and cognitive load of PR authors needing to ping (and reping) for reviews on PRs that have been open.

To clarify what "urgently review" means, I mean to treat the PR review as critical. In other words, reviewing the PRs assigned to you should be the next thing you do when you're working (assuming there isn't something obviously higher priority like a production outage).

The script I am suggesting would not need to be complicated. It would go like this:

prs = get_open_prs_needing_reviews()
sorted = sort_prs_by_oldest(prs)

# Round up, optimistic
20_percent = math.ceil((len(sorted) / 100) * 20)
oldest_20_p = sorted[:20_percent]

available_reviewers = get_openverse_maintainers() - sys.argv[1].split(",")
shuffle(available_reviewers)
reviewers: { [name: string]: PR[] } = assign_reviewers(available_reviewers, sorted)
print(reviewers)

[sarayourfriend]

I think this approach would work well combined with the change proposed in #1984. That issue would make the initial pings happen sooner while the community meeting change would catch PRs that still fall through the cracks and ensure that PRs do not go without review for a very long time.

[AetherUnbound]

I don't have opinions on any specific direction, but I thought I'd share my thoughts as it relates to PR reviews. I find in general that I prioritize reviews as some of the first set of work that I do daily and I try to do as much review as I can alongside other work. At the time of writing this comment, the last PR I opened was over two weeks ago, and in the interim I've almost exclusively been focusing on PR reviews, implementation plan discussions, and other meta/glue work. Especially with the discussions we're having around focusing on reviews and the urgency of some work needing to get merged before AFK, I think I'm likely to find myself focusing on reviews for most of my working time this week. All that to say, I feel like I'm, personally, at capacity with the mount of work I'm reviewing. I don't know how that might fold into or inform this discussion, but I wanted to share my perspective and how I've been feeling.

[sarayourfriend]

To clarify, my proposal does allow for excluding people from the list of available reviewers. If someone is already at absolute capacity then obviously assigning a PR for them to urgently review will only cause slow-downs elsewhere, helping no one, in the end.

I also want to extend empathy. I've been in the same boat as you before (for several months at the end of last year and start of this year) and it is exhausting. It wasn't until my implementation plan for the API filtering of sensitive textual content was approved that I started working on PRs again since last November that felt impactful. I hope that either the glue/meta work you're doing gets spread to others (please share what you're doing if it's something anyone else can do) or some other solution comes up to free you up to work on more exciting things soon.

But again, to clarify, if someone is already at capacity, asking them to do anything urgently doesn't help anyone. The idea behind this proposal is to say that for the people who are able to review a PR, as PR review is our regular job duty, then help everyone by intentionally reviewing older PRs first. We can do this more equitably and with less overall emotional and cognitive burden by making it a weekly practice that people can opt out of when they need to (just like we do with MSR, for example).

[krysal]

Recording communication that happened outside of GitHub (May 17 meeting): the general consensus in the team is that we don't need more automation for PR review reminders and will keep DM-ing peers for it.

[zackkrida]

Just a reminder, because I completely understand that it can be burdensome and stressful to DM folks for stuff like this, there's also the "blockers report" in the maintainer's slack channel that can be used for this purpose.

[sarayourfriend]

Based on the number of PRs open now and last week, it does seem like the change to the PR review reminders bot #1984 may have helped a lot here. I also don't think we need this new process right now and should re-evaluate in a month or so and decide whether some additional support is needed.

[sarayourfriend]

See #1976 (reply in thread) for resolution