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

Manage PR/Issues that are stale automatically #8706

Merged

Conversation

Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented May 20, 2023

Type of Changes

Type
✨ New feature

Description

Some issues can be managed automatically, and free some time for maintainers. See https://github.com/actions/stale for reference.

@Pierre-Sassoulas Pierre-Sassoulas added Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news 🔇 This change does not require a changelog entry labels May 20, 2023
@codecov
Copy link

codecov bot commented May 20, 2023

Codecov Report

Merging #8706 (33901ed) into main (9a2f113) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8706   +/-   ##
=======================================
  Coverage   95.81%   95.82%           
=======================================
  Files         172      173    +1     
  Lines       18320    18375   +55     
=======================================
+ Hits        17553    17607   +54     
- Misses        767      768    +1     

see 4 files with indirect coverage changes

@github-actions

This comment has been minimized.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Never a big fan of these personally. Waiting on author can still contain a very valid PR that we don't want to lose in the closed list, same goes for some of the other labels.
Hard coding this (imo) often gives more churn as you need to keep telling the bot it shouldn't close certain stuff.

Are we really spending so much time closing old issues/PRs that we need a bot for it?

@Pierre-Sassoulas
Copy link
Member Author

Are we really spending so much time closing old issues/PRs that we need a bot for it?

I feel like this is taking a lot of time especially if we need to keep track of what needs to be closed and craft polite messages asking authors if they still want to participate. (There's 700+ issues and almost 50 PR).

you need to keep telling the bot it shouldn't close certain stuff.

Unless the issue/PR is labeled "Waiting on author", "Cannot reproduce 🤷", "python past end of life", "Won't fix/not planned", "Needs take over" or "Work in progress", you won't have to do that. And if the issue is correctly labelled this way, you're not the one supposed to react: the author is. If no one bother it means the issue can be closed. If something was labelled incorrectly then you can change the label : it's (a lot) less work then checking all the issues and asking authors of stale issue if they are still interested in working on it.

@DanielNoord
Copy link
Collaborator

Are we really spending so much time closing old issues/PRs that we need a bot for it?

I feel like this is taking a lot of time especially if we need to keep track of what needs to be closed and craft polite messages asking authors if they still want to participate. (There's 700+ issues and almost 50 PR).

But how often do we do this? I don't really mind the current situation.

you need to keep telling the bot it shouldn't close certain stuff.

Unless the issue/PR is labeled "Waiting on author", "Cannot reproduce 🤷", "python past end of life", "Won't fix/not planned", "Needs take over" or "Work in progress", you won't have to do that. And if the issue is correctly labelled this way, you're not the one supposed to react: the author is. If no one bother it means the issue can be closed. If something was labelled incorrectly then you can change the label : it's (a lot) less work then checking all the issues and asking authors of stale issue if they are still interested in working on it.

The fact that an author is no longer responsive doesn't mean the PR isn't valuable. There are still some PR's here that would be really great to get back to, but we just haven't found the them. Closing them indicates that we don't want pursue them, which (imo) is not true. I also have PR's open that would be closed by this which I would then need to re-open every month to make sure we don't forget about them.

What about just closing issues/PR's you think are stale? If the author doesn't agree they can reopen themselves but at least they are dealing with a human maintainer and not a hard-coded bot.

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented May 20, 2023

But how often do we do this? I don't really mind the current situation.

I just stated that it feels like a lot of time to me.

The fact that an author is no longer responsive doesn't mean the PR isn't valuable. There are still some PR's here that would be really great to get back to, but we just haven't found the them. Closing them indicates that we don't want pursue them, which (imo) is not true. I also have PR's open that would be closed by this which I would then need to re-open every month to make sure we don't forget about them.

I think you're arguing against a bad bot that hurt you in the past 😄 But this is not what we have here. The fact that the author is no longer responsive is not the only criterion to close. The label applied by a maintainer are necessary too. I'm notified for every events in the repo : if the bot comment on a stale issues or PR, I'll see it and I can make a decision to act or not. This automation simply permits to automatically check a small set of specific issues that require input from someone that might be uninterested (or dead) periodically without us having to do it manually to be able to cleanup the enormous issue/PR list we have.

What about just closing issues/PR's you think are stale? If the author doesn't agree they can reopen themselves but at least they are dealing with a human maintainer and not a hard-coded bot.

No they can't reopen, they would need to open a new issue / PR or argue with a maintainer to please reopen the closed one. So right now I often ask a question, do not close because I don't want the author to have to open a new duplicate issue if I was wrong, then I forget about the issue and it never get closed. The bot would also automatically unstale the issue if the author comment on it without having to argue about reopening and depending on a maintainer's time to do it (or a maintainer can change the label).

On top of the fact that it's better for user, doing it manually we would have to think about cleaning up the issue from time to time, select an implicit list of labels (that I thought about and made explicit in this PR, and that are probably going to take some reviewing before being correct), examine each issues / PR that match, craft a message to the author asking for updates, deal with the answers or no answer later... Which is something we do currently but it's a lot of mental load and it's a bot job imo. I'll happily just apply and remove labels when the bot send a message instead of having to keep that in mind.

Let's please discuss the label applied on the issue/PR by a maintainer. For example maybe if a PR is not taken over after 16 weeks we don't want to close it after all, we might never want to close it. Or maybe if no one want to work on it it's not important enough to be kept and clutter our PR list.

@DanielNoord
Copy link
Collaborator

No they can't reopen, they would need to open a new issue

Can't users reopen issues? Anyway, arguing with a maintainer doesn't sound that bad. We all make mistakes and closes issues too soon.

The bot would also automatically unstale the issue if the author comment on it without having to argue about reopening and depending on a maintainer's time to do it (or a maintainer can change the label).

This is exactly what I don't like about this particular bot. Many repositories that use it just have constant streams of "bad bot" comments on their issues that drown out the actual content of the discussion.

Which is something we do currently but it's a lot of mental load and it's a bot job imo.

I think after the last clean up the current state of the issues is quite good? There are just a lot of issues with pylint 😄

I'll happily just apply and remove labels when the bot send a message instead of having to keep that in mind.

But this bot also auto closes stuff right? It is not just a message that you and I will both get but if we don't respond in time the user will face the same issue as they would if we closed it more pro-actively.

Or maybe if no one want to work on it it's not important enough to be kept and clutter our PR list.

Or maybe nobody found the time? It feels like a waste to close work that is progress towards a solution just because we don't want a cluttered list.

I really wonder how much of the maintainers work in this repository would really be automated by this. A lot of stuff comes down to testing yourself, taking over PRs or triaging successfully.
The only thing this bot automates is (unsurprisingly) closing stale issues/PRs). We tend to not do so very often as we value the work people put into PRs and can always work from them. I don't see a point in starting to do this more actively now. The current balance of closing stale stuff seems right to me.

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Let's make sure to take Pierre's concerns seriously. If he (or anyone who feels similarly) burns out, that's worse than any other outcome we're discussing on this thread.

I share Daniël's initial gut reaction. My experience sending PRs to CPython is that they wait years for a review. Marking them "stale" just confuses things, since it suggests someone is to blame (they're not) or that it's waiting on me (it's waiting on review, which is a different set of labels).

