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

[steve.dower]

He declined when we offered a number of years ago, and last I heard continues to decline. But I agree.

[pitrou (Antoine Pitrou)]

@steve.dower

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

I do think that even such PRs can benefit from review, but yes, if you have to wait weeks for approval on such changes then it gets griding.

[gpshead (Gregory P. Smith)]

@pitrou

@steve.dower

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

I do think that even such PRs can benefit from review, but yes, if you have to wait weeks for approval on such changes then it gets griding.

For what it’s worth, I review changes outside of my direct area of expertise like those examples all the time for somewhat random people at work. This might often appear like a rubber stamp to a casual observer, but that is still working as intended. For such things, I look at them and if I saw anything worthwhile to change I’d have pointed it out, or if I see something unusual I feel a domain expert would be good for the review turns into seeking a better reviewer for that bit.

As a reviewer, I often trust the change author (as I already do for Eryk and Steve when it comes to Windows specifics for example). The point of the review isn’t always specific domain knowledge on the reviewers part. It can just be checking to see that the is are dotted and ts are crossed or that modern policies and practices appear to be followed.

From a CPython perspective I think we should try to figure out how to include triagers in the set of people who’s reviews we’d count for this purpose. It could be easiest to just say “any review” counts rather than try to define a set of “valid” reviewers with an enforcement mechanism. (the “How?” here is an implementation question that we can settle after determining what policy we want).

“Reviewer Shopping” as it is called to get a random internet octocat to Approve a PR is always possible for a PR author to do, no matter what policy we define and system we setup. (Picture a dark alleyway with two core devs in it… “psst. over here. blindly approve my PR and I’ll approve yours and give you this signed FLUFL photo.”)

Lets not worry about that by overthinking things and pondering “what if it gets reviewed by the ‘wrong’ person”. That’s a people problem, we as core devs who can hit Merge can exercise our own judgement as to when acquired reviews honor the spirit of any policy we setup. The point of the question in this poll is more one of “shouldn’t we change our policy” from today’s age-old CVS repo era status quo. From experience I find that code reviews increase everybody’s trust in the long run as it increases the team feeling and means more of us see areas of the code base we might not have otherwise, spreading knowledge. Requiring reviews is a way to help build trust and increase project health.

My vote? I choose the “for new features” option as that seems like a way to ease into a process improvement. I predict in the long run we’d find it worthwhile to go further.

[steve.dower]

@gpshead

My vote? I choose the “for new features” option as that seems like a way to ease into a process improvement. I predict in the long run we’d find it worthwhile to go further.

I would have voted for that if it were technologically enforceable.

If we agree on a process whereby GitHub simply blocks PRs without a review, then we flip a switch and the process is guaranteed. For only some types of PRs, we can’t do that, and so it’s really just defining another reason to ask someone to revert.

Most people don’t blindly apply rules like this, but I worry about giving those who do a “legal” hammer to use. We expect nuance, and hope that such a rule will be applied in a nuanced manner, and so we may as well just say “use judgement, and if you disagree with someone else’s judgement, explain why (and don’t just cite the rulebook)”.

And for the record, I can’t imagine any new feature that should not require review “You added something that changes how end users perceive and use Python” is enough reason to need a review. If someone came to me and said that I’d done that, I’d at least have to prove that I didn’t - if someone came with “that’s a feature and ThE RuLeZ sAy…” I’d try and hide my work from that person in the future.

[gpshead (Gregory P. Smith)]

@steve.dower

I would have voted for that if it were technologically enforceable.

I believe this can be implemented. (Bots and labels and required CI checks can do a lot for example, we already do this today with skip-issue and skip-news). So I encourage people to vote for what they want to see and let the implementation to meet our desires be worked out as a follow on.

[alex_Gaynor]

@pitrou

I don’t think I have ever seen a project that has reliably solved the problem. Would you have an example?

For the Python Cryptographic Authority, we enforce mandatory code review for all changes. We’ve only got a handful of committers on each of our projects, but as a practical that means we spend more time reviewing than writing code. I think it’s a strong policy. Django has moved to a similar model. OpenStack used to require reviews for all changes (no idea if they still do, it’s been nearly 10 years since I was involved).

