Add issue-opener when wheels are almost dead - #175
Conversation
|
The CI / test upload via action (pull_request) is red on every PR, which is not good. I'll push a little commit for that, too. |
|
... and since this was largely agent-authored, I'll push a commit to seed an AGENTS.md (with a CLAUDE.md pointing to it) as well so future agent-based PRs are easier. Could be a separate PR (or omitted entirely!), but seems reasonable to add it here. |
There was a problem hiding this comment.
why do we need these lock files?
There was a problem hiding this comment.
To fix the dep pinning. But I think it'll be better to use a requirements.txt and let dependabot update it... I'll push that instead.
|
@bsipocz good to go now? I think this is the sort of thing it might be easiest to try and then refine iteratively if it doesn't work |
|
Not sure why I was assigned @matthewfeickert ? This is my PR and I've addressed review comments... 🤔 |
This is how PR assignment works. https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/assigning-issues-and-pull-requests-to-other-github-users I'm not sure what is unclear about this. Do you use a different standard in your projects? |
|
Yeah I don't remember ever being assigned a PR that I've authored (so if it has happened, I think it's rare), just issues. So I'm not quite sure what it signifies... not sure what I (or others) are supposed to take away, understand, do, etc. from me being assigned to my PR. Does it just mean it's mine to work on and edit and others shouldn't or something? That's at least what I've understood issue assignment means. But I figured for a PR, that's typically the default state: leave it for the PR author to work on (other than maybe minor maintainer tweaks before merge), hence my confusion... |
|
Okay for you @bsipocz ? Would be good to start trying this out before another wheel bites the dust! |
|
Maybe this is more common in larger orgs? To me this is quite normal; every PR gets assigned for clarity.
You can group by assignment. As I linked above, assignment makes it clear who is the responsible party/parties for implementation, which is why you can have multiple people assigned to a PR.
Indeed, the default assignment is to the PR creator, but if responsibility shifts it is useful to formalize this. As I said, every Issue and every PR gets assigned (ideally) so that there's any hope at being able to track states by team members.
@larsoner I'm currently about to go into meetings but I have some revision suggestions that I'll leave in the next few hours. |
matthewfeickert
left a comment
There was a problem hiding this comment.
RE: #175 (comment), this is one of a few. I'll leave more in a few hours.
|
|
||
| Until it is fixed, whatever that workflow keeps an eye on is unattended." | ||
|
|
||
| number=$(gh issue list --repo "${GITHUB_REPOSITORY}" --state open --json number,title \ |
There was a problem hiding this comment.
The default limit is 30
$ gh issue list --help
...
FLAGS
...
-L, --limit int Maximum number of issues to fetch (default 30)which could realistically be below the open issue count on some repos. Bump this to something larger like 200 or more? (gh paginates at 100 per page, so a few API calls isn't that bad)
| number=$(gh issue list --repo "${GITHUB_REPOSITORY}" --state open --json number,title \ | |
| number=$(gh issue list --repo "${GITHUB_REPOSITORY}" --state open --limit 200 \ | |
| --json number,title \ |
There was a problem hiding this comment.
Good call, fixed -- but by using --author. FWIW this part of the action is for opening issues on this repo when its own actions fail for some reason (i.e., not for opening issues on repos that upload wheels -- that's a separate step done elsewhere). We currently have 20 issues open total, and none from the bot, so with --author the default limit of 30 should be fine for finding non-stale issues opened by the bot I think.
The issue count for other repos like sklearn etc. is handled down in tools/check_stale_wheels.py, and it uses login=... which limits it to issues opened by the scientific-python bot, so I think we're hopefully good there.
Yeap, looking at it now. As for PR assignments, I also feel that's an overkill and we don't do that in community OSS. We don't/should not really do review assignments either besides pings -- it's different when someone self-assigns, but as none of us are each others manager we shouldn't really assign anything, unless that person's review/sign-off is an actual blocker. IMO. |
bsipocz
left a comment
There was a problem hiding this comment.
OK, so I have a bunch of nitpicky wordsmithing and documentation related comments. None of them are blockers.
However, please address my question about the packages included in the loop. IMO, that's the only blocker, but it maybe just a comment that resolves the confusion, or maybe a fix if I read things correctly.
There was a problem hiding this comment.
I think this will be a nice short action to reuse elsewhere, too. So ultimately, I think it would make sense to actually document/mention all of the actions, suitable for direct downstream usage, in the README.
|
|
||
| - name: Check for stale wheels | ||
| env: | ||
| ISSUE_OPENER_TOKEN: ${{ secrets.ISSUE_OPENER_TOKEN }} |
There was a problem hiding this comment.
I haven't reviewed or even read this file.
| upload is older than the warning threshold, and opens an issue on the project's | ||
| own tracker so the maintainers hear about it before the wheels disappear. | ||
|
|
||
| Requires ``ISSUE_OPENER_TOKEN`` (a classic token with the ``public_repo`` scope, |
There was a problem hiding this comment.
I think this docs should be exposed in the README.
| # Hidden markers let us find our own issues and comments again without relying on | ||
| # the (fuzzy, eventually consistent) search API. | ||
| MARKER = "<!-- scientific-python-nightly-wheels: stale-wheels -->" | ||
| FINAL_WARNING_MARKER = "<!-- scientific-python-nightly-wheels: stale-wheels-final -->" | ||
| REPORT_MARKER = "<!-- scientific-python-nightly-wheels: stale-wheels-report -->" | ||
|
|
There was a problem hiding this comment.
nor a merge blocker, but I wonder if things fail loudly if any of these markers or URLs change/stop working?
|
Okay, taken care of @bsipocz! Took/implemented the suggestions. On failing loudly, the idea is that anything that stops us reaching a project's tracker (no usable URL, issues disabled, bad token, etc.) gets collected into a single issue in this repo that's rewritten each run, and a failure of the workflow itself opens one here too, so it should always signal needing manual intervention. The hidden markers are the one thing that can't self-report: if we renamed one, the issues already carrying it would be orphaned and we'd open new ones alongside them, so there's a comment in the script saying not to do so. A maintainer stripping it out of an issue body just gets them a fresh issue, which seems fine. |
bsipocz
left a comment
There was a problem hiding this comment.
Thanks, it's looking good now and I agree that we can iterate on it in case we need adjustments and follow-ups.
|
It would be great to merge this in the next day if possible. A few wheels are near expiry. (Can open issues manually of course, but they would be an excellent test of the mechanics and a good opportunity for ironing out any residual bugs!) |
|
OK, given Matthew's thumbs up on your response to his blocking review, I would go ahead and consider it addressed and merge this without waiting for another round from him. |
|
Okay I'll |
|
Dry run looking good:
So I ran it in non-dry-run mode and it opened the three issues 🚀 |
|
I really love how good the report summary looks like! |

Drafted by Claude Opus 5, reviewed by me. Retention policy unchanged. But now with this action run in this repo on a daily schedule (1h before wheel removal):
Eventually if for some reason this becomes annoying, we can add an opt-out list, different reporting stages, etc. But I think this is a good start. Comes with some minimal tests. Ended up being a bit longer than I expected, but it covers a bit more ground/cases than I realized it should originally.
Closes #171
Once this is merged I'll
workflow_dispatchonce in dry-run mode, then once that's good once again in full mode. Both of these should say everything worked okay and that it's acting as thescientific-python-bot. And I expect issues forsunpy(29 days, hours from losing its wheels),pyproj(19), andscikit-image(18).