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

GitHub: auto set inactive label #25163

Merged
merged 1 commit into from
Feb 26, 2023
Merged

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Feb 6, 2023

PR Summary

Proposed stale action...

It labels PRs after 60 d, but never closes them.

It does close issues after 1 year with 30 day warning.

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@jklymak jklymak added the status: needs comment/discussion needs consensus on next step label Feb 6, 2023
@jklymak jklymak changed the title GitHub: inactive label GitHub: auto set inactive label Feb 6, 2023
@rcomer
Copy link
Member

rcomer commented Feb 6, 2023

Obviously I approve of this sort of thing, but I thought @melissawm was working on something?

@jklymak
Copy link
Member Author

jklymak commented Feb 6, 2023

I didn't see anything like that, but happy to close and let someone else put a PR in.

@oscargus
Copy link
Contributor

oscargus commented Feb 7, 2023

This would close like 1200 issues. Not saying that it is a bad idea, but some of those are clearly bugs or "surprising features".

Maybe we should have some labels that does not auto-close? Confirmed bug at least? 76 would be closed status: confirmed bug (at least based on opening date, probably a bit fewer in reality).

Edit: Also, I think that at least some of the feature requests should stay. Even though the easy ones often gets picked up quite quickly, it is a bit of a shame to lose good suggestions just because no one had time to implement them. (I think people may not really bother to reopen them.) The problem is most likely that we then would have to decide on "worthwhile feature requests"...

Btw, are these closed as "fixed" or "not planned"?

@rcomer
Copy link
Member

rcomer commented Feb 7, 2023

We currently have 871 open issues that have not been updated for a year of which 49 were confirmed bugs so this proposal would mark those as inactive. They would only be closed if nobody comments on them for a further month after they were labelled.

I expect that some of these bugs have actually been fixed, so I would not exempt this label. Rather, I would expect that someone who is watching the issue will see the notification from the bot's comment and check whether a recent mpl version still has the problem. If it does, they can comment to say so and the issue stays open for another year.

@jklymak
Copy link
Member Author

jklymak commented Feb 7, 2023

I think the same for feature requests - if anyone still wants the feature, they have 30 days to comment. We will also all see it in our notifications and can comment and perhaps reclassify or act on it as maintainers. If no one even comments, then the request probably wasn't that important or has workarounds.

@melissawm
Copy link
Contributor

Thanks @jklymak ! A couple of comments:

  • Can we add the timelines (and a mention about the action) to the documentation for PR reviewers?
  • For both actions, I think I'm still not sure on how we'll manage identifying if this is a PR that needs a review or that is pending an update from the author. Is there a plan for that? I would also suggest being more explicit in expectations to avoid people giving up on work entirely - which we want to avoid 😄