[Mariatta]

I voted for “require code reviews for new features”. In my mind the purpose of code review is not about “not trusting the PR author enough”, but more about knowledge transfer, ensuring someone else knows about the changes being made, having at least one other person learn about this new feature before it lands, giving opportunities to others to ask questions and suggest improvements.

Even when the core dev writing code is competent and doesn’t need review/feedback from others, this can be a good learning opportunity for the reviewer.

While it’s true that we can always revert commits and PRs, I find that in practice in CPython reverting new features can be difficult and controversial. I’m hoping that by requiring at least one thorough code review from someone else, including from non core devs, will be beneficial and reduce the need of reverting changes.

[CAM-Gerlach]

Sorry I’m late to the discussion—I’ve been at the SciPy conference since this was posted.

@pf_moore

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.

I can’t speak for Pablo, but (for others’ benefit, given you’re no doubt aware of it) it seems to me that this was prompted by the PR python/cpython#96146, where the new stdlib feature proposed in python/cpython#96145 was merged without any discussion or review or much opportunity for one, and later had to be reverted (in PR python/cpython#105948. There was strong core dev consensus that this unilateral process of fast track proposing, implementing and merging a demonstratively controversial feature without there first being a conclusive discussion was not something we want to encourage in the future.

@pitrou

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?

Requiring at least one approval, and sometimes two (plus a number of other branch protection checks) has been standard policy for about a half decade now in most of the other multi-maintainer projects I’m a core developer of (and often accepted community convention for much longer), including all the non-trivial projects in the spyder-ide org, the python-lsp org, jupyter/qtconsole, and others.

In our experience, while reviewer bandwidth can be an issue on some of them, this is rarely a concern for PRs by core developers who could otherwise theoretically merge them themselves as these typically get swiftly reviewed, unless they are sufficiently complex that there would be no chance the author would feel comfortable merging them without review anyway. The idea that at least one, and often multiple reviews is required to merge is strongly embedded into our community culture, and it is often at least informal convention (if not formal policy) that everyone that has reviewed or is requested for review should either approve, dismiss or remove their review request before a PR is merged, or at the very least be given a sufficient time to do so.

@steve.dower

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.

