On Thu, Apr 18, 2024 at 1:51 PM Matthias Koeppe <matthiaskoe...@gmail.com> wrote:
> David, none of this explains the misleading use of the word "unreviewed". I believe that it does. If there was confusion, hopefully this exchange can help clarify it for others. David > On Thursday, April 18, 2024 at 10:47:36 AM UTC-7 David Roe wrote: > >> On Thu, Apr 18, 2024 at 1:43 PM Matthias Koeppe <matthia...@gmail.com> >> wrote: >> >>> I will first note that the title of this post is misleading. >>> Everything that was merged has been reviewed -- as noted, many months >>> ago. >>> >> >> I agree that everything was reviewed. However review refers not only to >> the action of giving approval (which was done), but also to the status of >> the PR as indicated by Positive Review, Needs Review, and Needs Work >> labels. This is the standard used by the release management scripts, as >> well as our community understanding of what it means for a PR to be ready >> for merging. Under this definition, #36951 and #36676 did not have >> positive review at the time that #36964 was merged. >> David >> >> On Thursday, April 18, 2024 at 8:54:26 AM UTC-7 David Roe wrote: >>> >>>> Hi all, >>>> Sage has had a review process for over 15 years, but a combination of >>>> recent changes has led to the merging of a PR into sage-10.4.beta3 of a >>>> change (#36964 <https://github.com/sagemath/sage/pull/36964>) that I >>>> believe should not (yet) have been merged. In #37796 >>>> <https://github.com/sagemath/sage/pull/37796> I created a PR to revert >>>> the change, which was opposed by the author of the original change. After >>>> some >>>> voting >>>> <https://github.com/sagemath/sage/pull/37796#issuecomment-2053675535> >>>> using the disputed PR policy >>>> <https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ/m/kvmOlVb1AQAJ>, >>>> Matthias has asked >>>> <https://github.com/sagemath/sage/pull/37796#issuecomment-2061926393> >>>> for a vote on sage-devel about this reversion, in accordance with the >>>> section that "This process is intended as a lower-intensity method for >>>> resolving disagreements, and full votes on sage-devel override the process >>>> described below." I am therefore asking you to vote (+1 means merge >>>> #37796 <https://github.com/sagemath/sage/pull/37796> in order to >>>> revert #36964 <https://github.com/sagemath/sage/pull/36964>). >>>> >>>> First, here are the relevant parts of the history of this particular >>>> change: >>>> >>>> - #36964 <https://github.com/sagemath/sage/pull/36964> was created on >>>> December 25 by Matthias, positively reviewed >>>> <https://github.com/sagemath/sage/pull/36964#pullrequestreview-1796972215> >>>> by Kwankyu on Decemebr 27, disputed, received enough votes >>>> <https://github.com/sagemath/sage/pull/36964#issuecomment-2041646521> >>>> to get a positive review on April 7, and was merged >>>> <https://github.com/sagemath/sage/pull/36964#issuecomment-2053520605> >>>> by Volker on April 12. It had dependencies: #37667, >>>> <https://github.com/sagemath/sage/pull/37667>#36951 >>>> <https://github.com/sagemath/sage/pull/36951>, and #36676 >>>> <https://github.com/sagemath/sage/pull/36676>. While #37667 >>>> <https://github.com/sagemath/sage/pull/37667> had positive review and >>>> was already been merged, the other two were still disputed: they had >>>> received an initial positive review but others objected and discussion was >>>> ongoing. >>>> >>>> - #37667 <https://github.com/sagemath/sage/pull/37667> is not disputed. >>>> >>>> - #36951 <https://github.com/sagemath/sage/pull/36951> was created on >>>> December 23 by Matthias, positively reviewed >>>> <https://github.com/sagemath/sage/pull/36951#pullrequestreview-1799928234> >>>> by Kwankyu on January 1, disputed, received enough votes >>>> <https://github.com/sagemath/sage/pull/36951#issuecomment-2041636273> >>>> (3-1) to change to positive review on April 7, had a clarification to bring >>>> back to (3-2) and remove positive review, then was included in the merge of >>>> #36964 <https://github.com/sagemath/sage/pull/36964>. On April 13, >>>> John Palmieri voted in favor >>>> <https://github.com/sagemath/sage/pull/36951#issuecomment-2053686090>, >>>> so the current vote stands at 4-2, enough for the 2-1 threshold in order to >>>> get positive review under the disputed voting process. >>>> >>>> - #36676 <https://github.com/sagemath/sage/pull/36676> was created on >>>> November 8 by Matthias, positively reviewed >>>> <https://github.com/sagemath/sage/pull/36676#issuecomment-1813306867> >>>> by John Palmieri on November 15, and then disputed. The most recent count >>>> was 6-4 in favor >>>> <https://github.com/sagemath/sage/pull/36676#issuecomment-2050362637> >>>> (falling short of the 2-1 ratio needed under the disputed voting process); >>>> since then I voted >>>> <https://github.com/sagemath/sage/pull/36676#issuecomment-2050531437> >>>> in favor, it was included in the merge of #36964 >>>> <https://github.com/sagemath/sage/pull/36964>, and then Martin voted >>>> against. >>>> >>>> >>>> At issue is the PR #36676 <https://github.com/sagemath/sage/pull/36676>, >>>> where discussion was still ongoing when #36964 >>>> <https://github.com/sagemath/sage/pull/36964> was merged. The >>>> reversion of this PR proposed is purely for process reasons (I voted in >>>> favor of #36676 <https://github.com/sagemath/sage/pull/36676> before >>>> all this happened!). The 5 Sage developers opposed to #36676 >>>> <https://github.com/sagemath/sage/pull/36676> deserve to have our >>>> processes followed. What went wrong? >>>> >>>> I think what happened resulted from a combination of the new disputed >>>> voting process, mismatched expectations around dependencies after the move >>>> to github, and Volker's release management scripts. Several developers >>>> privately expressed concern prior to this merge about exactly this outcome, >>>> and I reassured them that dependencies would be taken into account. >>>> Unfortunately, dependencies are now (unlike in trac) just a text section of >>>> the PR comment, and the release scripts only see the label. >>>> >>>> >>>> There are lots of things to discuss around this chain of events. I ask >>>> that everyone keep this thread focused on whether to merge #37796 >>>> <https://github.com/sagemath/sage/pull/37796> in order to revert #36964 >>>> <https://github.com/sagemath/sage/pull/36964>. Some other topics, and >>>> places I suggest for discussing them: >>>> - Ways to improve or eliminate the disputed voting process: I suggest >>>> Dima's recent thread >>>> <https://groups.google.com/g/sage-devel/c/1eLrTCa7tVA>. >>>> - The merits of #36676 <https://github.com/sagemath/sage/pull/36676>: >>>> I suggest discussing this either in the comments on that PR, or starting a >>>> new sage-devel topic if you have broader changes to raise about sage >>>> development. >>>> - Broader discussion of technical differences or philosophy: start a >>>> new thread. >>>> >>>> I suggest a deadline of Sunday April 21 at 23:59 US/Pacific for this >>>> vote. >>>> >>>> Finally, many of these PRs have been plagued by conflict and >>>> inappropriate language. Please, keep comments friendly in this discussion. >>>> David >>>> >>> -- >>> >> You received this message because you are subscribed to the Google Groups >>> "sage-devel" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to sage-devel+...@googlegroups.com. >>> >> To view this discussion on the web visit >>> https://groups.google.com/d/msgid/sage-devel/8b29f6b0-d5c5-416a-8d93-362af4247a59n%40googlegroups.com >>> <https://groups.google.com/d/msgid/sage-devel/8b29f6b0-d5c5-416a-8d93-362af4247a59n%40googlegroups.com?utm_medium=email&utm_source=footer> >>> . >>> >> -- > You received this message because you are subscribed to the Google Groups > "sage-devel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to sage-devel+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/sage-devel/86d47685-91de-4173-9c2c-386b3599de1en%40googlegroups.com > <https://groups.google.com/d/msgid/sage-devel/86d47685-91de-4173-9c2c-386b3599de1en%40googlegroups.com?utm_medium=email&utm_source=footer> > . > -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/CAChs6_mMgeUr7pJZyapCp_SrWpUtwzLa1o48umuUXe6VcnnW6g%40mail.gmail.com.
