[Matplotlib-devel] Proposal to amend the PR merge rules

Tue Jan 15 14:11:17 EST 2019

I'm obviously more aware of my own PRs, so here are a few I'd have put up
for single-review-merge (note that it's quite possible that they'd have
attracted more attention and gotten merged faster under that system; that's
also the goal...).  Obviously other devs may think about other PRs that
could have benefitted from the same process.

- https://github.com/matplotlib/matplotlib/pull/9787 adds support for the
ttc font format (requested since 2014) for png/svg output (that's provided
basically for free by FreeType) but not for pdf/ps (that would require more
difficult changes).  The entire PR comes down to:
  * adding "ttc" as an extension that should be treated like "ttf"/"otf",
  * adding error handling to pdf/ps to signal these formats are not
supported there,
  * tests.
  The PR sat open for 10 months before a first positive review (because no
one cares about fonts, I guess) and took another 3 months to attract a
second positive review.

- https://github.com/matplotlib/matplotlib/pull/12472 does a fairly trivial
fix to fontList.json to make it reusable for Matplotlibs installed in
different venvs, got a positive review in one day and waited another 2.5
months for a second positive review.

- https://github.com/matplotlib/matplotlib/pull/9867 deduplicates
completely duplicated code between the pdf and ps backends, mostly for
maintainability's sake; it took 7.5 months (and 5 rebases due to conflicts)
to get a first positive review and another 6 (and around as many rebases)
to get a second one.

-----

As for the specifics (as I see it):
- Someone opens a PR, it gets a positive review, no negative review, and no
activity occurs for two weeks (significant activity -- excluding trivial
chat on the thread).
- Sponsor (first reviewer, or author if a committer) *pings* all devs by
mentioning @matplotlib/developers on the PR thread with the intent to
single-review-merge, and labels the PR accordingly.
- The PR can still be reviewed/merged/rejected by the normal review
process.  However, if no one explicitly opposes the merge within two weeks,
anyone can merge it at that point (including by self-merge).

