Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

GitHub: auto set inactive label#25163

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
rcomer merged 1 commit intomatplotlib:mainfromjklymak:bld-stale-action
Feb 26, 2023

Conversation

jklymak
Copy link
Member

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 (andpytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild 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 indoc/users/next_whats_new/
  • API changes are marked with a.. versionchanged:: directive in the docstring and documented indoc/api/next_api_changes/
  • Release notes conform with instructions innext_whats_new/README.rst ornext_api_changes/README.rst

melissawm reacted with hooray emojircomer reacted with eyes emoji
@jklymakjklymak added the status: needs comment/discussionneeds consensus on next step labelFeb 6, 2023
@jklymakjklymak changed the titleGitHub: inactive labelGitHub: auto set inactive labelFeb 6, 2023
@rcomer
Copy link
Member

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

story645 reacted with thumbs up emoji

@jklymak
Copy link
MemberAuthor

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

@oscargus
Copy link
Member

oscargus commentedFeb 7, 2023
edited
Loading

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"?

story645 reacted with thumbs up emoji

@rcomer
Copy link
Member

We currently have871 open issues that have not been updated for a year of which49 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 reacted with thumbs up emoji

@jklymak
Copy link
MemberAuthor

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.

rcomer reacted with thumbs up emoji

@melissawm
Copy link
Member

Thanks@jklymak ! A couple of comments:

  • Can we add the timelines (and a mention about the action) to thedocumentation 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 and rcomer reacted with thumbs up emoji

@jklymak
Copy link
MemberAuthor

jklymak commentedFeb 7, 2023
edited
Loading

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.

melissawm and rcomer reacted with thumbs up emoji

@rcomer
Copy link
Member

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.

story645 reacted with thumbs up emoji

@jklymak
Copy link
MemberAuthor

" 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

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 reacted with thumbs up emoji

@jklymak
Copy link
MemberAuthor

jklymak commentedFeb 7, 2023
edited
Loading

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.

story645 and rcomer reacted with thumbs up emoji

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
MemberAuthor

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.

story645 reacted with thumbs up emoji
@rcomer
Copy link
Member

The action sifts through issues/PRs in chronological order and by default starts at the newest. I suggest settingascending: 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
MemberAuthor

jklymak commentedFeb 7, 2023
edited by story645
Loading

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

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
MemberAuthor

OK, Ithink 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?

rcomer reacted with thumbs up emoji

@jklymak
Copy link
MemberAuthor

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 commentedFeb 7, 2023
edited
Loading

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 inIris - I haven’t actually studied its inner workings!

jklymak reacted with thumbs up emoji

@jklymak
Copy link
MemberAuthor

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

not-planned, by default...

@jklymak
Copy link
MemberAuthor

jklymak commentedFeb 7, 2023
edited
Loading

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

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 and story645 reacted with thumbs up emoji

@jklymakjklymakforce-pushed thebld-stale-action branch 2 times, most recently from3c075e2 to21fea65CompareFebruary 8, 2023 15:41
days-before-issue-stale: 365
days-before-issue-close: 30
ascending: true
exempt-issue-labels: keep
Copy link
MemberAuthor

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.

story645 reacted with thumbs up emoji
Copy link
MemberAuthor

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

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' &&

story645 reacted with thumbs up emoji

@jklymak
Copy link
MemberAuthor

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
MemberAuthor

@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

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

@ksunden
Copy link
Member

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
MemberAuthor

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?

Co-authored-by: Kyle Sunden <git@ksunden.space>Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com>
@jklymak
Copy link
MemberAuthor

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

Copy link
Member

@rcomerrcomer 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
Member

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

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.

story645 reacted with thumbs up emoji

@rcomerrcomer added this to thev3.8.0 milestoneFeb 26, 2023
@rcomerrcomer merged commit07c43e4 intomatplotlib:mainFeb 26, 2023
@rcomer
Copy link
Member

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

@jklymakjklymak deleted the bld-stale-action branchFebruary 26, 2023 16:23
@jklymak
Copy link
MemberAuthor

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 reacted with thumbs up emoji

@rcomer
Copy link
Member

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 setascending: true?

@oscargus
Copy link
Member

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

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 reacted with thumbs up emoji

@jklymak
Copy link
MemberAuthor

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 commentedMar 3, 2023
edited
Loading

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
melissawm reacted with heart emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@story645story645story645 left review comments

@ksundenksundenksunden approved these changes

@rcomerrcomerrcomer approved these changes

Assignees
No one assigned
Labels
CI: testingCI configuration and testing
Projects
None yet
Milestone
v3.8.0
Development

Successfully merging this pull request may close these issues.

7 participants
@jklymak@rcomer@oscargus@melissawm@story645@tacaswell@ksunden

[8]ページ先頭

©2009-2025 Movatter.jp