I'd be willing to try this out if we met in the middle somewhere, reducing the number of labels to just a few. I marked these below.

Many repositories that use it just have constant streams of "bad bot" comments on their issues that drown out the actual content of the discussion.

+1

Or maybe nobody found the time? It feels like a waste to close work that is progress towards a solution just because we don't want a cluttered list.

A closed PR is still a form of documentation. Good open source contributors when resuscitating an effort will check the closed PRs linked to the issue they're solving. If they're not doing that, as maintainers, we can point them toward that practice.

Unlike Daniël, I think we should be more aggressive about closing abandoned PRs. We can let the garden of issues grow forever, but I don't think the PRs should be treated as a garden. It should be an assembly line leading to reviews or wontfix. Seeing a long backlog of PRs can also discourage contributions. We can ping people once after some months to see if they forecast finding the time, and then close after another month or two.

In one instance I even wanted to close my own abandoned PR, but was asked to reopen it, but it became abandoned again. This is also a form of noise. We don't need to store people's personal to-do lists in a community space.

So right now I often ask a question, do not close because I don't want the author to have to open a new duplicate issue if I was wrong, then I forget about the issue and it never get closed.

I've found that a simple "please reopen if you can provide ... thanks." is the kind/effective way to touch it only once and discourage duplicates. Django does this, also.


days-before-pr-stale: 56
days-before-pr-close: 112
stale-pr-label: "Needs take over 🛎️"
Copy link
Member

Choose a reason for hiding this comment

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

Can we exclude PRs with a label of needs-review? I wouldn't be comfortable marking PRs stale without an initial review.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For me this label is really indicative of PRs which it would just be a shame to lose the time investment of. But if you really want to close these I'll give in.

Copy link
Member

Choose a reason for hiding this comment

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

I just don't see how it's a loss, it's in the repository permanently, and easily searchable with the label we've already given it.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like Pierre is suggesting not closing PRs for the moment, at least while we try this out. See the suggestion above to use -1 days.

@Pierre-Sassoulas
Copy link
Member Author

Can't we agree that asking for follow-up every x days and tracking issues labeled "Waiting for authors" or "Cannot reproduce" is not a good use of maintainer time ? If you did not provide a reproducer after 4 week + one week, you don't need a maintainer to whisper in your ear gently that your issue is going to be closed !

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented May 20, 2023

