Decision needed: should we close stale PRs? and how many lapsed days are PRs considered stale?

Mariatta · July 2, 2020, 7:54pm

In the past we’ve discussed about closing stale PRs (prs with no activity, no comment or anything after a certain period), however we have never reached a decision on how many days of no activity that the PR can be considered stale.

There is now a GitHub Action for automatically markings PRs and issues as stale, and it can also automatically close the stale issue/PR.

It works as follows: if a the PR has no activity after certain (configurable) period, it will apply the “stale” label, and leave a comment to give a warning that this PR is now stale. If someone them leaves a comment in the PR (they can leave a comment saying “I’m waiting for review” or “still working on it” or anything) , then the stale label gets removed.
However if there continued to be no activity, no further comments after certain number of days, then the bot will close the PR.

Questions and decisions needed:

Should we enable this stale GitHub actions for the CPython repo? (wip PR for adding it: https://github.com/python/cpython/pull/21247)
if we enable it, how many days of no activity do we consider the PR as stale?
how many days should it wait before closing the PR? (this is required configuration by the GitHub action)

remilapeyre · July 2, 2020, 8:06pm

The current version warns a PR will be removed after 30 days. There is a lot of PRs that wait more than that because it can take a long time for a core developer to read and review them. For example https://github.com/python/cpython/pull/20517 or https://github.com/python/cpython/pull/19632.

Obviously I’m aware of how much time consuming it is do this job and it’s not a critique, but I’m afraid that if they are closed automatically, a lot of work would be lost.

Especially for new contributors that are still learning the process of contributing to Python, if they miss the message their PR will be closed and they need to track them carefully or their work will be forgotten which raises the bar for them.

I think the number of days before sending the message should be greater than the maximum time it can take for a core developer to get to a PR, maybe 6 months?

Mariatta · July 2, 2020, 8:12pm

Yes it makes sense to me to have the “days before closing” to be really long like in months instead of days.

I think having the stale label will also be useful so coredevs and triagers can hopefully check the PRs that are stale periodically and prioritize reviewing such PRs.

EpicWink · July 2, 2020, 10:09pm

+1 on making as stale.

Once marked as stale, does it go to the top of the PRs list because it has activity? Will it also go to the top of the list because the PR author will say “waiting for review”. That’s going to be more than 3 PRs brought to the top every day.

Is there any benefit to closing PRs? I can’t look at the code right now, so I’m not sure if it can be passed a null value for the days-until-close parameter

remilapeyre · July 2, 2020, 9:56pm

According to https://github.com/actions/stale/blob/master/action.yml#L16 you can set days-before-close to -1 and it will never close the issue.

brettcannon · July 2, 2020, 11:04pm

Depends on how you sort the list of PRs. But the default view is entirely based on when a PR was first opened.

willingc · July 4, 2020, 6:18pm

I’m not sure the distribution of age on our PRs. 60 days from stale to close seems reasonable if no activity. As for how long before stale (6 months?).

edison.abahurire · July 9, 2020, 2:19pm

Great suggestions here and the idea is also really good.

I think a PR should not be marked as stale if its “Awaiting Core Review” though; that would be unfair to the author.

PRs that are labelled “Awaiting changes” for too long can be archived though.

ammaraskar · July 13, 2020, 11:33pm

Just for some quick data, I ran some analysis on our pull requests. If anyone has any requests for any other graphs they’d be interested in or the raw data, feel free to reply here.

% of pull requests merged versus days open

With core devs

Without core devs

The remaining two graphs are with core-devs excluded given this disparity.

Days since last update (comment, push etc) on open PRs

Not sure what that peak is near the ~400 day mark.

Time between “awaiting changes” label and merge

My two cents: we should lean a bit on the conservative side for the “time to mark as stale” and “time to close stale PRs”. Like @remilapeyre said, this can really dissuade contributors especially given how long reviews can take.

Based on the non core-dev data, given that 95% of pull requests are merged within 300 days, I would recommend that as the cut-off point for marking PRs as stale.

For time to close, setting it to 40-50 days seems reasonable since it looks like that amount of time is what it takes for authors to take actions on pull requests.

I do think it’s useful to close out really old PRs that haven’t been actioned on. It’s equally as annoying for reviewers when they go to review a patch that no longer merges cleanly. New contributors taking on a bug also have to wait for the stale PR author to respond back.

willingc · July 13, 2020, 10:59pm

Thanks for the helpful research. It would be great to capture this tool/notebook/script somewhere. Perhaps run it once a month.

ammaraskar · July 14, 2020, 7:52am

Here’s the quick and dirty code, it’s easy to generate the PR age distribution graph since you can retrieve 100 pull requests per request from Github (~215 requests in total currently). It’s harder to do ones based on labels since that needs a request per pull request to pull in all the data.

Code

from github import Github
import json


g = Github("GITHUB_TOKEN", per_page=100)

with open('pr_list.json', 'w', encoding='utf-8') as f:
    repo = g.get_repo("python/cpython")

    for i, pull in enumerate(repo.get_pulls('all')):
        print("Retrieved {} pull requests".format(i))
        f.write(json.dumps(pull._rawData))
        f.write("\n")

Jupyter Notebook

pull_requests = {}
with open('pr_list.json', 'r', encoding='utf-8') as f:
    for line in f:
        raw_object = json.loads(line)                 
        id = int(raw_object["url"].split("/")[-1])
        pull_requests[id] = raw_object

merge_days = []
for pr in prs.values():
    if pr['user'] in CORE_DEVS or pr['user'] in BOTS:
        continue
    time_to_merge = pendulum.parse(pr['merged_at']) - pendulum.parse(pr['created_at'])
    merge_days.append(time_to_merge.in_days())

ax = sns.kdeplot(all_merge_times, cumulative=True, shade=True)
ax.set_xlim(left=0)
ax.set(xlabel='Days after PR Creation', ylabel='Pull requests merged % (cumulative)')

I think the more complex analysis can be done through the backups we take of the Github repo if we want to avoid hitting the github api for all the info.

tjreedy · September 6, 2020, 3:48am

I will repeat here that I am opposed to mindless (mechanical) closing of issues and PRs in general and especially those for IDLE. Earlier this year, I closed about 40 IDLE issues (as other than ‘fixed’) and any linked PRs. But I left some open. I am more or less working now on a few really old issues (as in 10 years).

mbussonn · October 1, 2020, 9:41pm

There was a similar discussion in the matplotlib gitter, with both arguments that:

A large number of PRs can be discouraging for (new) contributors.
vs
Closing an inactive PR can also drive contributor away.

Both of which are good point, and I’m not going to delve into whether the PR is closed with/without manual/automatic warning or not. nor does this address whether the turnout time if fast or not and so whether the Number of PR is a good metric, but arguably that’s one of the most prominent (and default) things visible in a repo and finding out whether the turnaround time is short requires user active interaction with the repo which most people won’t do.

I personally don’t think there is a one answer fit all repository, still this have lead me to run this highly non-scientific poll:

Dear OSS contributors. Which feels the worst A) You started a PR on a project, have not finished/touched it in quite some time. PR get closed with a message “This looks inactive, feel free to reopen”. B) You want to contribute to project X, there are already 200+ opened PRs.

