New labels v: mimimal, v: small ... on pull requests

[seb....@gmail.com]

Dear Sage developers,

You may have noticed that since yesterday a new type of labels with the `v:` prefix has appeared on our PRs. These are automatically set to classify PRs based on their size. For more information, see #37262.

Sebastian

[Travis Scrimshaw]

I am *very* strongly opposed to these tags. Their cutoffs are arbitrary nor they serve no useful purpose as far as I can tell. To this point, they do not reflect the difficulty of a review; in fact, they are at best counterproductive to finding reviewers because it might deter people from reviewing "large" or "huge" changes as they can include lots of trivial doctest changes. At best it is just additional clutter in all of the information for PRs.

From a community perspective, I feel such changes should have been brought to the attention of sage-devel once the PR was at a positive review. Specifically, before the PR was merged. Not everyone has time to read every PR, and a small consensus of developers might not reflect the development community at-large when making changes like this.

Best,

Travis

[Vincent Delecroix]

I fully agree with Travis. I do not see the added value of these
additional tags.

> --
> 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/a1904934-a31f-4ce0-83c2-76cf2d1a70f1n%40googlegroups.com.

[seb....@gmail.com]

I must confess that I have not thought about the aspects of these labels that Travis points out, but I fully understand these concerns. If they are annoying for many developers, the feature can be easily disabled by removing corresponding variables from the repository.

[Dima Pasechnik]

On 9 May 2024 22:46:17 BST, Travis Scrimshaw <tcsc...@gmail.com> wrote:
>I am *very* strongly opposed to these tags. Their cutoffs are arbitrary nor
>they serve no useful purpose as far as I can tell. To this point, they do
>not reflect the difficulty of a review; in fact, they are at best
>counterproductive to finding reviewers because it might deter people from
>reviewing "large" or "huge" changes as they can include lots of trivial
>doctest changes. At best it is just additional clutter in all of the
>information for PRs.

It's also discouraging word, "minimal" - you do a "minimal" PR like <https://github.com/sagemath/sage/pull/37951>
- which potentially implies that we have thousands of missing "volatile" declarations in Cython code interfacing libgap, libsingular, and other libraries using setjmp/longjmp mechanics to gracefully process errors.

>
>From a community perspective, I feel such changes should have been brought
>to the attention of sage-devel once the PR was at a positive review.

>Specifically, *before* the PR was merged. Not everyone has time to read

>every PR, and a small consensus of developers might not reflect the
>development community at-large when making changes like this.
>
>Best,
>Travis
>
>
>On Tuesday, May 7, 2024 at 3:12:27 PM UTC+9 seb....@gmail.com wrote:
>
>> Dear Sage developers,
>>
>> You may have noticed that since yesterday a new type of labels with the
>> `v:` prefix has appeared on our PRs. These are automatically set to
>> classify PRs based on their size. For more information, see #37262

>> <https://github.com/sagemath/sage/pull/37262>.
>>
>> Sebastian
>>
>

[Matthias Koeppe]

FWIW, I suggested to implement this feature in https://github.com/sagemath/sage/issues/37254; I'm thankful to Aman Moon for implementing this feature and Sebastian Oehms for his help with it. 

Obviously a metric such as the number of lines of changes is only a one-dimensional way to express the complexity of a PR. 

When I suggested the feature, I explained the possible positive effects:
- A size label "tiny" could encourage quick reviews of trivial changes.
- A size label "huge" could help flag problematic PRs.

Personally I think that the size labels for "medium-sized" PRs do not add much and could be removed.

But I'll note that "developer experience" improvements like this one are really best developed exactly as it was done here: By deploying them early, the developer community can gain concrete experience with them -- and then suggest and implement refinements based on the experience. Harsh dismissals of the whole features, on the other hand, are not very helpful.

Matthias

[Travis Scrimshaw]

That model is not how we have worked as a community, nor do I think it is a productive way to run a smaller developer community such as ours. Doing things that way gives me a "all animals are equal, but some animals are more equal than others" feeling.

If you want to deploy things quickly, make them optional that those who want to use them can try them out to gain experience. If it is useful, then people will naturally take it up and want it to be standard. However, I still want this removed outright from the experience I already have from seeing it, and I see no way to gain any benefits. At the very least, can we agree to not make these labels automatically added. Let those who want to use them use them to see if people find a benefit to them.

