Labels and Reviewing

[David Roe]

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) 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-...@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 for the last few months, but hope that we can make some progress now.

David

[Kwankyu Lee]

Thank you for making progress on these urgent issues. I suggest the following:

1. Open two other new threads, each of which is for voting on each proposal. 

2. On a proposal, it should be clear that a positive vote (+1) is for the whole proposal, and if one is negative to any part of the proposal, (s)he should give a negative vote (-1).  

3. A proposal is accepted if the number of positive votes is at least twice of the number of the negative votes.

A minor suggestion for Proposal 2: for the label to be readable, I suggest "CI fix" for the name of the label (a blank between words). 

We may use this thread to get more comments on the Proposals before opening voting threads.

[David Roe]

On Wed, Feb 28, 2024 at 1:01 AM Kwankyu Lee <ekwa...@gmail.com> wrote:

Thank you for making progress on these urgent issues. I suggest the following:

1. Open two other new threads, each of which is for voting on each proposal. 

2. On a proposal, it should be clear that a positive vote (+1) is for the whole proposal, and if one is negative to any part of the proposal, (s)he should give a negative vote (-1).  

Voting threads seem reasonable.  I'll wait a day or two to see if people have any final comments before voting.

 

3. A proposal is accepted if the number of positive votes is at least twice of the number of the negative votes.

Despite the fact that we're asking for this threshold in voting on a PR, the standard for votes on proposals on sage-devel is just a plain majority (though of course I hope that we can come to a 2-1 consensus!).

A minor suggestion for Proposal 2: for the label to be readable, I suggest "CI fix" for the name of the label (a blank between words). 

I'm happy to adjust the label to be "CI fix."

 

We may use this thread to get more comments on the Proposals before opening voting threads.

--
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/554961a0-4ace-4317-bfcf-55b6a128bcden%40googlegroups.com.

[Giacomo Pope]

Apologies for the basic question in this thread, but recently I have seen lots of conversation about the different labels and I want to clarify something for myself.

In the past few PR I have made for Sage, randomised testing has uncovered (usually) trivial bugs. I then write new PRs to fix these bugs.

If there is code causing CI failure in random testing, should I mark the fix for this as a "blocker", even if the chance of this failure is small? I don't want to be melodramatic in my PR for fixes but I also want to make sure I'm labelling things as expected,

[Kwankyu Lee]

If there is code causing CI failure in random testing, should I mark the fix for this as a "blocker", even if the chance of this failure is small?

No. A "blocker" blocks the release. We use it for serious fixes worth the delay. 

[Dima Pasechnik]

On Wed, Feb 28, 2024 at 11:29 AM Giacomo Pope <giaco...@gmail.com> wrote:

Apologies for the basic question in this thread, but recently I have seen lots of conversation about the different labels and I want to clarify something for myself.

In the past few PR I have made for Sage, randomised testing has uncovered (usually) trivial bugs. I then write new PRs to fix these bugs.

If there is code causing CI failure in random testing, should I mark the fix for this as a "blocker", even if the chance of this failure is small? I don't want to be melodramatic in my PR for fixes but I also want to make sure I'm labelling things as expected,

I don't think we ever tag anything but most onerous maths bugs as blockers

(e.g. we have a plenty of outstanding symbolic integration bugs).

That is, unless it's absolutely Earth-shuttering, don't use "blocker".

Dima

 

On Wednesday, February 28, 2024 at 6:08:20 AM UTC David Roe wrote:

On Wed, Feb 28, 2024 at 1:01 AM Kwankyu Lee <ekwa...@gmail.com> wrote:

Thank you for making progress on these urgent issues. I suggest the following:

1. Open two other new threads, each of which is for voting on each proposal. 

2. On a proposal, it should be clear that a positive vote (+1) is for the whole proposal, and if one is negative to any part of the proposal, (s)he should give a negative vote (-1).  

Voting threads seem reasonable.  I'll wait a day or two to see if people have any final comments before voting.

 

3. A proposal is accepted if the number of positive votes is at least twice of the number of the negative votes.

Despite the fact that we're asking for this threshold in voting on a PR, the standard for votes on proposals on sage-devel is just a plain majority (though of course I hope that we can come to a 2-1 consensus!).

A minor suggestion for Proposal 2: for the label to be readable, I suggest "CI fix" for the name of the label (a blank between words). 

I'm happy to adjust the label to be "CI fix."

 

We may use this thread to get more comments on the Proposals before opening voting threads.

--
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/554961a0-4ace-4317-bfcf-55b6a128bcden%40googlegroups.com.

--
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/b84b22d2-9b57-460c-9f8d-5f8ebe2f982en%40googlegroups.com.

[David Roe]

Addressing a comment from Travis on the voting thread:

"but might want to consider cases when its 2 vs 1 as requiring at least one other person involved. (Sorry for being late to realize this.)"

This border case was actually one of the reasons that I suggested a 2 to 1 threshold.  I think that if a single objector thinks that a PR should not proceed and the author and reviewer both think it should, the onus should be on the objector to find other developers who share their objections.

David

To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/CAAWYfq2eMM%3DzUHCZ_dpfDvxattotQmaD-0kht6J8ZxCL9K005w%40mail.gmail.com.

[Dima Pasechnik]

On Thu, Mar 7, 2024 at 8:39 PM David Roe <roed...@gmail.com> wrote:

Addressing a comment from Travis on the voting thread:

"but might want to consider cases when its 2 vs 1 as requiring at least one other person involved. (Sorry for being late to realize this.)"

This border case was actually one of the reasons that I suggested a 2 to 1 threshold.  I think that if a single objector thinks that a PR should not proceed and the author and reviewer both think it should, the onus should be on the objector to find other developers who share their objections.

