How does the bot decide whether the PR is waiting for reviews or is being abandoned by contributor ?
How about letting the bot count the number of times contributor pings committer(s) for review ? When unanswered ping count crosses some threshold, say 3, the bot publishes the JIRA and PR somewhere. Cheers On Tue, May 15, 2018 at 8:19 AM, Till Rohrmann <trohrm...@apache.org> wrote: > I'm a bit torn here because I can see the pros and cons for both sides. > > Maybe a compromise could be to not have a closing but a monitoring bot > which notifies us about inactive PRs. This could then trigger an > investigation of the underlying problem and ultimately lead to a conscious > decision to close or keep the PR open. As such it is not strictly necessary > to have such a bot but it would at least remind us to make a decision about > older PRs with no activity. > > Cheers, > Till > > On Tue, May 15, 2018 at 3:26 PM, Chesnay Schepler <ches...@apache.org> > wrote: > > > /So far I did it twice for older PRs. In both cases I didn’t get any > > response and I even forgot in which PRs I had asked this question, so > now I > > can not even close them :S/ > > > > To be honest this sounds more like an issue with how your organize your > > work. No amount of closing PRs can fix that. > > With GitBox we can assign reviewers to a PR, but I'm not sure whether it > > only allows committers to assign people. > > Bookmarks or text files might help as well./ > > / > > > > /Regarding only 30% blocked on contributor. I wonder what would be this > > number if we tried to ask in the rest of old PRs “Hey, are you still > > interested in reviewing/merging this?”. If old PR is waiting for a > > reviewer for X months, it doesn’t mean that’s not abandoned. Even if it > > doesn’t, reducing our overhead by those 30% of the PRs is something./ > > > > No doubt the number would be higher if we were to go back, but as i > > explained earlier that is not a reason to close it. If a PR is abandoned > > because we messed up we should still /try /to get it in. > > > > /This is kind of whole point of what I was proposing. If the PR author is > > still there, and can respond/bump/interrupt the closing timeout, that’s > > great. Gives us even more sense of urgency to review it./ > > > > Unfortunately knowing that it is more urgent is irrelevant, as we > > currently don't have the manpower to review them. Reviving them now would > > serve no purpose. The alternative is to wait until more people become > > active reviewers. > > > > /As a last resort, closing PR after timeout is not the end of the world. > > It always can be reopened./ > > > > Let's be realistic here, it will not be reopened. > > > > > > On 15.05.2018 14:21, Piotr Nowojski wrote: > > > >> I agree that we have other, even more important, problems with reviewing > >> PR and community, but that shouldn’t block us from trying to clean up > >> things a little bit and minimise the effort needed for reviewing PRs. > Now > >> before reviewing/picking older PRs I had to ask this “Hey, are you still > >> interested in merging this?” manually and wait for the response. If it > >> doesn’t come, I have to remember to go back and close PR, which I of > course > >> forget to do. Bah, so far I did it twice for older PRs. In both cases I > >> didn’t get any response and I even forgot in which PRs I had asked this > >> question, so now I can not even close them :S Wasted effort and wasted > time > >> on context switching for me and for everyone else that will have to > scroll > >> pass or look on those PR to check their status. > >> > >> Keep in mind that I am not proposing to close the PR automatically > >> straight on after 3 months of inactivity. Only after asking a question > >> whether original contributor is still there and he is interested in the > PR > >> to be reviewed. > >> > >> for Flink 1.5, I merged a contribution from PR #1990 after it was > >>> requested a few times by users. > >>> > >> This is kind of whole point of what I was proposing. If the PR author is > >> still there, and can respond/bump/interrupt the closing timeout, that’s > >> great. Gives us even more sense of urgency to review it. On the other > hand > >> if there is no response (no interest from the author for whatever the > >> reasons) and nobody so far has picked this PR to review/merge, what’s > the > >> point of keeping such PR open? As a last resort, closing PR after > timeout > >> is not the end of the world. It always can be reopened. > >> > >> Regarding only 30% blocked on contributor. I wonder what would be this > >> number if we tried to ask in the rest of old PRs “Hey, are you still > >> interested in reviewing/merging this?”. If old PR is waiting for a > reviewer > >> for X months, it doesn’t mean that’s not abandoned. Even if it doesn’t, > >> reducing our overhead by those 30% of the PRs is something. > >> > >> Piotrek > >> > >> On 15 May 2018, at 10:10, Fabian Hueske <fhue...@gmail.com> wrote: > >>> > >>> I'm with Chesnay on this issue. > >>> Stale PRs, i.e., a PR where a contributor becomes inactive, are one of > >>> our > >>> smallest issues, IMO. > >>> > >>> There are more reasons for the high number of PRs. > >>> * Lack of timely reviews. > >>> * Not eagerly closing PRs that have no or very little chance of being > >>> merged. Common reasons are > >>> 1) lack of interest in the feature by committers, > >>> 2) too extensive changes and hence time consuming reviews, or > >>> 3) bad quality. > >>> > >>> For 1), there are lots of older JIRA issues, that have low priority but > >>> which are picked up by contributors. In the contribution guidelines we > >>> ask > >>> contributors to let us know when they want to work on an issue. So far > >>> our > >>> attitude has been, yes sure go ahead. I've seen very little attempts of > >>> warning somebody to work on issues that won't be easily merged. > >>> Once a PR has been opened, we should also be honest and let > contributors > >>> know that it has no chance or might take a while to get reviewed. > >>> For 2) this is typically not so much of an issue if the feature is > >>> interesting. However, if 1) and 2) meet, chances to get a change in > drop > >>> even more. > >>> > >>> A common "strategy" to deal with PRs that fall into 1), 2), or 3) is to > >>> not > >>> look at them or giving a shallow review. > >>> Of course, contributors become unresponsive if we don't look at their > PRs > >>> for weeks or months. But that's not their fault. > >>> Instead, I think we should be honest and communicate the chances of a > PR > >>> more clearly. > >>> > >>> Browsing over the list of open PRs, I feel that most older PRs fall > into > >>> the category of low-priority (or even unwanted) features. > >>> Bug fixes or features that the committers care about usually make it > into > >>> the code base. > >>> In case a contributor becomes inactive, committers often take over an > >>> push > >>> a contribution over the line. > >>> > >>> So, I do not support an auto-close mechanism. > >>> I think a better way to address the issue is better communication, more > >>> eagerly closing PRs with no chance, and putting more reviewing effort. > >>> IMO, we should only eagerly close PRs that have no chance of being > >>> merged. > >>> PRs with low-prio features might be picked up later (for Flink 1.5, I > >>> merged a contribution from PR #1990 after it was requested a few times > by > >>> users). > >>> > >>> However, I think we could do a pass over the oldest PRs and check if we > >>> can > >>> close a bunch. > >>> There are quite a few contributions (many for flink-ml) that I don't > see > >>> a > >>> chance for getting merged. > >>> > >>> Best, Fabian > >>> > >>> > >>> - > >>> > >>> 2018-05-15 9:13 GMT+02:00 Chesnay Schepler <ches...@apache.org>: > >>> > >>> -1 > >>>> > >>>> For clarification (since the original mail indicates otherwise), the > >>>> number of pull requests that this would affect is fairly small. > >>>> Only about 25-30% of all open PRs are blocked on the contributor, the > >>>> remaining ones are actually blocked on the review. > >>>> Thus is reject the premise that one has to search through that many > PRs > >>>> to > >>>> find something to review, there is plenty. > >>>> > >>>> I believe it to be highly unfair for us to close PRs due to > inactivity, > >>>> when the primary cause has been /our /own inactivity. > >>>> If a PR is opened and the first comment comes in 3 months late, then I > >>>> don't blame the contributor for not responding. > >>>> IMO we owe it to the contributor to evaluate their PR, and if > necessary > >>>> bring it to a merge-able state (to a certain extend). > >>>> > >>>> There's also the fact that closing these PRs outright would waste a > lot > >>>> of > >>>> good contributions. > >>>> > >>>> Finally, this solution is overkill for what we want to achieve. If we > >>>> want > >>>> to make it easier to find PRs to review all we need is > >>>> GitBox integration and tagging or PRs. That's it. We could have a > /fully > >>>> /tagged PR list /tomorrow/, if we really wanted to. > >>>> > >>>> > >>>> On 15.05.2018 05:10, Ted Yu wrote: > >>>> > >>>> bq. this pull request requires a review, please simply write any > >>>>> comment. > >>>>> > >>>>> Shouldn't the wording of such comment be known before hand ? > >>>>> > >>>>> Otherwise pull request waiting for committers' review may be > >>>>> mis-classified. > >>>>> > >>>>> Cheers > >>>>> > >>>>> On Mon, May 14, 2018 at 7:59 PM, blues zheng <kisim...@163.com> > wrote: > >>>>> > >>>>> +1 for the proposal. > >>>>> > >>>>>> > >>>>>> Best, > >>>>>> blues > >>>>>> On 05/14/2018 20:58, Ufuk Celebi wrote: > >>>>>> Hey Piotr, > >>>>>> > >>>>>> thanks for bringing this up. I really like this proposal and also > saw > >>>>>> it work successfully at other projects. So +1 from my side. > >>>>>> > >>>>>> - I like the approach with a notification one week before > >>>>>> automatically closing the PR > >>>>>> - I think a bot will the best option as these kinds of things are > >>>>>> usually followed enthusiastically in the beginning but eventually > >>>>>> loose traction > >>>>>> > >>>>>> We can enable better integration with GitHub by using ASF GitBox > >>>>>> (https://gitbox.apache.org/setup/) but we should discuss that in a > >>>>>> separate thread. > >>>>>> > >>>>>> – Ufuk > >>>>>> > >>>>>> On Mon, May 14, 2018 at 12:04 PM, Piotr Nowojski > >>>>>> <pi...@data-artisans.com> wrote: > >>>>>> > >>>>>> Hey, > >>>>>>> > >>>>>>> We have lots of open pull requests and quite some of them are > >>>>>>> > >>>>>>> stale/abandoned/inactive. Often such old PRs are impossible to > merge > >>>>>> due > >>>>>> to > >>>>>> conflicts and it’s easier to just abandon and rewrite them. > Especially > >>>>>> there are some PRs which original contributor created long time ago, > >>>>>> someone else wrote some comments/review and… that’s about it. > Original > >>>>>> contributor never shown up again to respond to the comments. > >>>>>> Regardless > >>>>>> of > >>>>>> the reason such PRs are clogging the GitHub, making it difficult to > >>>>>> keep > >>>>>> track of things and making it almost impossible to find a little bit > >>>>>> old > >>>>>> (for example 3+ months) PRs that are still valid and waiting for > >>>>>> reviews. > >>>>>> To do something like that, one would have to dig through tens or > >>>>>> hundreds > >>>>>> of abandoned PRs. > >>>>>> > >>>>>> What I would like to propose is to agree on some inactivity dead > line, > >>>>>>> > >>>>>>> lets say 3 months. After crossing such deadline, PRs should be > >>>>>> marked/commented as “stale”, with information like: > >>>>>> > >>>>>> “This pull request has been marked as stale due to 3 months of > >>>>>>> > >>>>>>> inactivity. It will be closed in 1 week if no further activity > >>>>>> occurs. If > >>>>>> you think that’s incorrect or this pull request requires a review, > >>>>>> please > >>>>>> simply write any comment.” > >>>>>> > >>>>>> Either we could just agree on such policy and enforce it manually > >>>>>>> (maybe > >>>>>>> > >>>>>>> with some simple tooling, like a simple script to list inactive > PRs - > >>>>>> seems > >>>>>> like couple of lines in python by using PyGithub) or we could think > >>>>>> about > >>>>>> automating this action. There are some bots that do exactly this > (like > >>>>>> this > >>>>>> one: https://github.com/probot/stale <https://github.com/probot/ > stale > >>>>>> > > >>>>>> ), > >>>>>> but probably they would need to be adopted to limitations of our > >>>>>> Apache > >>>>>> repository (we can not add labels and we can not close the PRs via > >>>>>> GitHub). > >>>>>> > >>>>>> What do you think about it? > >>>>>>> > >>>>>>> Piotrek > >>>>>>> > >>>>>>> > >>>> > >> > > >