Travis

[Matthias Koeppe]

On Sunday, May 12, 2024 at 6:50:05 PM UTC-7 Travis Scrimshaw wrote:

That model is not how we have worked as a community, nor do I think it is a productive way to run a smaller developer community such as ours.

I'm not sure what your reference point may be for this description, but in my experience it has not been remotely like this ever since I started contributing to Sage (2015). The project's infrastructure (in the past, for example Trac or the patchbot) has been run by volunteer administrators; the wiki page https://github.com/sagemath/sage/wiki/Infrastructure is intended to keep track of it (though much info is likely outdated), and there is a closed mailing list "sagemath-admins" for those involved. 

Perhaps a good reference: https://www.redhat.com/en/blog/understanding-open-source-governance-models

[Dima Pasechnik]

It does not look Sage clearly fits into one of their types. Anyhow,
RedHat (wholly owned by IBM nowadays) is known to be subverting open
source in general,
and GPL in particular, for years. I would take whatever they write on
this topic with a very big grain of salt, as it might be merely
creating more open-source FUD.
Cf. https://sfconservancy.org/blog/2023/jun/23/rhel-gpl-analysis/

Dima

>
> --
> 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/e0c30573-7e4b-4991-a70f-53659d268571n%40googlegroups.com.

[Travis Scrimshaw]

On Tuesday, May 14, 2024 at 9:48:32 AM UTC+9 Matthias Koeppe wrote:

On Sunday, May 12, 2024 at 6:50:05 PM UTC-7 Travis Scrimshaw wrote:

That model is not how we have worked as a community, nor do I think it is a productive way to run a smaller developer community such as ours.

I'm not sure what your reference point may be for this description, but in my experience it has not been remotely like this ever since I started contributing to Sage (2015). The project's infrastructure (in the past, for example Trac or the patchbot) has been run by volunteer administrators; the wiki page https://github.com/sagemath/sage/wiki/Infrastructure is intended to keep track of it (though much info is likely outdated), and there is a closed mailing list "sagemath-admins" for those involved. 

Maintaining the infrastructure, yes. However, the way it interacts with the developers and the things that are forced upon the developers. Things that affect everyone have traditionally gotten at least a post on sage-devel notifying about the change before it is accepted, with potentially an exception for those from our BDFL. The issue at hand is not some background maintenance or something that can be ignored if desired.

To this point, I cannot (effectively) disable it because the bot automatically is adding it every time a change occurs.

Perhaps a good reference: https://www.redhat.com/en/blog/understanding-open-source-governance-models

That's not relevant to how we do things. You could argue that we should follow one of those, but that is separate.

Best,

Travis

[Matthias Koeppe]

On Friday, May 10, 2024 at 7:01:43 AM UTC-7 Dima Pasechnik wrote:

It's also discouraging word, "minimal" 

I'm also not happy about the word "minimal", but simply because it's too close to "minor", which appears in priority labels.

[Travis Scrimshaw]

Another data point: the bot is getting the size wrong too:

https://github.com/sagemath/sage/pull/38105

I *very* strongly believe we should disable this automatically being added to PRs.

Travis

[Kwankyu Lee]

I *very* strongly believe we should disable this automatically being added to PRs.

How about having a voting for this issue?

 

[Matthias Koeppe]

On Tuesday, May 28, 2024 at 1:24:17 AM UTC-7 Travis Scrimshaw wrote:

Another data point: the bot is getting the size wrong too:

https://github.com/sagemath/sage/pull/38105

Thanks, I've let the author know in https://github.com/sagemath/sage/pull/37262#issuecomment-2135699139

 

[Matthias Koeppe]

I'll expand a little bit, in the hope to stimulate a constructive discussion. (A previous related thread: https://groups.google.com/g/sage-devel/c/sulCa-6EZRA/m/86jFAw9NAAAJ)

There is a very serious, project-level concern: Is our project welcoming to new contributors? 

We tell contributors to get started by preparing small simple PRs; but are these PRs getting reviewed and merged?

We currently have 177 open, non-draft, non-"positive review", non-"needs work", non-"needs info" pull requests.

https://github.com/sagemath/sage/pulls?q=is%3Aopen+is%3Apr+-label%3A%22s%3A+positive+review%22+-label%3A%22s%3A+needs+work%22+-label%3A%22s%3A+needs+info%22+draft%3Afalse

