On Thu, Apr 18, 2024 at 1:43 PM Matthias Koeppe <matthiaskoe...@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+unsubscr...@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/CAChs6_m9c6MsV6G5ducV%2BWzS7MJzR7FB6qUqMjjAsMt221fZ8A%40mail.gmail.com.