Our posts came at nearly the same instant, so forgive me if I'm replying before you had a chance to read mine. I think we can try the bot if start really small.

Can't we agree that asking for follow-up every x days and tracking issues labeled "Waiting for authors" or "Cannot reproduce" is not a good use of maintainer time ? If you did not provide a reproducer after 4 week + one week, you don't need a maintainer to whisper in your ear gently that your issue is going to be closed !

True, and though I'm open to doing this with a bot we can do this without one. "please reopen if ... / thanks" works well. Contributors that open duplicates instead after being guided toward reopening can be warned/blocked have their interactions limited.

@DanielNoord
Copy link
Collaborator

Let's make sure to take Pierre's concerns seriously. If he (or anyone who feels similarly) burns out, that's worse than any other outcome we're discussing on this thread.

Yes, sorry Pierre if I sounded dismissive. I hope you know I appreciate all the work you put in!

I'd be willing to try this out if we met in the middle somewhere, reducing the number of labels to just a few. I marked these below.

👍

Just to reiterate, I don't oppose closing issues and PRs that have become stale without asking for confirmation (and I think for some labels we could perhaps automate this), but I think it is just a much nicer experience, although more involved, if for most of these it is a manual step. That way the opener knows who to address if they don't agree instead of fighting with a bit.

Unlike Daniël, I think we should be more aggressive about closing abandoned PRs. We can let the garden of issues grow forever, but I don't think the PRs should be treated as a garden.

Fair enough, I don't necessarily agree with this but think this would a nice middle ground.

We don't need to store people's personal to-do lists in a community space.

I do think that we should then at least communicate that we agree with the approach in a PR before closing it. That way another maintainer knows that it was actually somewhat along the right path.

I've found that a simple "please reopen if you can provide ... thanks." is the kind/effective way to touch it only once and discourage duplicates. Django does this, also.

👍 to this approach. I think it is both welcoming but also clears up some of the backlog in an effective manner.

True, and though I'm open to doing this with a bot we can do this without one. "please reopen if ... / thanks" works well. Contributors that open duplicates instead after being guided toward reopening can be warned/blocked have their interactions limited.

Again, agree with Jacob here. I'm not opposed to closing PRs and issues (I mean I can re-open them if I want to 😄), I'm just not a big fan of automating this. I feel the bot makes the repository less approachable and you don't know who to ask to re-open. If we don't want to wait for responses indefinitely we could close the issue immediately for certain labels.

@Pierre-Sassoulas
Copy link
Member Author

I feel the bot makes the repository less approachable and you don't know who to ask to re-open. If we don't want to wait for responses indefinitely we could close the issue immediately for certain labels.

Quite the contrary, the bot state what needs to be done to reopen (comment on the issue, presumably with the reason why it's still a valid issue). So you know precisely what need to be done to not be closed. Asking for something and applying "Needs reproduction" or "Waiting on author" would be the equivalent of deciding right now to close the issue if the author did not bother in 5 weeks (or 16 weeks for a PR) and not think about it again. Having an issue be closed immediately without a convincing reason why just feel bad as a contributor.

@github-actions

This comment has been minimized.

Co-authored-by: Pierre Sassoulas <[email protected]>
Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Let's try this out and see how it goes.

@mbyrnepr2
Copy link
Member

Let's try this out and see how it goes.

Agreed. Could be quite handy!

mbyrnepr2
mbyrnepr2 previously approved these changes May 20, 2023
DanielNoord
DanielNoord previously approved these changes May 20, 2023
Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @Pierre-Sassoulas

Sorry if the tone of my initial comment made this discussion more heated than it had to be. I can see this is clear improvement for many of the labels included here!

@Pierre-Sassoulas Pierre-Sassoulas dismissed stale reviews from DanielNoord and mbyrnepr2 via 33901ed May 21, 2023 06:38
@Pierre-Sassoulas
Copy link
Member Author

Thank you all for the review :) I didn't mean to be too grumpy and prevent discussions, I only felt strongly about managing this automatically. There's going to be a follow-up after checking what the dry-run is doing (debug-only: true will need to be removed). We can also add or remove labels later ("python 3.8", "needs reproduction" ?) and I'm not opposed to closing stale MR just wanted to keep it consensual so we can use this asap as Daniel felt strongly about it and I don't.

@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 33901ed

@Pierre-Sassoulas Pierre-Sassoulas disabled auto-merge May 21, 2023 13:57
@Pierre-Sassoulas Pierre-Sassoulas merged commit 427c0ae into pylint-dev:main May 21, 2023
@Pierre-Sassoulas Pierre-Sassoulas deleted the fix-stale-automatically branch May 21, 2023 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news 🔇 This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants