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

Eric Firing efiring at hawaii.edu
Tue Jan 15 16:36:06 EST 2019


Your examples illustrate that the problem is often that we don't have 
enough people who understand some areas, like fonts and pdf/ps file 
formats, to get 2 thorough and knowledgeable reviews in a reasonable time.

I'm fine with your proposed rule.  It should help at least a little bit.


On 2019/01/15 9:11 AM, Antony Lee wrote:
> 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 <mailto: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
>     <mailto:jklymak at uvic.ca>> wrote:
>>         On 15 Jan 2019, at 10:20, Paul Hobson <pmhobson at gmail.com
>>         <mailto: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 <mailto: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 <mailto: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
>>                 <mailto:Matplotlib-devel at python.org>
>>                 https://mail.python.org/mailman/listinfo/matplotlib-devel
>>             _______________________________________________
>>             Matplotlib-devel mailing list
>>             Matplotlib-devel at python.org
>>             <mailto:Matplotlib-devel at python.org>
>>             https://mail.python.org/mailman/listinfo/matplotlib-devel
>>         _______________________________________________
>>         Matplotlib-devel mailing list
>>         Matplotlib-devel at python.org <mailto: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 <mailto: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

More information about the Matplotlib-devel mailing list