Policy for closing abandoned PRs

[Vincent Macri]

Currently the allowed reasons to close PRs (see
https://doc.sagemath.org/html/en/developer/review.html) is if they try
to do too many things, are patch bombs, or aren't sage-specific. We have
some PRs that have been reviewed with changes requested with no response
from the person who proposed the pull request. Do we have a policy for
when to close these PRs? The review guide also says to tag a rejected PR
with one of "r: duplicate", "r: invalid", "r: wontfix", and "r:
worksforme". None of these seem appropriate for closing a PR from an
unresponsive author.

Open PRs with an unresponsive author clutter up the GitHub UI and can
potentially bury other important PRs. I also think that if new potential
contributors see hundreds of open PRs on the repo, some of them months
old, they may be discouraged from contributing out of fear that any PR
they make won't get merged.

If we don't have a policy on this already, may I suggest something along
the lines of this: After a PR is reviewed and changes are requested, if
the author does not respond within 4 months (or some other period of
time that sage-devel thinks is appropriate) then the PR may be closed
with the tag "r: no response". The PR may be reopened at a later date if
the author responds.

--
Vincent Macri (He/Him)

[Jackson Walters]

That sounds reasonable to me. I have a couple open PRs, one of which originated months ago, but I try to stay current with it. I don't see a problem with closing it with an appropriate tag with the possibility of reopening down the road if they do respond.

Best,

Jackson

--
You received this message because you are subscribed to the Google Groups "sage-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+...@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/sage-devel/e1c5735c-553e-4012-ba0d-2cb9ee0d9d83%40ucalgary.ca.

[kcrisman]

This would be a change in (perhaps not official) policy from Trac.  Since PRs are separated from issues on GH (which wasn't really the case on Trac), a change might make sense.  However, my opinion (for whatever it's worth) is that this is not really necessary, and could lead to useful code being completely abandoned, in practice.  There were many instances in the past where partial fixes/new functionality was finished over a period of months and years in precisely this way (and that could still work as long as the original issue has a clear link to the unfinished PR).  Maybe the GH UI is just cluttered in general!  (I'm aware this may be a minority opinion.)

[Vincent Macri]

To be clear, my proposal isn't that PRs can't take months or years. It's simply that if changes are requested and the developer doesn't respond for several months (so no progress on the ticket at all) then the PR is closed but can be reopened (even by the person who made the PR ideally, if GitHub permissions allow which I think they do) as soon as they start working on it again.

Ideally any closings of PRs in this way would be accompanied by a link to the developer guide explaining that all the dev needs to do to reopen the PR is continue working on it.

Vincent Macri (he/him)

---

From: sage-...@googlegroups.com <sage-...@googlegroups.com> on behalf of kcrisman <kcri...@gmail.com>
Sent: Wednesday, January 22, 2025 8:48:31 AM
To: sage-devel <sage-...@googlegroups.com>
Subject: Re: [sage-devel] Policy for closing abandoned PRs

 

[△EXTERNAL]

This would be a change in (perhaps not official) policy from Trac.  Since PRs are separated from issues on GH (which wasn't really the case on Trac), a change might make sense.  However, my opinion (for whatever it's worth) is that this is not really necessary, and could lead to useful code being completely abandoned, in practice.  There were many instances in the past where partial fixes/new functionality was finished over a period of months and years in precisely this way (and that could still work as long as the original issue has a clear link to the unfinished PR).  Maybe the GH UI is just cluttered in general!  (I'm aware this may be a minority opinion.)

--

You received this message because you are subscribed to the Google Groups "sage-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+...@googlegroups.com.

To view this discussion visit https://groups.google.com/d/msgid/sage-devel/d2262ac0-a84e-49d1-b6d1-efe28ac27791n%40googlegroups.com.

[Dima Pasechnik]

On Wed, Jan 22, 2025 at 9:48 AM kcrisman <kcri...@gmail.com> wrote:
>
> This would be a change in (perhaps not official) policy from Trac.
> Since PRs are separated from issues on GH (which wasn't really the case on Trac), a change might make sense. However, my opinion (for whatever it's worth) is that this is not really necessary, and could lead to useful code being completely abandoned, in practice.

A PR where the author went AWOL (or worse) do clutter, and there is
little harm in closing these with an appropriate tag - as long as the
branch (pull/<PR number>/head) is not removed, no data/code is lost.

I'd say 6 or 9 months is probably a sufficient period of inactivity to
declare a PR abandoned,

Dima

> There were many instances in the past where partial fixes/new functionality was finished over a period of months and years in precisely this way (and that could still work as long as the original issue has a clear link to the unfinished PR). Maybe the GH UI is just cluttered in general! (I'm aware this may be a minority opinion.)
>

> --
> You received this message because you are subscribed to the Google Groups "sage-devel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+...@googlegroups.com.

> To view this discussion visit https://groups.google.com/d/msgid/sage-devel/d2262ac0-a84e-49d1-b6d1-efe28ac27791n%40googlegroups.com.

[Gonzalo Tornaría]

As a comparision, in void-packages there is the following policy:
- an issue (or PR) with 90 days of inactivity is labeled "stale" and will send a notification.
- any activity in the issue will reset the "stale" label, so an author can just make a status report on receipt of the notification to keep the issue open
- after 14 days "stale", the issue is closed
- author (or maintainer?) can reopen the issue. However: note that github does not allow reopening a PR *unless* the corresponding branch points to the exact same commit as when the PR was closed, so this is sometimes tricky.

Implementing this seems easy, see: https://github.com/void-linux/void-packages/blob/master/.github/workflows/stale.yml

There can be certain labels which make the issue exempt from the stale check.

Best,
Gonzalo

[Vincent Macri]

I would be against automatically closing stale issues just because realistically we aren't closing issues that fast (I would love if all Sage issues were resolved in 90 days, but I don't think that is realistic right now). Perhaps as a first step for dealing with all the open issues would be doing a better job of triage, especially using priority tags. It's probably worth clarifying the circumstances in which we do close an issue as well. But that's worth its own separate discussion.

For closing PRs with no activity, if we were to do it automatically, that timer should start after the PR is tagged with "changes requested". I don't want to punish contributions where it's our fault for not reviewing them in a timely manner. Basically we should only be closing PRs where we reviewed them, changes were requested, and changes were not made after several months.

Vincent Macri (he/him)

---

From: 'Gonzalo Tornaría' via sage-devel <sage-...@googlegroups.com>
Sent: Wednesday, January 22, 2025 11:22:43 AM
To: sage-...@googlegroups.com <sage-...@googlegroups.com>

Subject: Re: [sage-devel] Policy for closing abandoned PRs

 

[△EXTERNAL]

--
You received this message because you are subscribed to the Google Groups "sage-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+...@googlegroups.com.

To view this discussion visit https://groups.google.com/d/msgid/sage-devel/e11f448f7ff6bf66412d2288c6ca11e6%40cmat.edu.uy.

[dmo...@deductivepress.ca]

When we were on trac, if there was a PR that had been dormant, but was close to completion, someone else could rebase it, and make the necessary changes to pull it across the finish line.  I'd like to see that happen in our new system, instead of just closing these PRs.  Is that possible?  My impression is that I can't edit someone else's PR, so how can we move these things forward, instead of just closing them unfinished?

[Jackson Walters]

I had a similar thought but didn’t know if it was possible. I don’t really understand the idea of being religious about not touching someone’s PR if they were the one that opened it. Surely just getting the code finished and merged is more important, and being able to rebase rather than abandoning seems like a good idea.

To view this discussion visit https://groups.google.com/d/msgid/sage-devel/def22026-d56a-487e-a8eb-853cc9e20862n%40googlegroups.com.

[Nils Bruin]

On Wednesday, 22 January 2025 at 20:30:46 UTC-8 jackson...@gmail.com wrote:

I had a similar thought but didn’t know if it was possible. I don’t really understand the idea of being religious about not touching someone’s PR if they were the one that opened it. Surely just getting the code finished and merged is more important, and being able to rebase rather than abandoning seems like a good idea.

The conversation on the PR is part of the sagemath repo, so team members can for the most part comment there. The code, however, resides in a fork that is owned by the person who made the PR. You can't really expect to be able to push updates to that branch. "Taking over" a PR on trac meant one would connect a different branch to the ticket. I'm not sure github allows that.

It looks like "allow edits by maintainers" goes some way to alleviating these problems (see https://github.blog/news-insights/product-news/improving-collaboration-with-forks/), but I expect we don't have enough "maintainers" to really make this useful (only people with push privilege on the sagemath repo would qualify).

If you'd know beforehand that there are several people who'll be working on a PR it should be possible to just make a fork where all these people are given push privilege. But that requires some advance planning which wouldn't help these after-the-fact resuscitation endeavours. There's always the option of forking the PR branch and make a new pull request from that. The git metadata would still accurately reflect authorship of the earlier commits. It would lead to a discontinuity in the discussion history, though, and it could give people the feeling their "PR" was stolen if they're more concentrated on PR authorship than commit authorship. For posterity, the git commit data is much more important but in github the PR discussions are much more visible, so it's not unlikely people would feel strongly about PR authorship. Even on trac, where tickets were owned by the organization, people would often feel attached to "their" tickets.

[Travis Scrimshaw]

Short version is I am strongly opposed to such a policy, automated or not. It's good to see what code has been contributed but not merged for different issues. We also should not assume unresponsive authors means they have abandoned their PR too since I would say most of us have more limited time than we wish to contribute.

IMO, GH is quite cluttered and generally horrible for organization of things like this (in particular, their PR/issue search is severely lacking), but we can't do too much about that I'm afraid.

On Thursday, January 23, 2025 at 2:20:43 PM UTC+9 Nils Bruin wrote:

On Wednesday, 22 January 2025 at 20:30:46 UTC-8 jackson...@gmail.com wrote:

I had a similar thought but didn’t know if it was possible. I don’t really understand the idea of being religious about not touching someone’s PR if they were the one that opened it. Surely just getting the code finished and merged is more important, and being able to rebase rather than abandoning seems like a good idea.

The conversation on the PR is part of the sagemath repo, so team members can for the most part comment there. The code, however, resides in a fork that is owned by the person who made the PR. You can't really expect to be able to push updates to that branch. "Taking over" a PR on trac meant one would connect a different branch to the ticket. I'm not sure github allows that.

Unfortunately that is the case. PRs are directly linked to branches. However, as Nils said, you can at least pull a PR's branch into your own fork and then create a new PR based on that. So the new PR then "replaces" the old one.

 

It looks like "allow edits by maintainers" goes some way to alleviating these problems (see https://github.blog/news-insights/product-news/improving-collaboration-with-forks/), but I expect we don't have enough "maintainers" to really make this useful (only people with push privilege on the sagemath repo would qualify).

Elevating people to maintainers gives some very special privileges beyond that IIRC. It likely 

If you'd know beforehand that there are several people who'll be working on a PR it should be possible to just make a fork where all these people are given push privilege. But that requires some advance planning which wouldn't help these after-the-fact resuscitation endeavours. There's always the option of forking the PR branch and make a new pull request from that. The git metadata would still accurately reflect authorship of the earlier commits. It would lead to a discontinuity in the discussion history, though, and it could give people the feeling their "PR" was stolen if they're more concentrated on PR authorship than commit authorship. For posterity, the git commit data is much more important but in github the PR discussions are much more visible, so it's not unlikely people would feel strongly about PR authorship. Even on trac, where tickets were owned by the organization, people would often feel attached to "their" tickets.

Indeed, but I think that is a fairly unlikely to happen. GH's model makes it even more difficult in some ways to have such things happen. I probably wouldn't spend much effort thinking about such situations.

Best,

Travis

[Dima Pasechnik]

On 22 January 2025 22:27:26 GMT-06:00, "dmo...@deductivepress.ca" <dmo...@deductivepress.ca> wrote:
>When we were on trac, if there was a PR that had been dormant, but was
>close to completion, someone else could rebase it, and make the necessary
>changes to pull it across the finish line. I'd like to see that happen in
>our new system, instead of just closing these PRs. Is that possible? My
>impression is that I can't edit someone else's PR, so how can we move these
>things forward, instead of just closing them unfinished?

Indeed, unless the author gave you push access to their repo (where the PR comes from), or is willing to merge PRs to their PR branch, or you have a lot of rights (e.g. are a repo owner),
the only way to proceed is with a new PR based on the existing one.

>On Wednesday, January 22, 2025 at 1:24:00 PM UTC-7 Vincent Macri wrote:
>
>> I would be against automatically closing stale issues just because
>> realistically we aren't closing issues that fast (I would love if all Sage
>> issues were resolved in 90 days, but I don't think that is realistic right
>> now). Perhaps as a first step for dealing with all the open issues would be
>> doing a better job of triage, especially using priority tags. It's probably
>> worth clarifying the circumstances in which we do close an issue as well. But
>> that's worth its own separate discussion.
>>
>> For closing PRs with no activity, if we were to do it automatically, that
>> timer should start after the PR is tagged with "changes requested". I don't
>> want to punish contributions where it's our fault for not reviewing them in
>> a timely manner. Basically we should only be closing PRs where we reviewed
>> them, changes were requested, and changes were not made after several
>> months.
>>
>> Vincent Macri (he/him)

>> ------------------------------
>> *From:* 'Gonzalo Tornaría' via sage-devel <sage-...@googlegroups.com>
>> *Sent:* Wednesday, January 22, 2025 11:22:43 AM
>> *To:* sage-...@googlegroups.com <sage-...@googlegroups.com>
>>
>> *Subject:* Re: [sage-devel] Policy for closing abandoned PRs

[Vincent Macri]

I want to clarify a couple things that might convince the people opposed to this (and if not I have a compromise proposal). Closing a PR doesn't mean we lose the potential contribution. You can still find closed PRs here: https://github.com/sagemath/sage/pulls?q=is%3Apr+is%3Aclosed+is%3Aunmerged. I think rather than thinking of closing a PR as deleting it, it's more accurate to think of it as archiving the PR. With my proposal there would also be a tag to filter to the abandoned ones. I think this would provide an easy way to search for abandoned PRs to take over and have someone else complete.

There are two reasons I go to https://github.com/sagemath/sage/pulls:

1. To find PRs to review myself.
   
2. To get a sneak peak for what new features/fixes might appear in an upcoming version of Sage.

A PR that was reviewed with changes requested and no developer response doesn't fall into either of the two categories above, which is why I think these abandoned PRs are mostly noise.

But if people are still opposed to moving the PR from the open tab to the closed tab, instead of closing it we could just add a tag for "unresponsive" but still leave those PRs open. We could then say that any PR marked with the unresponsive tag is fair game for someone else to take over (preferably after asking the original author if they intend to finish it).

Adding an "unresponsive" tag (either in the open or closed tab of pull requests) would communicate that you shouldn't expect that feature to be added anytime soon, but if you want to do that work yourself someone has already started it and you can take over.

I think functionally leaving it open or closed doesn't change much, it's the "unresponsive" tag that makes it clear that someone else is free to take the PR over. I would prefer to close abandoned PRs just to clean up the list of open pull requests, but if people prefer to leave it as open and just have the "unresponsive" tag then I'd be okay with that as a compromise.

To provide some additional context, the PR that prompted me to post this was this: https://github.com/sagemath/sage/pull/38292, it adds a new feature (Enigma encryption) that's only really of pedagogical/historical interest and hence low-priority. Unless there's a Sage contributor with both the interest and knowledge in the Enigma cipher and the time to fix up the PR, it's unlikely that it will get picked up by someone else. There are at least a dozen other encryption schemes that would be more useful to add to Sage, so while interesting it's a pretty niche contribution that I doubt anyone will want to pick up and finish since most of the people who have the expertise to finish it would probably want to work on adding an encryption scheme that is of interest to current cryptography researchers. Despite multiple people working on the original PR, there has been no response from the developers since I reviewed it in July. As somewhere between my original proposal and my compromise proposal, we could add an "unresponsive" tag and leave it up to discretion whether the PR is important enough to leave open or if it's better of closing it because it's unlikely to get picked up. Then a search for the "unresponsive" tag on open PRs would return a list of abandoned PRs that add/fix something important, and a search for the "unresponsive" tag on closed PRs would return a list of abandoned PRs that while they would be cool to have, aren't really important for Sage.

Vincent Macri (He/Him)

[Michael Orlitzky]

On 2025-01-23 10:35:13, Vincent Macri wrote:
> I want to clarify a couple things that might convince the people opposed
> to this (and if not I have a compromise proposal). Closing a PR doesn't
> mean we lose the potential contribution.

Any time you make a proposal you will get responses that skew negative
because the people who don't mind or don't care one way or the other
don't respond. So let me take the time to say that it's a fine idea.

If we are polite and communicative, no one should be offended, and
removing the things that can't be worked on from the PR list will make
it easier to work on the things that can. Most projects do this to
some extent. For a long time we were judiciously closing "dead"
tickets on Trac once they got a +1 to the milestone change.

[Kwankyu Lee]

The reviewer may close unresponsive PRs with the label "r: invalid". By the way, "r" stands for resolution. Or we could make up a new label "r: unresponsive" (with one word). I propose to wait for 3 months before closing. 

That policy should apply for strictly unresponsive  (with no apparent reason) PRs. The author may not agree with the reviewer's suggestions, and may look unresponsive.

I wonder how many such PRs we have. I guess not many.

[Jackson Walters]

I think a new label like “r: unresponsive” makes a lot of sense as opposed to “r: invalid”.

Best,

Jackson

--

You received this message because you are subscribed to the Google Groups "sage-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+...@googlegroups.com.

To view this discussion visit https://groups.google.com/d/msgid/sage-devel/ebcf2d0f-a8fd-43fb-a2e8-9a8f4dd46e45n%40googlegroups.com.

[kcrisman]

To clarify, I don't think that adding a label that indicates there is code which will probably bitrot and has been asked to address some issue is a bad idea.  It's the "closed" problem - GH does not by default automatically search closed issues, and actually I could totally see somebody wanting to add Enigma (or other) pedagogical crypto schemes in the future, who won't know there is pre-existing work.

I'm not sure why it matters if there are 5000 or 10000 open issues, because once there are more than 100 no one can keep track of all of them anyway and you need to search.  For better or for worse, there is a big distinction between "fix this Sage notebook bug" which is now truly invalid, and "fix this other bug which has some code but the person couldn't find a job in academia so they don't have time to finish up the small change requested" situation.  It's the fact that GH does treat closed tickets differently that means we shouldn't close them (in my opinion).

[Martin R]

I use the (very disappointing) search facility on github extensively to see whether other people had similar problems when trying to implement something.

To do this, it is very helpful that unfinished tickets are *not* closed, because this gives me the "interesting" tickets with a single click: I hide all the closed ones.  Usually only a few remain, and the older among those tell me the story I want to listen to.

One major annoyance for this scheme was that, some time in the past, tickets were automatically modified with a message similar to "As the Sage-8.8 release milestone is pending,...next release milestone (sage-8.9)."

All the best,

Martin

[Vincent Macri]

It seems like adding an "r: stale" or "r: unresponsive" tag might be a
better approach with more support then, and we could have it written in
the developer guide how long after changes are requested this tag should
be applied. Then anyone who is looking to pickup unfinished work can
search for that tag, and anyone who in interested in the PR being merged
will know that it's probably not coming soon unless someone else steps
up to finish it. I think Dima's suggestion of 6 months is good, but if
we aren't closing the PR then maybe even 3 months is sufficient. The "r:
stale" tag can be viewed as saying that other people should feel free to
take over the PR (preferably after checking with the original author if
they were planning on finishing it soon, in case they were about to
start work on it again or if they have any objection to or advice for
someone else finishing their work).

If someone does pickup a stale PR and opens a new PR based on it, should
the stale PR be closed when the new PR is opened, or when the new PR is
merged? I lean towards when it's opened unless someone can think of a
reason to keep open PRs for multiple iterations of the same thing.

Vincent Macri (He/Him)

[Martin R]

I'm afraid that adding a tag will modify the pull request, which would not be good for my workflow.  Could this be avoided?

Martin

[Nils Bruin]

On Friday, 24 January 2025 at 11:28:42 UTC-8 axio...@yahoo.de wrote:

To do this, it is very helpful that unfinished tickets are *not* closed, because this gives me the "interesting" tickets with a single click: I hide all the closed ones.  Usually only a few remain, and the older among those tell me the story I want to listen to.

One major annoyance for this scheme was that, some time in the past, tickets were automatically modified with a message similar to "As the Sage-8.8 release milestone is pending,...next release milestone (sage-8.9)."

 

That's a relevant data point: indeed, "stale" PRs are not "closed" PRs (which would include integrated PRs and truly discarded ones). The "stale" PRs need to be differentiated from them so that they can show up in searches. So perhaps "closing" them isn't the right thing to do.

However, if changing a tag on them changes a modification date and therefore changes where they show up in searches, wouldn't that give a handle to distinguish them? Just search on PRs that have a recent modification date? Then tickets would automatically become "stale" just by being not modified.

So if some people are advocating a tag (because otherwise there is no way to distinguish stale tickets) and other people are advocating *not* a tag, because that would ruin a way to distinguish stale tickets, perhaps these people should compare notes and investigate convenient search criteria.

 

[Kwankyu Lee]

On Saturday, January 25, 2025 at 5:35:47 AM UTC+9 Nils Bruin wrote:

On Friday, 24 January 2025 at 11:28:42 UTC-8 axio...@yahoo.de wrote:

To do this, it is very helpful that unfinished tickets are *not* closed, because this gives me the "interesting" tickets with a single click: I hide all the closed ones.  Usually only a few remain, and the older among those tell me the story I want to listen to.

 

That's a relevant data point: indeed, "stale" PRs are not "closed" PRs (which would include integrated PRs and truly discarded ones).

I agree with this view. A stale PR (with unresponsive author) should not be closed, because it is related with an open issue.

Having unresponsive author is rather the state of the review process. A reviewer may tag it with the existing label "pending" or with "s: needs info" until the author returns, to warn off other reviewers.

The "stale" PRs need to be differentiated from them so that they can show up in searches. So perhaps "closing" them isn't the right thing to do.

Searches also show closed PRs in the "closed" tab.