As long as the issue of GitHub blocks is not resolved, this issue is moot. A developer can block their opponents

on GitHub to make sure there is not enough opposition to their PRs.

To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/CAChs6_nR1T2CtkD%2BEY7AHFDU5Y4TE0u1WUeWZ9crVkbhLt4%2Byg%40mail.gmail.com.

[Martin R]

I don't know exactly what case of blocking you have in mind, but I'm going to be bold and state that blocking among fellow developers (this is not about private messages, right?) cannot really be a solution, in my opinion:

* either there has been a substantial breach of conduct, in which case I think the person cannot be part of sagemath anymore,

* or otherwise, there shouldn't be a reason to block.

I hope that we are talking about the empty set.

Martin

[Dima Pasechnik]

On 8 March 2024 14:11:41 GMT, 'Martin R' via sage-devel <sage-...@googlegroups.com> wrote:
>I don't know exactly what case of blocking you have in mind,

on GitHub you can block a user - such a block prevents the blocked user from commenting and changing the status of your PRs and issues.

Currently there are a few Sage team members blocking each other.
I have made our CoC committee aware of this fact, but they are in no rush to rule on it, it seems.

> but I'm going
>to be bold and state that blocking among fellow developers (this is not
>about private messages, right?) cannot really be a solution, in my opinion:
>
>* either there has been a substantial breach of conduct, in which case I
>think the person cannot be part of sagemath anymore,
>* or otherwise, there shouldn't be a reason to block.
>
>I hope that we are talking about the empty set.
>
>Martin
>
>On Friday 8 March 2024 at 10:22:30 UTC+1 Dima Pasechnik wrote:
>
>> On Thu, Mar 7, 2024 at 8:39 PM David Roe <roed...@gmail.com> wrote:
>>
>>> Addressing a comment from Travis

>>> <https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ/m/CCKJ0dVCAAAJ> on

>>>>>>> 2. On a proposal, it should be clear that *a positive vote (+1) is
>>>>>>> for the whole proposal,* and if one is negative to any part of the

>>>>>>> proposal, (s)he should give a negative vote (-1).
>>>>>>>
>>>>>>
>>>>>> Voting threads seem reasonable. I'll wait a day or two to see if
>>>>>> people have any final comments before voting.
>>>>>>
>>>>>>
>>>>>>> 3. A proposal is accepted if the number of positive votes is at least
>>>>>>> twice of the number of the negative votes.
>>>>>>>
>>>>>>
>>>>>> Despite the fact that we're asking for this threshold in voting on a
>>>>>> PR, the standard for votes on proposals on sage-devel is just a plain
>>>>>> majority (though of course I hope that we can come to a 2-1 consensus!).
>>>>>>
>>>>>> A minor suggestion for Proposal 2: for the label to be readable, I
>>>>>>> suggest "CI fix" for the name of the label (a blank between words).
>>>>>>>
>>>>>>
>>>>>> I'm happy to adjust the label to be "CI fix."
>>>>>>
>>>>>>
>>>>>>> We may use this thread to get more comments on the Proposals before
>>>>>>> opening voting threads.
>>>>>>>
>>>>>>> --
>>>>>>> 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/554961a0-4ace-4317-bfcf-55b6a128bcden%40googlegroups.com

>>>>>>> <https://groups.google.com/d/msgid/sage-devel/554961a0-4ace-4317-bfcf-55b6a128bcden%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+...@googlegroups.com.
>>>>> To view this discussion on the web visit

>>>>> https://groups.google.com/d/msgid/sage-devel/b84b22d2-9b57-460c-9f8d-5f8ebe2f982en%40googlegroups.com
>>>>> <https://groups.google.com/d/msgid/sage-devel/b84b22d2-9b57-460c-9f8d-5f8ebe2f982en%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+...@googlegroups.com.
>>>> To view this discussion on the web visit

>>>> https://groups.google.com/d/msgid/sage-devel/CAAWYfq2eMM%3DzUHCZ_dpfDvxattotQmaD-0kht6J8ZxCL9K005w%40mail.gmail.com
>>>> <https://groups.google.com/d/msgid/sage-devel/CAAWYfq2eMM%3DzUHCZ_dpfDvxattotQmaD-0kht6J8ZxCL9K005w%40mail.gmail.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+...@googlegroups.com.
>>>
>> To view this discussion on the web visit

>>> https://groups.google.com/d/msgid/sage-devel/CAChs6_nR1T2CtkD%2BEY7AHFDU5Y4TE0u1WUeWZ9crVkbhLt4%2Byg%40mail.gmail.com
>>> <https://groups.google.com/d/msgid/sage-devel/CAChs6_nR1T2CtkD%2BEY7AHFDU5Y4TE0u1WUeWZ9crVkbhLt4%2Byg%40mail.gmail.com?utm_medium=email&utm_source=footer>
>>> .
>>>
>>
>

[David Roe]

Dear Sage developers,

Dima is correct that there are several developers who have blocked each other.  The Sage Code of Conduct Committee is aware of several cases and is working on resolving them.  We believe both that the presence of these blocks is harming the Sage project, and that it can be appropriate to block another user.  In an extreme example, if one user "A" was stalking or harassing another "B", it would absolutely be appropriate for B to block A. Any resulting harm to the project is completely the responsibility of A in such a situation. We are not saying that this situation applies for any of the existing blocks, but there will certainly be less extreme cases in which blocking remains appropriate.

In any case, the committee asks that anyone affected by blocking to please channel their understandable frustration towards patiently working with us to constructively resolve the current blocks individually and voluntarily.  We will continue to work toward addressing the underlying reasons (revisions to the code of conduct, recruiting new members, new policies on labels and reviewing, additional actions on github and in private).

Thanks,

David

for the Sage Code of Conduct Committee