I would not exclude all API changes from the process.  For example,
https://github.com/matplotlib/matplotlib/pull/13173/commits/e90d264f3e5a263ef57243a2af86b46ab74ccc16
deprecates (and prepares for deletion) two parameters to imshow() that have
had no effect since 2006.  Let's pretend for a second this commit was a
single independent PR (and not an example to showcase the
parameter-deprecating decorator...); if it started being forgotten I would
have proposed it for single-review-merge (again, note that if you're
worried about the change, you don't need to reject the PR; you can just
say, I think this API change needs to be approved by a second reviewer and
that'll block the single-review-merge just as well).

Antony

On Tue, Jan 15, 2019 at 7:46 PM Nelle Varoquaux <nelle.varoquaux at gmail.com>
wrote:

> The proposed scheme by Paul doesn't seem reasonable to me. Core
> contributor A or committers needs to actively reach out to other core
> contributors: labeling is not enough IMO.
>
>
> On Tue, 15 Jan 2019 at 10:37, Jody Klymak <jklymak at uvic.ca> wrote:
>
>>
>>
>> On 15 Jan 2019, at 10:20, Paul Hobson <pmhobson at gmail.com> wrote:
>>
>> I think this would be a good policy as well. Can I get some clarification
>> on the flow? The way I understand it:
>>
>> 1) Someone (committer, contributor, or first-timer) opens a simple PR
>> 2) Committer A reviews it, adds a "single-review-merge" label, then
>> digitally walks away
>> 3) No less than two weeks later, Committer B sees the PR, notices the
>> label, opens it up, and merges without reviewing.
>>
>>
>> If there is a Committer B, I think they should at least look at the PR to
>> make sure it doesn’t have any hidden API changes, or introduce anything
>> hostile.
>>
>> The point here is that getting the third person to look at a PR is
>> proving quite difficult, and if the rest of the folks with the commit bit
>> haven’t looked at a PR for a month, even after being warned, then silence
>> indicates consent.
>>
>> Note that only folks with the commit bit can label PRs, so anyone
>> flagging a PR like this is implicitly trusted to not be wreaking havoc, and
>> not doing this for major changes.  Since we all curate our own PRs, I
>> rather expect that the PR contributor will often be the person who adds the
>> flag.  Whether we want to allow a self-merge at that point is up for
>> debate, but if not, then folks need to get in the habit of looking at old
>> flagged PRs and merging them.
>>
>> I think a huge problem we have is the LIFO default sort of the github PR
>> queue, and that any PR not on the first page might as well be closed for
>> all the attention it will get without constant nagging.
>>
>> Cheers,   Jody
>>
>> Does that capture a successful "single-review-merge" lifecycle?
>> -paul
>>
>> On Tue, Jan 15, 2019 at 9:57 AM Nelle Varoquaux <
>> nelle.varoquaux at gmail.com> wrote:
>>
>>> Hello,
>>>
>>> Overall, I think this is a good idea, but I would like specifics on what
>>> "uncontroversial but uninteresting" PR mean. IMO, that should exclude any
>>> PR with API changes.
>>>
>>> Cheers,
>>> N
>>>
>>> On Tue, 15 Jan 2019 at 06:00, Antony Lee <anntzer.lee at gmail.com> wrote:
>>>
>>>> Hi all,
>>>>
>>>> During the weekly dev call, I proposed to amend the PR merge rules,
>>>> following an initial comment on Github [
>>>> https://github.com/matplotlib/matplotlib/pull/13173#issuecomment-453921220].
>>>> There was general agreement among the devs present (Tom, Hannah, Jody, and
>>>> myself), so I'm putting it here for discussion.  The objective of this
>>>> change is to prevent "uncontroversial but uninteresting" PRs from falling
>>>> into oblivion, and try to decrease the size of the open PR stack.
>>>>
>>>> The current rule is that a PR needs positive reviews (from two
>>>> committers, and excluding the author if a committer) to be merged, except
>>>> that doc-only PRs (docstrings, rst) only needs a single positive review.
>>>>
>>>> I am proposing that, if a (non-doc) PR already has one positive review,
>>>> but no activity on that PR has occurred for two weeks (exact time interval
>>>> up to bikeshedding) [Jody suggests: and the PR has 100% code coverage],
>>>> then a committer (either the first reviewer, or the author if a committer)
>>>> can suggest that it be merged on the basis of that single review.  To do
>>>> so, the "sponsor" should ping all developers (@matplotlib/developers) on
>>>> that issue indicating that intent, and add a "single-review-merge" label on
>>>> the PR (so that these PRs can easily be found).  Committers are encouraged
>>>> to review the PR to accept and merge it or reject it or request changes on
>>>> it; but they can also just indicate that they consider the PR sufficiently
>>>> complex that a proper second review is needed before merging, or request an
>>>> extension, etc.  To do so they should still leave a "reject" review, even
>>>> if just saying "objecting to single-review merged; anyone can dismiss after
>>>> a second review".  However, if within another two weeks, no committer
>>>> voiced any objection (explicitly, i.e. by rejecting), then the PR can
>>>> indeed be merged on the basis of that single review.
>>>>
>>>> To avoid overwhelming the system, any committer can only "sponsor" a
>>>> single PR at a time.
>>>>
>>>> Thoughts?
>>>>
>>>> Antony
>>>> _______________________________________________
>>>> Matplotlib-devel mailing list
>>>> Matplotlib-devel at python.org
>>>> https://mail.python.org/mailman/listinfo/matplotlib-devel
>>>>
>>> _______________________________________________
>>> Matplotlib-devel mailing list
>>> Matplotlib-devel at python.org
>>> https://mail.python.org/mailman/listinfo/matplotlib-devel
>>>
>> _______________________________________________
>> Matplotlib-devel mailing list
>> Matplotlib-devel at python.org
>> https://mail.python.org/mailman/listinfo/matplotlib-devel
>>
>>
>> --
>> Jody Klymak
>> http://web.uvic.ca/~jklymak/
>>
>>
>>
>>
>>
>> _______________________________________________
> Matplotlib-devel mailing list
> Matplotlib-devel at python.org
> https://mail.python.org/mailman/listinfo/matplotlib-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/matplotlib-devel/attachments/20190115/b39756fd/attachment-0001.html>