Proposal: Require non-committer review for PRs

Development
1

In the same spirit as More granular commit access control via merge bot, I’d like to discuss a proposal to reduce PR lead time and improve the experience for authors and reviewers:

Require an approval from a reviewer who is not a committer for a PR to be merged.

Wait, what?!? How could adding more obstacles improve the PR review situation? Let me explain:

For a code contributor, getting a committer to look at your PR is everything. A non-committer can give helpful feedback, in theory - but usually, it’s the committer who either merges your changes or tells you the Nix incantation that will make your derivation worthy.

While non-committers are encouraged to review PRs, there remains little incentive to do so:

  • Since committer review is necessary always, non-committers can feel unwanted in a PR. Or, it feels like wasted effort.

  • For a contributor with PRs waiting for review, becoming a committer is not a solution. Code contributors want other people to become committers. (Plus, when it’s an expectation for a committer to have 50+ merged PRs and you’re struggling to get anything reviewed, it’s a discouraging situation.)

Non-committer reviews are valuable, but the system we have now fails to make non-committers feel valued.

The idea of this proposal is to increase reviewer participation by making non-committer review a crucial part of the process.

  • Authors will seek out reviews specifically from non-committers.
  • Non-committer approval meaningfully moves a PR forward.
  • Authors are more incentivized to address non-committer comments.
  • Non-committers get satisfaction from all of the above.
  • Non-committers get more practice reviewing PRs.

Reviews are valued → Reviewers have fun → More reviews → Shorter backlog. :tada:

A few details:

  • Exceptions may be necessary for security fixes, urgent reverts, and other emergencies.
  • It should be easy to tell at a glance whether a PR is eligible to merge.
  • It should be easy to see a list of PRs requiring first-pass reviews (or PRs already approved).

Concretely, I propose adding a GitHub status check that checks whether any approver is not a member of nixpkgs-committers. This check should be configured as not required in GitHub (at least initially). Committers should (in the RFC2119 sense) refrain from merging PRs where this status check has not passed (but committers should always feel free to leave a review and approval.)

I don’t know much about automation on GitHub, but adding labels based on approval status seems eminently doable.

Thanks for reading my proposal!

9 Likes
2

Additionally, if there were clear expectations of what made a review useful, and how to do so in a way that the project would appriciate, it would make it more welcoming for people considering reviewing.

5 Likes
3

I don’t think there’s any need to feel unwanted. I appreciate any helpful reviews, and I’m sure that many other committers feel the same. When other people leave positive reviews, I can merge easier, even if they’re not committers. And especially, those from meta.maintainers of the affected packages; those often are not committers either and have significant weight there in my mind.

8 Likes
4

I’m not in favour if this:

There are many PRs where author and merger are both (different) committers.
Requiring an additional person really does feels like a significant obstacle for such cases.

For example, @Sandro has merged 16000 PRs in the last 3 years.
Many of those are from other committers.

If for each one, he’d have to think “the change looks good, but wait, it’s not reviewed by a non-committer, let’s postpone it”, nixpkgs merging would probably slow down.

Like @vcunat above, as a committer, I see big value in non-committer and especially maintainer reviews, and value their feedback no less than that of commiters.

I recently started to thank PR authors and reviewers more when I do a final comment in a PR along with the merge – maybe doing this a bit more can make clear how appreciated these roles are.

2 Likes
5

I think the interesting thing about this proposal is that it’s effectively trying to socially engineer a larger community of active reviewers, at a cost of a (hopefully) short-term significant obstacle for committers.

Since the entire idea is adding an obstacle to PRs, I don’t think it should be dismissed just because it will add an obstacle to PRs. It would be more useful to think about how much of a slowdown it would actually cause, and whether that’s worth the trade-off.

I do certainly feel some of the sentiments @Majiir intends to tackle with this when I attempt a review.

Perhaps it could be experimented with in a more limited capacity than all of nixpkgs?

The risk is of course that all of this fails and the PR backlog just grows. I frankly don’t have the numbers or context to judge if it’s practical, and it’s not even certain that it would achieve the stated goal, but as usual with social engineering it’s a cool if slightly insane idea.

2 Likes
6

I don’t have the time/energy to write a larger response for now, but I’ve recently gotten to the conclusion that the fundamental problem with Nixpkgs’ PR backlog is that the number of packages grows quicker than the number of committers.

And imo the best way to fix this is automation, more automation, and even more automation! This should happen from two sides:

  • Tree-wide architecture and changes: This is what I’ve been going for with RFC 140 and the shepherding/co-authoring of RFC 101, both of which should relieve some burden on committers over the long term with tree-wide changes and CI going forward
  • Per-package work: Make packages maintainers truly responsible for maintaining their packages (duh!). This should mean: Getting notified for build failures, responding to issues, apply tree-wide improvements to your package, etc. Concretely this could be implemented with a weekly/monthly email summalising any actions that need to be done for the package. And if somebody isn’t doing these, the package should get removed or get new maintainers. And all of this should be done automatically.
20 Likes
7

This seems obviously correct, yeah.