(Note that only members of the Triage team can set the "needs review" label.)

What can we do to make sure that PRs get reviews?

I frequently set component labels ("c: ...") on other people's unlabeled Issues and PRs. Does this help at all?

What labels would help people discover PRs that they would be able to review?

Matthias

[Matthias Koeppe]

His PR https://github.com/sagemath/sage/pull/38114 fixes the size computation for PRs that are based on an out-of-date base branch.

[seb....@gmail.com]

> (Note that only members of the Triage team can set the "needs review" label.)

See this comment in #35927 for a suggestion to solve this.

[seb....@gmail.com]

> How about having a voting for this issue?

Another possibility would be to have a blacklist (but I don't know where) that developers can sign up to if they want to be excluded from automations that apply to PRs. In this thread's example, a developer would exclude PRs that he himself authors or reviews from having the `v: ...` labels automatically set.

[Travis Scrimshaw]

Or instead you have a whitelist for those that want it.

I think at least one of these changes (whitelist or blacklist) should just be done in the same general way that these labels were introduced: no vote and just do it. This really should have had a vote, or at least a notification on sage-devel, before it was merged, so doing such a change would bring us back to where we should have been in the first place.

Best,

Travis

[Matthias Koeppe]

You may want to take a second look. This article does not prescribe one governance model. It describes several governance models. After reading it, a more informed discussion will be possible among those who care about questions of governance.

[Kwankyu Lee]

You may want to take a second look. This article does not prescribe one governance model. It describes several governance models. After reading it, a more informed discussion will be possible among those who care about questions of governance.

Indeed, the article offers some useful terms. Our governance model is a mixture of "do-ocracy", "council or board", "electoral" (it was "founder-leader" at the beginning). 

[Travis Scrimshaw]

The vote is now https://groups.google.com/g/sage-devel/c/3PLZD-4UFIA/m/bRgiaa11AQAJ

Although, again, the original adding of labels should have also had a vote...

On Wednesday, June 12, 2024 at 11:05:16 AM UTC+9 Kwankyu Lee wrote:

You may want to take a second look. This article does not prescribe one governance model. It describes several governance models. After reading it, a more informed discussion will be possible among those who care about questions of governance.

Indeed, the article offers some useful terms. Our governance model is a mixture of "do-ocracy", "council or board", "electoral" (it was "founder-leader" at the beginning). 

Putting some terms on it doesn't change how we have done things. That article is still a red herring.

Best,
Travis

 

[seb....@gmail.com]

I think that regardless of questions about governance models, we should take objections from community members seriously if they find something annoying. So why not offer them the possibility to set preferences for extensions to tools we have implemented ourselves? This has no impact on the goals of the project.

[Matthias Koeppe]

On Thursday, June 13, 2024 at 11:46:59 PM UTC-7 seb....@gmail.com wrote:

I think that regardless of questions about governance models, we should take objections from community members seriously if they find something annoying.

Of course. Open source projects stand and fall with participation. 

But participation comes in degrees. Expressing that one is annoyed, or saying that one is "for" or "against" something -- well, that's some participation and better than no participation at all.

Here a development infrastructure feature was designed and implemented by a new contributor. This rare engagement with the development infrastructure of the project is something that our community should welcome and encourage -- by engaging with the details of the feature in a discussion (I have offered several prompts in this direction).

[Kwankyu Lee]

Here a development infrastructure feature was designed and implemented by a new contributor. This rare engagement with the development infrastructure of the project is something that our community should welcome and encourage 

The feature was introduced through the normal process. At lease three people welcomed the new contributor. Claiming that there should have been a discussion in sage-devel before merging the PR is asking too much. There is no clearcut of what to discuss here or what not.

But now we are discussing about the feature, not about the PR of the new contributor. As we have some experiences with the feature, we can now discuss whether to keep it or not.

 

[Kwankyu Lee]

I agree with what Vincent said in the other thread. Thus for decision (A1), we just turn the script off so that  we could revive it in future for some other useful functionality.

One such functionality is to add component labels (aka "c:" labels) automatically by analyzing what part of sage codebase the PR branch touches.

[seb....@gmail.com]

> One such functionality is to add component labels (aka "c:" labels) automatically by analyzing what part of sage codebase the PR branch touches.

This is #37373. Temporarily this has been part of #37262. But finally it has been kept off.