SC Poll: Should we require reviews to merge all PRs?

Committers
1

In the Steering Council we have been discussing the possibility of requiring at least one review from a core dev in all Pull Request to ensure that relevant changes are always approved by at least another core dev other than the one proposing the change. This would have some advantages and disadvantages and we are aware of it but at this time we would like to gauge what are the views of the core dev team on this matter.

Please, vote in the following poll what is your preference on this matter and feel free to comment in this post what are your views/worries around this topic.

Should we require reviews in Pull Requests?
  • We should NOT require reviews to merge PRs
  • We should require reviews to merge PRs for new features
  • We should require reviews to merge ANY PR
0 voters

As a reminder, the purpose of this poll is just to gather information regarding how core dev team members feel about this and is not binding in any regard.

Thanks a lot!

Representing the Steering Council,
Pablo Galindo Salgado

5 Likes
2

I’m perhaps going to be the minority here. I voted that We should NOT require reviews to merge PRs.

My main reasoning follows: it should stay a strong recommendation but not a strict requirement. I have seen many cases where it is infeasible/not required to have another core dev reviewer. For e.g.:

  • When there is only one active maintainer for that part of the codebase.
  • When a trusted member, triager or domain expert who isn’t a core dev gives the approval, why do we strictly need another core dev? E.g. if Steven Sun or Eryk Sun approves my C/Windows PR, or if Nikita approves my typing PR, I won’t have any doubts merging those.
  • When there are certain esoteric security fixes that can only be reviewed by other external security experts. I recall on one occasion a security fix was reviewed by a 3rd party security expert who wasn’t a core dev.

However, I would be strongly supportive of this if we change the wording to allowing a triager or core dev review for PR merge, and leave the other exceptions to the purview of the security developer in residence or release manager.

8 Likes
3

I would be interested to know if any specific events triggered this discussion, and/or if there’s a general concern around unmotivated changes being pushed by core developers.

Abstractly, I think it would be best if all PRs were validated by a review, but the dreaded manpower problem may make this unfeasible.

In any case, I don’t think a distinction should be made between “new features” and “bugfixes” in this regard. Both categories of changes have potential for negative impact that deserve careful attention.

4 Likes
4

Commit rights are an indication that we trust the person receiving them to know when they need a review vs. when they don’t need a review.

If we don’t trust someone, we need to talk to them first. Perhaps what we need is a system to review/suspend/remove commit rights when someone repeatedly demonstrates that they don’t know when a contribution has been suitably reviewed (which may include no additional review)?

6 Likes
5

For comparison, previous poll in 2020: Make code review mandatory?

6

Honestly, I would love getting more reviews. It’s common that I ask for review to 1 to 4 core developers, and most of the time, I never get any kind of review :frowning: (even if I wait a few weeks, or months)

If it’s a “review request just for information”, I just wait for 1 day (or even just a few hours): usually, it’s the time for me to “sleep on the change” and review it with a new look :slight_smile: I’m now using cc @github-handle syntax to send a “copy” of the PR to some developers to notify them. At least, they may get my notification later and do a “post-merge” review (I love them, please continue doing that!).

When it’s an important API additional/removal, I wait for a formal review. Usually, either I get a feedback in a few days, or… never :frowning:

I learnt to adopt the “it’s better to ask forgiveness than permission” approach just to avoid stalling on my work.

Moreover, the lack of “patch series” support on GitHub makes it really painful to work on a long serie of changes. I would prefer to create a “pull request serie” once and wait for a review. Right now, I have to move fast by merging each change, one by one. I do that often for “refactoring” work (no API or behavior change).

I know that my development speed (I’m more available than some other core devs) causes friction time to time with other core developers who would like to be involved in changes, but are not really available to provide feedback. I’m asked to do nothing (in short), until these people are available (which may not happen before a few months).

Other problem to consider is that each core devs have different interest areas, and it’s common that a single dev only really care about a specific topic and so it’s unlikely that they get a pull request (ever!). For example, I saw that problem over the last years in the asyncio module: there were not enough maintainers. I saw that problem is many other areas.

If we require reviews, how to address the lack of available reviewers? Or is the willingness to slow down Python development on purpose? Less changes to break less things!

