VOTE: Revert merged PR with unreviewed dependencies

[David Roe]

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) that I believe should not (yet) have been merged.  In #37796 I created a PR to revert the change, which was opposed by the author of the original change.  After some voting using the disputed PR policy, Matthias has asked 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 in order to revert #36964).

First, here are the relevant parts of the history of this particular change:

- #36964 was created on December 25 by Matthias, positively reviewed by Kwankyu on Decemebr 27, disputed, received enough votes to get a positive review on April 7, and was merged by Volker on April 12.  It had dependencies: #37667, #36951, and #36676.  While #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 is not disputed.

- #36951 was created on December 23 by Matthias, positively reviewed by Kwankyu on January 1, disputed, received enough votes (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. On April 13, John Palmieri voted in favor, 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 was created on November 8 by Matthias, positively reviewed by John Palmieri on November 15, and then disputed.  The most recent count was 6-4 in favor (falling short of the 2-1 ratio needed under the disputed voting process); since then I voted in favor, it was included in the merge of #36964, and then Martin voted against.

At issue is the PR #36676, where discussion was still ongoing when #36964 was merged.  The reversion of this PR proposed is purely for process reasons (I voted in favor of #36676 before all this happened!).  The 5 Sage developers opposed to #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 in order to revert #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.

- The merits of #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

[Martin R]

+1, yes, the unintended merge should be reverted.

Martin

[Dima Pasechnik]

+1 to reverting the wrong merge

[David Joyner]

On Thu, Apr 18, 2024 at 11:54 AM David Roe <roed...@gmail.com> 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) that I believe should not (yet) have been merged. In #37796 I created a PR to revert the change, which was opposed by the author of the original change. After some voting using the disputed PR policy, Matthias has asked 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 in order to revert #36964).
>

+1

> First, here are the relevant parts of the history of this particular change:
>
> - #36964 was created on December 25 by Matthias, positively reviewed by Kwankyu on Decemebr 27, disputed, received enough votes to get a positive review on April 7, and was merged by Volker on April 12. It had dependencies: #37667, #36951, and #36676. While #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 is not disputed.
>
> - #36951 was created on December 23 by Matthias, positively reviewed by Kwankyu on January 1, disputed, received enough votes (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. On April 13, John Palmieri voted in favor, 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 was created on November 8 by Matthias, positively reviewed by John Palmieri on November 15, and then disputed. The most recent count was 6-4 in favor (falling short of the 2-1 ratio needed under the disputed voting process); since then I voted in favor, it was included in the merge of #36964, and then Martin voted against.
>
>
> At issue is the PR #36676, where discussion was still ongoing when #36964 was merged. The reversion of this PR proposed is purely for process reasons (I voted in favor of #36676 before all this happened!). The 5 Sage developers opposed to #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 in order to revert #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.
> - The merits of #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/CAChs6_%3Dj65YMtx%3DOEX7r5wUcRxr0iCk__0mbqJTKBZB5c_RoBQ%40mail.gmail.com.

[Matthias Koeppe]

I will first note that the title of this post is misleading.

Everything that was merged has been reviewed -- as noted, many months ago.

[David Roe]

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

--

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.

[Matthias Koeppe]

David, none of this explains the misleading use of the word "unreviewed".

[David Roe]

On Thu, Apr 18, 2024 at 1:51 PM Matthias Koeppe <matthia...@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

 

To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/86d47685-91de-4173-9c2c-386b3599de1en%40googlegroups.com.

[Nils Bruin]

+1 to merge #37796.

[Michael Orlitzky]

On Thu, 2024-04-18 at 11:54 -0400, David Roe wrote:
> 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>).

+1

[julian...@fsfe.org]

+1

[Marc Culler]

On Thursday, April 18, 2024 at 12:47:36 PM UTC-5 David Roe wrote:

On Thu, Apr 18, 2024 at 1:43 PM Matthias Koeppe 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. 

And that is why I vote -1 on reverting the merge.

- Marc

[Kwankyu Lee]

-1 to be in sync with my vote in #37796.

[G. M.-S.]

-1

If something has been done that should be undone, I very much trust Volker to take care of it when he can, without the need for endless time-consuming discussions and votes.

Best,

Guillermo

[Travis Scrimshaw]

+1 for merging #37796.

Volker, I would appreciate if you could say something about how #36964 was merged. It would be useful to understand the process with merging this, rather than guessing the intent. Additionally, I thought we didn't merge things when the dependencies have not been merged (or merged simultaneously)? (This is why I am voting for reverting.)

Best,

Travis

[Gareth Ma]

+1 to revert

--
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/67d98d5d-2121-44a9-9d99-a0f3cd7fbc67n%40googlegroups.com.

[Volker Braun]

It was merged because it was positively reviewed. 

Neither I nor the merge script reads every ticket description and looks through the text whether any dependency is mentioned that has not yet been reviewed. We can try to build such a Rube Goldberg machine, but I would very much argue against it. Just go with how github works, which is positive review = ready to merge and "files changed" shows the actual changes that this PR implements. Anything else will just prevent us from using standard tooling in the future. If anything introduce a "preliminary positive review" tag that gets replaced with actual positive review when the dependencies are in.

[David Roe]

The vote was 8-4.  I'm setting the PR back to positive review.

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/0d36cacd-8072-4092-b8c3-d165989bdaaan%40googlegroups.com.

[seb....@gmail.com]

> Just go with how github works, which is positive review = ready to merge and "files changed" shows the actual changes that this PR implements.

The problem with this is: if there are commits on a branch that are reviewed in more than one PR, the question is: does positive review mean all or some PR's? I don't think some is a good choice. Unfortunately, I don't have the time to read through all these controversial PR's at the moment. However, what I have seen is that every dispute started with one participant feeling ignored by others. Maybe this is enforced by written communication. But, if we allow a procedure that systematically ignores the opinions of some reviewers, we may be adding fuel to the fire.

Therefore I would have voted +1 if I hadn't missed the deadline. Like Travis, my vote is independent of the content of #36964.

[Volker Braun]

On Tuesday, April 23, 2024 at 6:24:51 PM UTC+2 seb....@gmail.com wrote:

The problem with this is: if there are commits on a branch that are reviewed in more than one PR, the question is: does positive review mean all or some PR's?

The review is for a single PR, not for individual commits.

The positively reviewed PR gets merged, and then you may or may not want to rebase other PRs on the new develop branch. If you do, then the changes that were merged disappear from the "files changed" tab of that other PR on github. Either way, the commits from the positively reviewed PR are added to the sage codebase.

[seb....@gmail.com]

Sure! But the fact was that reviewers objected to a commit in PR A that was approved in PR B, so their objection was simply ignored! Please don't understand me wrong. I'm not saying that it was your responsibility. It is still the responsibility of the reviewers to make sure that this does not happen. Probably this has become a bit harder since we moved to GitHub.