Since this Pull Request hasn't been updated in 60 days, it has been marked "inactive." This doesn't mean it will be closed, but it will help maintainers prioritize their work. You can pick it back up anytime - please ping us if you need a review or guidance to move the PR forward!

  • A bot that closes issues automatically is a pretty controversial topic in many communities (as we've discussed). I would try to make the messaging as friendly as possible to avoid pushing folks away. We want to clean up but also make sure we're not ignoring reports - many folks assume that if an issue is closed by a maintainer (or a bot) it means they should not be opening issues at all.

'This issue has been marked "inactive" because it has been 365 days since the last comment. If this issue is still present in recent Matplotlib releases, or the feature request is still wanted, please leave a comment and this label will be removed. If there are no updates in another 30 days, this issue will be automatically closed, but you are free to re-open or create a new issue if needed. Thanks!'

@jklymak
Copy link
Member Author

jklymak commented Feb 7, 2023

Sure that wording is great.

For issues, I think we could even be more explicit why we are doing this so people don't take offence. I added

We value issue reports, and this procedure is meant to help us resurface and prioritize issues that have not been addressed yet, not make them disappear.

See the uploaded commit with slight modifications to the above.

Also, note the time frames on these are just my first guess at what we want. Super easy to change... 30 d close seems long enough to catch anyone who went on vacation. 365 days seems like a good length of time to expect an issue to have been looked at and acted upon (somehow). PRs should really not sit around for two months, in my opinion at least, particularly if it is a non-core dev we should be making it clear they are welcome to ping us.

@rcomer
Copy link
Member

rcomer commented Feb 7, 2023

For PRs, I think it would be good to find a form of words that encourages the author to at least let us know whether they plan to continue working on it. I worry that "you can pick it up anytime" leaves it open to drift indefinitely.

@jklymak
Copy link
Member Author

jklymak commented Feb 7, 2023

" If you do not plan on continuing the work, please let us know and we will find either find someone to take the PR over or close it" ??

@story645
Copy link
Member

story645 commented Feb 7, 2023

If you do not plan on continuing the work, please let us know and we will find either find someone to take the PR over or close it

very slight tweak:

If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over or close it

@jklymak
Copy link
Member Author

jklymak commented Feb 7, 2023

Note that I've also reduced the operations per run to 5. The idea being that this will lead to fewer notifications per day, and as this comes on line we may actually respond to them. We may need to play with this or the cron frequency depending on how many notifications we get.

jobs:
stale:
runs-on: ubuntu-latest
steps:
Copy link
Member

Choose a reason for hiding this comment

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

can you also add an inactive project? my big reason for that is it lets us potentially mark up why stuff is stalled in a private way, which may be helpful for some more contentious stalled things. https://github.com/actions/add-to-project

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you follow up with that? I have no idea how to do that, and I think can be completely independent of this PR.

@rcomer
Copy link
Member

rcomer commented Feb 7, 2023

The action sifts through issues/PRs in chronological order and by default starts at the newest. I suggest setting ascending: true so it starts with the oldest where it will be more likely to find things to do.

It also doesn’t remember where it got to last time it ran, so I think after the first several runs it will use up its operations just re-checking issues/PRs that it already checked. So I think we would need to gradually increase the operations per run to help it get further.

@jklymak
Copy link
Member Author

jklymak commented Feb 7, 2023

It also doesn’t remember where it got to last time it ran, so I think after the first several runs it will use up its operations just re-checking issues/PRs that it already checked. So I think we would need to gradually increase the operations per run to help it get further.

I don't understand how it could possibly work then if it has no way of knowing what's already been done?? The default query is 30.

@story645
Copy link
Member

story645 commented Feb 7, 2023

Sorry @jklymak for editing your comment when I was trying to reply. Is there a way to filter so that the action is only run on issues not labeled inactive.

@jklymak
Copy link
Member Author

jklymak commented Feb 7, 2023

OK, I think it works by downloading issues/PRs 100 at a time, and then doing operations on them. I think each 100 takes an action. Then each operation takes an action. So we have about 2000 open issues+PRs, so that will use at least 20 actions leaving 10 actions to set labels and/or close things. So maybe that is not too bad left at 30?

@jklymak
Copy link
Member Author

jklymak commented Feb 7, 2023

Sorry @jklymak for editing your comment when I was trying to reply. Is there a way to filter so that the action is only run on issues not labeled inactive.

I'm pretty sure thats how it works.

@rcomer
Copy link
Member

rcomer commented Feb 7, 2023

If it finds the “inactive” label it then checks whether there was a comment since the label was added. If yes, it removes the label. If no, it checks whether the “days before close” time is up and closes if so.

Disclaimer: all my comments are based on having observed the behaviour in Iris - I haven’t actually studied its inner workings!

@jklymak
Copy link
Member Author

jklymak commented Feb 7, 2023

Btw, are these closed as "fixed" or "not planned"?

not-planned, by default...

@jklymak
Copy link
Member Author

jklymak commented Feb 7, 2023

actions/stale#792 seems relevant. Perhaps we need to do some of this offline first with a job that can store its state. I'm still not sure from this what constitutes an "action" on the GitHub API that counts against the counts.

https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#rate-limits-for-requests-from-github-actions seems to indicate that from GH actions we could use 1000/h. But if that is still trying to get through 2000 PRs/issues, it's still not going to succeed.

@rcomer
Copy link
Member

rcomer commented Feb 8, 2023

I also don't know what counts as an "operation", but maybe it doesn't matter if it can't get through all 2000: after the first month, it should start closing some, so then there are less than 2000 to get through. If we also want to keep notification frequency down, we probably don't want it to find all ~900 inactive items in the first month.

@jklymak jklymak force-pushed the bld-stale-action branch 2 times, most recently from 3c075e2 to 21fea65 Compare February 8, 2023 15:41
days-before-issue-stale: 365
days-before-issue-close: 30
ascending: true
exempt-issue-labels: keep
Copy link
Member Author

Choose a reason for hiding this comment

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

added exempt labels (which could be expanded) so that if we have things we don't want cleaned up we can "keep"

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the "Orphaned PR" label should be exempt, as it seems redundant to check those.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Not sure if I have the syntax right for the labels with spaces in them...

Copy link
Member

Choose a reason for hiding this comment

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

@rcomer
Copy link
Member

rcomer commented Feb 10, 2023

Should the repo token be set like it is for PR welcome?

repo-token: ${{ secrets.GITHUB_TOKEN }}

I admit that I don’t know the difference between this and the default used by the action.
https://github.com/actions/stale#repo-token

It might also be good restrict to this repo so it doesn’t run on forks, like we do for the tests:

github.repository == 'matplotlib/matplotlib' &&

@jklymak
Copy link
Member Author

jklymak commented Feb 10, 2023

Sure, did both of those. No idea what the effect is so maybe someone with more GH Actions experience should check?

@tacaswell
Copy link
Member

I killed appveyor for this PR to un-block running a higher-priority one for 3.7.0 prep.

@jklymak
Copy link
Member Author

jklymak commented Feb 23, 2023

@rcomer, can I ping you for review? Or we should close this if we aren't actually going to do it....

@rcomer
Copy link
Member

rcomer commented Feb 23, 2023

I think @ksunden said he would take a look at this. I'm far from an expert in GitHub Actions.

@ksunden
Copy link
Member

ksunden commented Feb 23, 2023

Functionally, my only comment is the space in the comma separated label list.

Though I do want to revive @melissawm's request that these timelines appear in the docs.

@jklymak
Copy link
Member Author

jklymak commented Feb 23, 2023

Though I do want to revive @melissawm's request that these timelines appear in the docs.

Agreed, but I think it's premature to document all this yet until we figure out if/how it works?

.github/workflows/stale.yml Outdated Show resolved Hide resolved
Co-authored-by: Kyle Sunden <git@ksunden.space>
Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com>
@jklymak
Copy link
Member Author

jklymak commented Feb 23, 2023

squashed and pushed to kill CI, which doesn't do anything for this PR

Copy link
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

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

I think this is fine to go in based on one (@ksunden) technical review - it’s not like it’s user facing code. The messaging has had input from several people. My only question is whether we should now leave it a couple of days before merge, so that notifications don’t pile up over the weekend.

@oscargus
Copy link
Contributor

Rather, I would expect that someone who is watching the issue will see the notification from the bot's comment and check whether a recent mpl version still has the problem.

This is the thing. I do not expect most users to check five or ten year old bug reports and, if the bug still exists, write something kind/sensible. Also, keeping track of notifications is quite hard for many people. A few days off and it is impossible to not miss something. I'm quite sure we will lose sensible reports and feature requests.

Finally, do we know if these are "closed as completed" or the other option? Ideally it should be a third option, "hide", for these, so one can check back in case one decides to work on a particular thing.

@rcomer
Copy link
Member

rcomer commented Feb 24, 2023

A few days off and it is impossible to not miss something. I'm quite sure we will lose sensible reports and feature requests.

I think the worst that can happen is that a currently valid issue gets closed and forgotten. But the situation now is that we have nearly 1600 open issues, and I'm not sure it's realistic to manually sort through them all to see which are still relevant. So either way, issues are forgotten.

If a bug or feature request affects a lot of users, then I would expect more than one user to be subscribed to the issue. That should reduce the risk of the notification being completely missed. Also devs can see the bot's updates and can make our own judgements if we think something should stay open. @jklymak has deliberately chosen settings to keep the frequency low and therefore hopefully manageable.

Finally, do we know if these are "closed as completed" or the other option? Ideally it should be a third option, "hide", for these, so one can check back in case one decides to work on a particular thing.

They are closed as "not planned", but will also retain the "inactive" label, so could be searched that way.

@rcomer rcomer added this to the v3.8.0 milestone Feb 26, 2023
@rcomer rcomer merged commit 07c43e4 into matplotlib:main Feb 26, 2023
@rcomer
Copy link
Member

rcomer commented Feb 26, 2023

I just created the “inactive” and “keep” labels. Currently in white. Please update colour and description as you see fit!

@jklymak jklymak deleted the bld-stale-action branch February 26, 2023 16:23
@jklymak
Copy link
Member Author

jklymak commented Feb 27, 2023

Well, that worked - caught 8 old issues (no PRs) that it marked inactive. I marked one of them "keep". We will see if it moves on from there....

@rcomer
Copy link
Member

rcomer commented Feb 27, 2023

I guess the oldest PR is newer than a lot of issues so it will take a long time to get to any PRs. Maybe I shouldn’t have suggested to set ascending: true?

@oscargus
Copy link
Contributor

I've checked the eight and am tempted to say that they are all valid requests (not sure if the colormap one is still an issue though). Not sure if I should mark the non-marked ones though...

What about adding a "closed but relevant" tag? In that way we can only keep "active" issues, but still access unsolved issues when needed. Like the mathtext issues in case we get a GSoC student working on it?

@rcomer
Copy link
Member

rcomer commented Feb 27, 2023

Maybe in a month's time when things start to close, we could review what was closed and what we think about it. Certainly I see no downside to adding whatever labels you think are useful to the closed issues.

@jklymak
Copy link
Member Author

jklymak commented Feb 27, 2023

There is "valid" and then there is "reasonable". If an issue is open for 12 y folks have clearly been making do without the request. But for sure, if anyone thinks an issue will get acted on, they should comment or mark as "keep".

@rcomer
Copy link
Member

rcomer commented Mar 3, 2023

Notes from the action’s logs on operations used:

  • Newly labelling an issue uses 4. example
  • Checking an already labelled issue uses 2. example
  • Checking an already labelled issue and removing the label uses 3. example
  • Checking an already labelled issue and closing it uses 3. example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: testing CI configuration and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants