Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing the automatic size labeler #38334

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

tscrim
Copy link
Collaborator

@tscrim tscrim commented Jul 4, 2024

We revert #37262. This was voted on sage-devel: https://groups.google.com/g/sage-devel/c/3PLZD-4UFIA

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link
Member

@soehms soehms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should stop auto-labeling, as more people find it annoying than helpful. On the other hand, I agree with @videlec 's post in the Sage devel thread that it is not very welcoming to simply remove a new Sage developer's code. So my suggestion is to change the job's reaction events so that it still can be triggered manually:

diff --git a/.github/workflows/pr-labeler.yml b/.github/workflows/pr-labeler.yml
index 63f70bb653..ab5eaf5931 100644
--- a/.github/workflows/pr-labeler.yml
+++ b/.github/workflows/pr-labeler.yml
@@ -2,14 +2,8 @@
 # based on files edited and no of lines changed.
 name: Size Labeler / Checker
 on:
-  pull_request_target:
-    types:
-      - opened
-      - reopened
-      - synchronize
-      - ready_for_review
-      - review_requested
-      - edited
+  workflow_dispatch:
+    # Allow to run manually

Furthermore, I think it would be fair to involve the author of the code here, as well.

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 10, 2024

While I agree with you @soehms and with @videlec that it can be seen as unwelcoming to simply revert this, I find that argument to be a very weak reason to not revert it for a number of points. In particular, with broader input sought before merging, the original PR would have gotten to a point where it would have been seen by the community as net benefit. Nor do I think it is fair to tell someone that you cannot modify/remove someone’s code just because they are new. (For example, what if I want to completely rewrite how a function works after a new contributor just did the same?)

Additionally, we had a vote that included an option to keep it with manual inclusion (and exclusion), but the vote was clearly for removing it. I don’t think it is fair to wait around with this auto-checker trying to figure out a better solution. It will still be there in the git history if we want to revive it later.

One other point is the labels themselves will still be there for people to add manually. IMO we have enough clutter around already with GH’s various things, in addition to the Sage specific stuff; I don’t want to add more things for new contributors/reviewers to have to sort through with some extra button (if that is even possible).

@mkoeppe
Copy link

mkoeppe commented Jul 10, 2024

we have enough clutter around already with GH’s various things, in addition to the Sage specific stuff

Travis, what are you talking about?

It's really not possible to discussion Developer Experience issues on the level of such general rants. Honestly I find these claims that (1) the automatic labels somehow are harmful, (2) adding the mechanism would have required consultation, and (3) now require urgent action ("p: critical") to be removed -- absurd and disrespectful.

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 11, 2024

@mkoeppe I am sorry I have made you feel that way. We have a clear difference of opinion about this. I stand by my reasoning and opinion, and you are also entitled to your opinion on these matters. We can downgrade the priority to "p: major" if you think that is better, but I think we should enact the result of the vote as soon as possible because it is constantly affecting the development process.

@soehms
Copy link
Member

soehms commented Jul 11, 2024

Nor do I think it is fair to tell someone that you cannot modify/remove someone’s code just because they are new. (For example, what if I want to completely rewrite how a function works after a new contributor just did the same?)

I don't mean that you can't do this in general. It's a question of how you bring the new contributor along. If @amanmoon can follow your reasoning and agrees to remove his code, then I will agree too. If not, you should at least try to convince him.

@soehms
Copy link
Member

soehms commented Jul 11, 2024

Honestly I find these claims that (1) the automatic labels somehow are harmful, (2) adding the mechanism would have required consultation, and (3) now require urgent action ("p: critical") to be removed -- absurd and disrespectful.

Also the majority of people who voted to stop that automatic labeling would find it disrespectful if we left it as it is.

@amanmoon
Copy link
Contributor

amanmoon commented Jul 11, 2024

I feel there is nothing unwelcoming in reverting the changes. According to me the unwelcoming part shouldn't be one of the arguments here.

I am grateful to @soehms for reviewing the changes, I learnt a lot about GH workflows while creating the PR.

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 11, 2024

@amanmoon Thank you for your understanding. Thank you also for your efforts to try and improve the developer experience too.

@mkoeppe
Copy link

mkoeppe commented Jul 11, 2024

@mkoeppe I am sorry I have made you feel that way.

No, Travis, this is not about "feelings".

Public disrespect poisons our community, and moreover it is an instrument of bullying.
These are two reasons why your conduct is unacceptable.

We have a clear difference of opinion about this.

No, Travis, what is happening is not a difference of opinion.

What has happened here is something very problematic, and something that I have observed several times in the recent past -- that someone is trying to force their will by bringing their own personal version of project procedures. I've explained it already in detail in the sage-devel thread. I won't repeat it here, but you should re-read it and think about it.

The idea that this mechanism needs to be urgently reverted before even a "proper discussion" of it can happen -- that's just absurd.

@mkoeppe
Copy link