At least for the review requirement as enforced by GitHub’s branch protection rules (the standard way to require this), triager reviews “count” as required approvals just like core devs’ do. Therefore, PRs approved by a triager would be mergable by a core dev, assuming in that core dev’s judgement that triager’s review is sufficient to give them confidence to merge (due to python/core-workflow#460, which seems reasonable to me. (If we actually do want to enforce a core dev review being required, it would be trivial to do by adding a check for the awaiting merge label to a PR to the existing do-not-merge/review requested check).

@steve.dower

@gpshead

My vote? I choose the “for new features” option as that seems like a way to ease into a process improvement. I predict in the long run we’d find it worthwhile to go further.

I would have voted for that if it were technologically enforceable.

In fact, it should be relatively straightforward to technically enforce this, more or less, with a fairly small adaptation of our existing changes requested/do-not-merge check, that already blocks merge if someone either submits a review with the “request changes” state or adds a do-no-merge label. We could check that a PR either A. had at least one Write-role approval (either via Bedevere labels or the built-in GitHub info); or did not the feature label applied and at least one of the other appropriate labels (bug, crash, doc, tests, etc).

It’s not 100% perfectly foolproof and certainly bypassable by a suitably motivated core dev, but outside of corner cases or active mistakes when labeling, that would likely require a significant betrayal of the competence and trust that makes them a core dev in the first place. It would also be rather straightforward to make it more granular and gradually more or less permissive, now or in the future, based on further label filtering.

Personally, I view it as cleaner and more straightforward long term to just require triager/core dev review on all PRs, and would be happy to further increase my review cadence in support of that goal—I already review many times more PRs than I make, and have never merged my own PRs without a approval, nor would I contemplate doing so anyway. But this seems like it would make sense as an intermediate step, and to address the specific case that prompted this discussion.

[brettcannon]

The SC talked this proposal over now that the poll is over and we have decided to task @ambv with investigating what is needed to require reviews for feature PRs. This not included the mechanism but also what might need to change to make the experience as smooth as possible.

[CAM-Gerlach]

Just in case others miss it due to the length and diversity of my above reply, the actual mechanism itself should be a quite small, straightforward extension of our existing checks—the potentially non-trivially interesting bit is the UX around all that, as you mention.

@CAM-Gerlach

It should be relatively straightforward to technically enforce this, more or less, with a fairly small adaptation of our existing changes requested/do-not-merge check, that already blocks merge if someone either submits a review with the “request changes” state or adds a do-no-merge label. We could check that a PR either A. had at least one Write-role approval (either via Bedevere labels or the built-in GitHub info); or B. Had at least one of the other comparable labels (e.g. bug, crash, docs, tests) but not the feature label.

[vstinner (Victor Stinner)]

@brettcannon

The SC talked this proposal over now that the poll is over and we have decided to task @ambv with investigating what is needed to require reviews for feature PRs. This not included the mechanism but also what might need to change to make the experience as smooth as possible.

What does qualify as a “feature”? Adding a new API easily fall into that category. For example, adding a parameter or changing the behavior of a function parameter, does it require a review?

I suppose that if there is a doubt, it’s safer to wait for a review

[brettcannon]

@vstinner

What does qualify as a “feature”? Adding a new API easily fall into that category. For example, adding a parameter or changing the behavior of a function parameter, does it require a review?

Did you add that parameter or change the behaviour to fix a bug? If not then it’s a feature.

[hugovk (Hugo van Kemenade)]

@vstinner

What does qualify as a “feature”?

We ask ourselves the same question when deciding whether to backport PRs.

• Can it be backported to the bugfix branches?

• Or must it only be merged to the feature branch?

Status of Python versions

[encukou (Petr Viktorin)]

Maybe there should be some requirements on merging without review, to give interested people time to look at the changes.

I dislike PRs that are merged right after the CI turns green. Adding suggestions to a closed PR seems either pointless (if they’re minor), or hostile.

[vstinner (Victor Stinner)]

@encukou

I dislike PRs that are merged right after the CI turns green. Adding suggestions to a closed PR seems either pointless (if they’re minor), or hostile.

Can you elaborate why do you consider that suggestions after a PR is merged can be seen as pointless or hostile?

[gpshead (Gregory P. Smith)]

@vstinner

Can you elaborate why do you consider that suggestions after a PR is merged can be seen as pointless or hostile?

They’re not tracked anywhere as unresolved issues on work in progress so there is no strong incentive for someone to act on them beyond social pressure. Post merge comments are easily lost.

I recommend people who leave them always makes sure to leave record on the issue itself and ensure the issue is not closed.

[gpshead (Gregory P. Smith)]

@encukou

Maybe there should be some requirements on merging without review, to give interested people time to look at the changes.

I think there is merit here: it’d likely require a GitHub check to sort out. We don’t currently have a way of indicating to the PR state machine a blocking “please don’t merge without review & approval from Alice and Bob” interest in a PR.

At work we implement this with an optional review-bot that can be added to the reviewers list. It parses some structured text in the change description in our own review system to run a tiny state machine based on what people put in there. (It also lets you implement other types of external dependency prerequisite checks)

[CAM-Gerlach]

@gpshead

We don’t currently have a way of indicating to the PR state machine a blocking “please don’t merge without review & approval from Alice and Bob” interest in a PR.

It could be done by requiring CODEOWNERS review and setting up the file accordingly, though only on a per-file rather than a per-PR basis, and it would require admin to override. Theoretically marking specific PRs as requiring specific people could be done in the DO-NOT-MERGE check, as we currently do for the do-no-merge label and changes-requested reviews, though the UI would probably have to be via bot commands in PR comments which is rather klunky, unless we devised something more complex like your bot.

But as to the original suggestion of requiring a PR have either at least one approval or have been opened for a certain period of time, else block merge, the only real challenge with implementing that (e.g. in the existing labels/reviewers action check) would be providing a way to re-trigger it after the period of time has passed, as well as being triggered on review (unless we set a cron-triggered action to sweep through all PRs every e.g. hour or so).