Can we have an automation hackathon or something that might draw the focus of some senior community members? Getting infrastructure maintainers to review automation PRs takes even longer than getting Nixpkgs committers to review package PRs, in a vicious meta-cycle.

Develop your machines so you can free up people to develop your people so more of them can develop machines.

4 Likes
8

This is exactly it!

That’s interesting. I’m thinking of it as a short-term obstacle for authors rather than committers. There is large (probably total) overlap between those groups, but they are different activities.

From nixos-23.05 to nixos-unstable, the number of package maintainers has grown (+214, to 2821) by more than the total number of committers (203).

With so many non-committers who are engaged contributors, it shouldn’t be hard for a committer to find a review. If it does turn out to be hard, then that implies non-committers are not engaged as reviewers, and we should fix that.

Absolutely, I don’t doubt it. But your feeling of appreciation doesn’t necessarily make non-committers feel appreciated or make reviewing a rewarding activity.

We can all agree that non-committer reviews are helpful today, but we should also understand why someone without the commit bit might hesitate to submit a review. Ultimately, if those individuals don’t find reviewing to be rewarding, then they won’t do it. The idea here is to change the game so that more people want to review.

Automation is great, and we need more. I agree that it would help with the backlog. These are orthogonal solutions for slightly different problems, though (too many things to do vs. not enough people to do them). We can walk while we chew bubblegum!

1 Like
9

The pull request template has some suggestions what to do during review. Maybe it’s unclear that the items aren’t just for the original author?

2 Likes
10

How would this work with automatic update scripts? Would committers not be allowed to merge until it got a non-committer review even though that seems to defeat the point of the automation (getting update PRs done and merged more quickly)?

1 Like
11

Committers should (in the RFC2119 sense) refrain from merging PRs where this status check has not passed (but committers should always feel free to leave a review and approval.)

As a non-committer, I think this part would just produce unnecessary churn. I think there are many other opportunities to make non-committer more valuable thou, and the rest of the proposal could be a step in the right direction.

Non-committer reviews are valuable, but the system we have now fails to make non-committers feel valued.

I would also say that at times, it fails not only to make them feel values, but also at actually making them matter in the first place.

What I find the most discouraging is when there are things that aren’t just “nitpicks”, but genuine issues with a PR by a committer, and the committer isn’t open to criticism. In those cases, it can be hard to be heard, and it can feel frustrating that there isn’t any mechanisms in place to fall back on.

I personally have a vague sense of where I go with those things now to get someone with commit to look at the PR, but I don’t think everyone does, and I think there is a great opportunity to make non-committers valuable here. Having some sort of way to escalate concerns would be very nice.

Another thing I did notice was that there are many different views of what’s expected on a review. The suggestions I received on my first contributions were often mostly about stylistic changes, but later I came to learn that some contributors frown on those kind of suggestion, while others think they’re important. This makes it hard to know what’s actually expected of a review, and I imagine that for many people, it can be daunting to review a PR when it seems unclear what that entails.

I think RFC 101 is a great step for solving this, but until then, it would be great if there was a clear idea whats expected of a review.

The pull request template has some suggestions what to do during review. Maybe it’s unclear that the items aren’t just for the original author?

I’m unsure whether it’s unclear, but it would be a safe default to make it clear. I think it might also make sense to reorder it slightly. In general, I feel like there could be many improvements to it to make it less overwhelming for new users, if you’re open to suggestions on that lmk.

5 Likes
12

The PR template does get changed. I’m quite certain that it’s not set in stone, though I’d proceed with caution before merging a PR changing it, ensuring there’s wide consensus. (it’s just another file in the same repo)

Overall I personally think that even among people with push access there’s significant diversity. These people have different opinions and will focus on different things during review. Maybe making a bit more concrete guidelines would be nice, but I believe we can’t really try to force strong sense of “unity” (in some sense) in the nixos.org community – because that would probably fail (or at least push a significant fraction of it away).

1 Like
13

At many work places removing obstacles is a big topic as it is a big factor to demotivate people until they (silently) quit. Placing down obstacles that are hard to justify for technical reasons will not be met with joy by everyone and losing very talented committers which review each others changes through such a policy change could be described as stupid.

:100:

1 Like
14

For this proposal to go anywhere, I think you’re going to have to provide objective evidence that A) Non-committers feel disincentivized right now, and that B) They would feel incentivized by this change. Without actual evidence that this is the case, it’s very risky to make it even more difficult for a PR to be merged.

My personal impression is that most non-committers won’t care.

3 Likes
15

I use the word “insane” :slight_smile: But there is a thought process behind this particular madness, and I at least think it’s a neat idea.

Realistically I think @ElvishJerricco is right, there are far more, larger obstacles that make it just too much effort to contribute than the perceived opinion of committers. Git repo size (download time), build time and relative complexity of testing a PR are probably far more important barriers. Automation can come in to help resolve them.

Making PRs harder to land might even further disincentivize reviews, given the review-for-a-review “system” and friendly prompt from the PR template.

And yes, I suppose I didn’t consider what might even be a subconscious “chilling” effect on current committers. Even if you were totally on board with this, the extra effort would probably be draining over time.

It’s probably more productive to help with reviews, whether through automation or more structure/guidance, than to force it through requirement.