mkoeppe commented Jul 11, 2024

Also the majority of people who voted to stop that automatic labeling would find it disrespectful if we left it as it is.

@soehms Would they? Nobody is saying that the opinion of the voters should be ignored. And most people understand what a spoiled ballot is.

@soehms
Copy link
Member

soehms commented Jul 12, 2024

Also the majority of people who voted to stop that automatic labeling would find it disrespectful if we left it as it is.

@soehms Would they? Nobody is saying that the opinion of the voters should be ignored. And most people understand what a spoiled ballot is.

At least it is not clear to me whether we have rules to distinguish between valid and invalid votes. However, my impression is that we loop around in a chain of misunderstandings. Wouldn't it be best to reboot and start with a modified approach from the begining?

My suggestion:

  1. Approve this PR since @amanmoon has no objections.
  2. Reopen CI: Add pull request size labeler / checker #37254.
  3. Ask the community if we should implement Component auto-labeler for GitHub PRs #37373. This might be harder to implement, but I think it would be easier to communicate it as a useful feature.
  4. If Component auto-labeler for GitHub PRs #37373 is completed successfully, start the same procedure with CI: Add pull request size labeler / checker #37254.

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 12, 2024

I agree with @soehms’s approach. That was what I basically had in mind (the only difference being opening a new PR with the same data as #37254; either way is fine with me).

@mkoeppe
Copy link

mkoeppe commented Jul 17, 2024

There's no need to "reboot". How we discussed and implemented this DX feature was following best practices. People who have become newly interested in discussing DX matters should join the discussion in a normal way, not by alleging that something was done wrongly etc.

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 17, 2024

@soehms Could you please approve the PR so we can proceed? The community has made clear its opinion through the vote (and the original author has agreed to this); we do not require unanimous consent to do so.

@mkoeppe
Copy link

mkoeppe commented Jul 17, 2024

The community has made clear its opinion through the vote

The vote is invalid because of its obvious and severe violations of every principle of governance.

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 17, 2024

The community has made clear its opinion through the vote

The vote is invalid because of its obvious and severe violations of every principle of governance.

This is simply not true. We did have a discussion with a consensus forming, which then there was a vote which included the options presented in said discussion. In addition, I have very carefully again (re)read and thought your objections and still find them to be completely meritless in general. Furthermore, I will point out that you raised these objections during the vote and nobody else concurred with you. If you disagree with our processes, then you are free to try to change them.

I would also appreciate it if you could refrain from using charged language during these discussions as it does not help with making this into an inclusive and respectful community.

@soehms
Copy link
Member

soehms commented Jul 18, 2024

There's no need to "reboot". How we discussed and implemented this DX feature was following best practices. People who have become newly interested in discussing DX matters should join the discussion in a normal way, not by alleging that something was done wrongly etc.

What do you mean by DX-feature? I understand that people object to sudden changes that influence their daily work.

@soehms
Copy link
Member

soehms commented Jul 18, 2024

The vote is invalid because of its obvious and severe violations of every principle of governance.

I still don't understand this and I think all participants of the vote don't. Can you explain this in more detail and mention other community members who support your opinion?

@vbraun vbraun merged commit 8855e58 into sagemath:develop Jul 24, 2024
26 of 27 checks passed
@mkoeppe mkoeppe added this to the sage-10.5 milestone Jul 25, 2024
@nbruin
Copy link
Contributor

nbruin commented Jul 26, 2024

Because the discussion on this issue included concerns about conduct and procedure in the SageMath project, the Code of Conduct Committee believes it is important to clarify a few points for future reference.

This ticket was created to implement the decision of a vote conducted in https://groups.google.com/g/sage-devel/c/3PLZD-4UFIA
The thread was opened on 2024-06-12, with a call to vote and a reference to the thread https://groups.google.com/g/sage-devel/c/w4IeYgXgVUc/m/nV-ZT9BpAgAJ for discussion. Activity on that thread ran from 2024-05-06 to 2024-06-16. The original closing for the vote was announced as 2024-06-22, but the vote tally was made 2024-07-01 (prior to that, activity on the voting thread had ceased by 2024-06-16). The outcome of the vote was overwhelmingly in favour of one option.

This timeline is in line with how votes of impactful decisions are generally conducted and the end of discussion well before the closure of the vote indicates that all parties had ample opportunity to discuss their views and concerns.

Subsequently, issue #38334 was created to implement the favoured option. This is also in line with how generally votes are conducted (although usually the issue already exists before the need to vote becomes apparent).

We therefore see nothing unusual in how this change was discussed, voted on, or implemented and find any allegations otherwise unfounded.

The Sage Code of Conduct Committee.

@mkoeppe
Copy link

mkoeppe commented Jul 27, 2024

Making such "clarifications" is outside of the mandate of the Sage Code of Conduct committee.

@tscrim tscrim deleted the remove_size_autolabeler branch August 7, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants