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.