Maybe if reviews are required, we will see way more devs available to “trade reviews” :slight_smile: I do that with a few core devs: they review my changes, in exchange I review their changes :slight_smile: In the past, Martin von Loewis offered 1 review in exchange with 5 reviews on his work :slight_smile:

10 Likes
7

I agree - if we don’t trust a committer’s judgement to know when it’s OK to merge something without review, and when to wait, we need to address that with the individual(s) causing the problem. Adding more roadblocks that will make it harder for committers who do exercise good judgement to make progress isn’t the right approach IMO.

But like @pitrou said, this feels like there’s something that has triggered this question[1] and I feel that not having the explicit background makes it very difficult to answer the question. If I say “no, we should not require reviews”, does that mean I’m implicitly accepting the problems that currently exist, and there will be no discussion with individuals?

I agree - all such a distinction will do (IMO) is change the debate into “what is classed as a new feature?” Is upgrading the bundled pip a new feature? What about removing setuptools from ensurepip? I currently make my own judgement over whether it’s OK to merge PRs like this without a review. Having to decide whether they constitute a “new feature” is no easier a decision, and opens me up to arguments over my answer to that question just as much as the current situation.

That seems like the wrong conclusion to draw. If you decided that a PR needed a review, the reasons for making that decision surely still apply even if it takes a long time to get the review? Yes, sometimes it is better to ask forgiveness than permission[2] but that’s not what you’re doing here, instead you’re letting impatience change your decision.

That’s the key question here. Do we have the resources to mandate reviews? Wouldn’t we be better addressing the problematic behaviour that causes us to consider mandatory reviews?

  1. Disclaimer: I’m on Discord and I’ve seen conversations which might have led to this, but I don’t want to make assumptions. ↩︎

  2. Although I’m not sure it’s appropriate in this context :slightly_frowning_face: ↩︎

4 Likes
8

I’m very in favor of more reviews.

First, the purpose of review isn’t to “punish” the author and its not because the author isn’t trusted. It’s because reading code and writing code are not the same process, and you need someone with a fundamentally different disposition towards the code to look at it to be able to spot potential issues. It’s the same reason professional authors have editors.

Second, making core developers participate in the same process as external contributors builds empathy. If the process is bad for them, we should make it bad for ourselves, or we won’t be properly motivated to improve it.

16 Likes
9

I totally agree with this. Also we have a review backlog, the reason being no reviewers, external contributors already feel demotivated. We will have another PR backlog from core devs, by the way this is discouraging, a typo for example waiting for a year, just because no one has volunteered a review. I think core devs understand the power of a commit bit and are able to exercise responsibility.

6 Likes
10

If there has to be moderation, we can maybe discuss a compromise for new features, I think requiring review for every PR, might slow some even really simple things.

11

It’s easy to say that the process should be improved, but I don’t think I have ever seen a project that has reliably solved the problem. Would you have an example?

12

If we don’t require reviews, how would we address that lack of available reviewers?

Does anyone not agree that reviews are good? Putting aside the problem of getting a reviewer – if it was easy (guilt-free, non-intrusive to other people, with an easy/automated escalation mechanism for stalled reviews, no manually writing pleading posts) to get someone else to review your PR, do people think reviews are not worth it?

At work we require reviews. There are a number of effects that happen precisely because reviews are mandatory:

  • Nobody minds rubber-stamping simple reviews.
  • Reviews that were expected to be rubber-stamps come back with comments anyway, or with questions to clarify things, and that’s useful. A fresh set of eyes can be useful even if you didn’t expect it, and questions indicate where documentation or comments need expanding.
  • Reviews in areas where other people work too, and who you can send reviews to, automatically keep those people up to date on what you’re doing. This is even valuable when dealing with small bugfixes.
  • You pull in new ideas, new contributors, and spread knowledge of your changes, the code you’re changing, the bugs you’re fixing and how you’re fixing them, all because you’re putting them under more eyes. Having more code review means more awareness of the changes and the code in general.

(Just as a minor example, when I first joined Google I was the only person working on Python in the “languages” team, and I had to send my reviews to people outside of the team. @gpshead was one of those people, and he’s been on the Python team with me for a long time now. Other people I sent reviews to ended up helping with team projects, too, things they would otherwise probably not have been involved in.)

