Dear Sage developers, The conflicts we've seen in the last several months are multifaceted, but one of the central issues at hand is how we decide what code is incorporated into Sage through our review process. I have two goals for this thread: to describe our current standards (as codified in the Developer Guide <https://doc.sagemath.org/html/en/developer/review.html>) and ask for help in enforcing them, and to call a vote on several changes.
*Our current standards*: 1. A PR must be positively reviewed by another sage developer; there is no notion of negative review. If you come across a ticket that has positive review and you notice a problem that the reviewer missed, you have several options. First, you can just make a comment drawing attention to the matter. If the problem causes repeatable build/test failures, mathematically incorrect output or causes Sage's documentation to not build, you should change the label from Positive Review to Needs Work and make a comment describing the problem. If the author and reviewer do not agree that it is actually a problem, they should set the ticket back to Positive Review and you *should not engage more on that PR*. You are free to create another issue or PR addressing the problem that you have noticed. 2. We have recently started using Disputed as a tag for PRs where there is disagreement about how to proceed. This is not mentioned in the Developer Guide, and there has been no vote on sage-devel about how it should be used. But the purpose seems clear to me: it indicates the presence of disagreement. As such, once a PR has been marked Disputed, *do not remove this tag*: you cannot unilaterally decide that there is no controversy. 3. We have no set process for deciding whether a PR should be marked as a blocker; our guide currently just says "developers should use blocker priority sparingly and should indicate the rationale on the PR. Though there is no one fixed rule or authority that determines what is appropriate for blocker status, PRs introducing new features and PRs that make big changes are usually not blockers." I make some proposals below to clarify the situation, but in the meantime the sage-abuse committee will be enforcing the following standards to prevent label wars: *if there is disagreement about whether a PR should be marked as a blocker, mark it as critical instead*. All three of these standards have been frequently violated in the last several months. The sage-abuse committee intends to enforce them more vigorously; a consistent pattern of violations will result in the loss of Triage privileges (resulting in an inability to review PRs and change labels). If you notice violations of these standards, please draw them to the committee's attention by emailing sage-ab...@googlegroups.com. Proposals I have two separate proposals related to the above issues; feel free to vote on them separately. Voting will be open until Friday, March 8. 1. Using Github to vote on disputed PRs. Working things out amicably is preferable, and anyone is welcome to ask on sage-devel for more eyes on a PR. If you notice a serious issue with a PR, it is acceptable to change it to Needs Work (and make a comment!) as an initial step, but if the author/reviewer don't agree the process below should be followed instead. This process is intended as a lower-intensity method for resolving disagreements, and full votes on sage-devel override the process described below. a. When there is disagreement about whether a PR should be merged, anyone may mark a PR as disputed. b. There is no scheduled vote, but rather an ongoing poll based on opinions expressed by developers on the PR (these opinions can be expressed via previous positive reviews or explicit comments giving approval). The PR author is presumed to vote in favor; if they give up or no longer favor the PR they have the right to close the PR overall without any further voting. c. If the total number of positive votes is at least twice the number of negative votes, anyone involved may set the status to *positive review*; if the total number of positive votes is less than twice the number of negative votes, anyone involved may set the status to *needs review*. When either of these actions is taken, the person changing the status must list the people they are counting as positive and negative votes in a comment using @ mentions. d. The final decision on merging a disputed PR remains with the release manager, and we encourage the release manager to give enough time for everyone to express an opinion. 2. We use CIfix rather than Blocker to determine whether an open PR should be merged before running CI. The process below describes how to resolve disagreements about whether this label should be applied. a. Only PRs with positive review should be marked with the CIFix label. This should be done if both author and reviewer agree that it is appropriate, and a rationale should be given in a comment on the ticket. b. If a PR becomes disputed, The CIFix can be voted on separately upon request; otherwise it will just follow the same process described above. I'm sorry for abandoning this thread <https://groups.google.com/g/sage-devel/c/XON6NTJa33o> for the last few months, but hope that we can make some progress now. 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/CAChs6_k5QSCNFf_%2BuxN36n5GYoWgiVVQp5JdXdJxoWh%3DTW5_fQ%40mail.gmail.com.