528 reply, (~186 self identified maintainers, ~342 self identified users).

In general both groups have about 2.5 to 3 time more respondent feeling that coming across a repo with more than 200 PR is worse than having a PR closed for inactivity.

I’ll leave the analysis to the reader. Some of the tweets reply also have good points.

I will also point out that closing a PR for inactivity can be discouraging for O(1) user per closed PR, (basically anyone interacting with this PR in particular or coming across it, and realising it was closed)
while having a discouragingly high number of PRs will effect about anyone coming across the repo even before contributing. So my guess is that there are many people (at least N=1, me) that won’t even send a PR if this number is too high.

I also don’t talk about maintainer feeling of having too many PRs, personally I feel stressed, and have difficulty finding what to focus on when the number of PRs on repo I maintain is high.

Thanks for reading my rambling, and everyone for you consideration on this issue.

Mariatta · December 14, 2020, 11:17pm

I have just closed a 3 year old PR about this issue. I think has stalled for too long

It sounds like folks here don’t mind the “stale” label but some folks don’t want the PR to be automatically closed. I have adjusted my PR so that the “stale” label will get added, but it doesn’t ever close the PR, by setting the days-before-closing value to -1.

If there is no objection I will merge the PR tomorrow. (https://github.com/python/cpython/pull/21247)

pablogsal · December 16, 2020, 1:01am

Thanks for the change @Mariatta! One thing we should fix is the message as it currently says:

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

For example:

The part I am referring to is “…or this will be closed in 5 days”

Mariatta · December 16, 2020, 1:06am

Yes I realized that now. I will look into customizing the message. thanks!