And yes, we, as core developers, trust other core developers. We trust each other to know what we’re doing, and to know our own limitations. We can still be wrong :slight_smile: And we can still (and often do!) disagree on the interpretations of certain boundaries (like PEP 387 cough). I’ve seen quite a few changes where I’ve thought “that could’ve done with another round of reviews”, but since they’re already in I haven’t complained or pushed back on them – and one of the reasons I don’t is because I know it can be hard to get reviewers.

But I think part of the reason it’s hard to get reviewers is because it’s not established culture to need them. If we want mandatory reviews (on features, or on everything), we need to make that possible. I know it’s different at work than in open-source, where nobody[1] gets paid to do reviews, but I do believe we can set expectations, and still handle it gracefully when people aren’t in a position to meet those expectations. If you can’t review something in a timely manner, that should be fine, and we should have an easy mechanism to find another reviewer. (To be honest, if we had mandatory reviews, I’d be quite happy to spend all my time reviewing other people’s changes.)

So the question really is, do we need to put effort into improving the review process so that we can have mandatory reviews (of features or of everything)?

  1. I know a few people get paid to work on Python, but they’re probably not paid specifically to review code, and their reward structure probably doesn’t recognise those contributions. ↩︎

13 Likes
13

Yes, we do. This is the key thing - let’s not increase our reliance on reviews until we have a good mechanism for getting reviews. I don’t believe the argument “let’s make it painful for ourselves, then we’ll fix it” holds water - we know reviews are good, we would have already fixed the issue of getting reviews if it were easy to do so. But just because the problem is hard, doesn’t mean we can give up on trying to fix it.

The secondary (but still important) question for me is, until we get to a point where reviews are easy to get and don’t hold people up, how do we address the issues caused by people not being willing to wait for reviews, when they are needed? We can:

  1. Educate people better in what PRs can reasonably be merged in spite of lack of reviews.
  2. Introduce ways of dealing with consistent offenders who don’t wait for reviews (if there are any such people - my feeling is that there are, or we wouldn’t be having this discussion).
  3. Make it clear what we’re doing to improve the availability of reviewers, so people have confidence that the problem is known, and being addressed.

Edit: To be explicit, I would have voted “yes” to mandatory reviews if I could have been sure that timely reviews were available. Even though I believe we should trust committers’ judgement. Because, as you say, a culture of always getting reviews changes how people think about reviews, and always-mandatory reviews are no more problematic than always requiring a news entry, in that context. But only if getting a review is no harder than adding a news entry!

4 Likes
14

Excuse me, but that’s not what I was suggesting at all. I’m not saying we should require reviews and then find ways to make that workable. I’m saying we should decide we want to require reviews, then find ways to make that possible.

4 Likes
15

Sorry, I didn’t mean that comment to relate to what you said. It was intended as a response to what @AlexWaygood said previously - “If the process is bad for them, we should make it bad for ourselves, or we won’t be properly motivated to improve it”. I should have been clearer.

16

My view is slighly different. I think we should decide we want to make it easy to get reviews, and then requiring reviews becomes much easier to accept if we don’t have to address the question “but what if it takes 3 months to get a review?” In other words, I want easier reviews, whether or not we end up requiring reviews.

17

Just to note, it was @alex_Gaynor who said that, not me :slight_smile:

1 Like
18

Sorry, I’m obviously having a bad day :slightly_frowning_face: (not even going to try to blame autocomplete at this point). I’ll just shut up now.

2 Likes
19

A few examples of my own PRs that I don’t believe should have to wait for someone else to review them:

There are also a decent number that were only reviewed by triagers (specifically Eryk Sun), and would no doubt have been held up significantly longer due to the lack of core devs who either already understand the implications of these changes, or have access to and the time to test them thoroughly on Windows.

I will gladly admit there are other PRs I’ve merged that could have benefitted from core dev review, but generally I made the call that having the change in an upcoming prerelease would get more useful feedback than skipping that release and waiting for a reviewer. I also take full responsibility for those changes and will prioritise later fixes to them when needed, and of course I fully respect the rising threshold as we approach final releases.

So I feel that making all reviews mandatory would inhibit my ability to make fixes, and I’d become a fairly annoying voice pinging people all the time to try and get someone to review an area they’re not really familiar with.

The only reason I’m not even more strongly against this is because the release scripts live in a different repository, and so mandatory reviews won’t prevent me making hotfixes in order to unblock a release.

20

He should be promoted as a core dev, his insights are very precise and useful. I trust him.

6 Likes