Add the mention bot?

[dstufft]

One feedback people have been having so far is that the firehouse of notifications from Github on python/cpython is too much and they're having to unwatch the repository. That is a reasonable thing to do (I, for instance, do not watch the repository). However that can make it harder to get people to review PRs since they'll no longer be notified of news ones and they'll have to go out of their way to look for them.

One solution to this problem is https://github.com/facebook/mention-bot. It will attempt to intelligently notify people on new PRs (by CCing them) who it thinks is a good fit to review the code (effectively, which people were working around this area most recently). This bot also has a feature that might help with the module owner issue mentioned in #29, which it allows people to configure themselves to always get notified for certain PRs that touch certain files. For people who never want to be automatically notified, they can add themselves to the configuration blacklist and it will never select them as a potential reviewer. It has a number of other features (skipping if the person is a contributor, delaying the mention for X amount of time to let a PR get a review "naturally" before notifying people, etc) that attempt to reduce the amount of notifications it gives as well.

[brettcannon]

Since the bot has an open endpoint so we don't have to keep it running then I'm +1 for giving a try. I guess we would require someone be in the Python org in order to be mentioned? And would we want to blacklist any specific files?

[dstufft]

I would say yea you'd want them to be in the Python org.

We probably want to blacklist files that regularly get updated in PRs that are not related to the actual change. So Misc/NEWS, Misc/ACKS, etc. That will prevent someone from getting flagged as a possible reviewer just because they happened to have edited Misc/NEWS also. Not sure if there any other files like that but if there we probably want to blacklist them too.

I'm on the fence about delaying. On one hand I think it could reduce some noise if people are asking each other for reviews themselves in IRC or whatever, but on the other hand it's a nicer experience if someone is able to quickly review instead of having it happen X hours later.

[brettcannon]

We can try it without a delay and then scale one up if we find it too noisy.

[orsenthil]

Yeah, I am open on trying this. We can evaluate how helpful this is after we try.

[dstufft]

In python/cpython#280 a bit of a process foul up happened when a change to a security only branch got merged without the explicit approval of the RM for that branch. An additional feature that would be useful for the mentionbot is to see what branch a change is being merged into, and if it's a security only branch then either assign or request a review for the RM for that branch.

Looking at the docs for mentionbot, it doesn't currently support this, but presumably we could add it.

[warsaw]

You know I did that on purpose, to expose the glaring weaknesses in our process. It wasn't at all me being an idiot.

[dstufft]

Looking at the mentionbot PRs, it might actually already be workable as is. It appears that it pulls the configuration from the target branch. Which is somewhat helpful in that it allows people to control their settings on a per branch basis (on the flip side, settings must be controlled on a per branch basis).

But that means that on each security branch, we can set it up to always notify the relevant release manager (and probably only notify them, if all changes have to go through them).

[berkerpeksag]

alwaysNotifyForPaths, assignToReviewer and createReviewRequest settings of mentionbot would be really useful, but I'm not really fan of the default "here's a list of potential reviewers" comment.

[brettcannon]

I have yet to hear anyone say this is a bad idea, so do you want to draft up a PR for master @dstufft and we can give it a shot? I can add the webhook once the PR is ready.

[dstufft]

Alright @brettcannon , done in python/cpython#352.

• It doesn't delay notification.
• It ignores Misc/ACKS and Misc/NEWS assuming that those files are going to get touched a lot in ways not related to the change that needs reviewed.
  • This will of course need updated with the outcome of Generate Misc/NEWS from individual files #6.
• I left the place where people would add themselves to the file to have the mention bot ignore them and never mention them in a comment. This allows people to opt-out of participating by simply editing .mention-bot.
• If we get a bot that automatically creates backports, we could add it to userBlacklistForPR so it doesn't create additional review comments.
• I required that to be mentioned, someone should be in the python org. Ideally we could narrow that down even further by saying what team they need to be on. This is being tracked by Mention only users who are part of a team facebookarchive/mention-bot#92.

We should probably modify the release procedure for moving a release to a security only release to either remove this file, or modify it so that it says:

{
  "findPotentialReviewers": false,
  "createReviewRequest": true,
  "assignToReviewer": true,  // This is an alternative to createReviewRequest
  "alwaysNotifyForPaths": [
    {
      "name": "the-release-manager-for-this-branch",
      "files": ["**/*"]
    }
  ]
}

This should automatically either assign or create a review request (depending on which option we pick) the release manager for that branch on each PR.

In addition, when cutting a brand new release branch (but not a security release branch) we'd probably want a procedure to just delete this file. New PRs should be coming in on master after that branch is created anyways so the primary thing creating old PRs will be backports which should have already been reviewed.

[brettcannon]

OK, PR is merged and the webhook has been added. Now I guess we wait and see it work. 😄

[Mariatta]

Hmm.. do you think we can disable the mention-bot in cherry-pick PRs?

[dstufft]

@Mariatta Can you link an example?

[Mariatta]

@dstufft This PR? python/cpython#364
Perhaps if the description starts with [3.6] or [3.5] then we know it's a cherry-pick.

[dstufft]

I think we just need to add a .mention-bot file with the contents:

{
   "findPotentialReviewers": false
}

I'll make a PR to 3.6 and we can try it!

[dstufft]

This should fix it (for 3.6 anyways) python/cpython#365. It won't work until it's been merged though.

[dstufft]

Testing in python/cpython#368.

[Mariatta]

lol I'm also testing it in python/cpython#367

[dstufft]

WELP

[Mariatta]

Seems to work. Thanks @dstufft 🎉
Backport to 3.5 and 2.7?

[dstufft]

Yea we should back port it to any of the maintenance branches. For security only branches we probably want something like:

{
  "findPotentialReviewers": false,
  "createReviewRequest": true,
  "assignToReviewer": true,  // This is an alternative to createReviewRequest
  "alwaysNotifyForPaths": [
    {
      "name": "the-release-manager-for-this-branch",
      "files": ["**/*"]
    }
  ]
}

Instead. Not sure if we would prefer that the RM for security branches get notified by:

1. Comment
2. Review Request
3. Pull Request Assigned

[dstufft]

Here's backports for 3.5 and 2.7: python/cpython#370 python/cpython#369

I haven't done anything for 3.4 or 3.3, we'll need to pick which of the three options we want for that (or if we want to just disable it completely for 3.4 and 3.3 like we did for 3.5, 3.6, and 2.7).

[dstufft]

Another question is should we document somehow how to, as a core committer, document how to opt-out of mention-bot bothering you? It's fairly simple, you just add your GH username to userBlacklist but presumably we should mention it somewhere.

[pfmoore]

Is that opt-out via a commit to the cpython repo? That sounds like it could result in a bit of churn. What if I wanted to temporarily opt out?

[dstufft]

Yea commit to the master branch on the CPython repository, the other branches have it disabled. If you wanted to temporarily opt out you'd have to add yourself and then remove yourself, although I suspect that it's not going to amount to much churn long term. I figure anyone who doesn't want mentioned by it will add themselves to the file once and that will be the end of it. I don't personally see a large use case for opting out temporarily but I could just be missing it.

Once the set of current committers who don't ever want mentioned gets added the .mention-bot, it should generally not get touched unless we add a new committer who also wants to not be notified (or someone changes their mind).

[berkerpeksag]

I doubt findPotentialReviewers will be useful because of svnmerge commits and massive cleanups (e.g. tabs to spaces) It might be better to disable findPotentialReviewers and fill alwaysNotifyForPaths manually (also probably set createReviewRequest to true). Otherwise, we are going to end up with adding half of the core developers to userBlacklist.

[pfmoore]

@dstufft OK, fair enough. At the moment I'm getting flooded by a lot of github traffic, and I'm pretty confused where it's all coming from. At some point I'll want to start selectively changing settings to see if I can get things down to a manageable level (without just blanket switching everything off). I'm not particularly comfortable with the need to commit to the main repo just to do that sort of settings testing (will I need to go through the PR/commit route to do that?) but if that's the way to do it then OK. (I suspect that actually the mention-bot stuff isn't my real issue anyway, so maybe I'll not need to do anything).

[brettcannon]

I'm shutting the bot off for Guido at his request in python/cpython#385 because he wrote so much code he's getting hammered.

[brettcannon]

I also noticed that Benjamin has already shut the bot off.

[dstufft]

Interestingly, he shut it off for PRs that he makes rather than shutting it off from mentioning him. It's unclear if he did that on purpose or if he meant to make it stop mentioning him.

[brettcannon]

I guess we can ask @benjaminp what he meant to do. 😉

[benjaminp]

I changed what I meant to change. :) Every time I created a PR, I felt like I was spamming 5 people and myself—I wish the bot could subscribe people without leaving a message. The value of the bot for newer contributors is probably higher than it is for me; I expect can I manually find the right person or people to review a change.

I don't mind being mentioned by the bot so far, though, it may not be terribly high signal for me, since back in the day, I svnmerge.pyed many times from trunk (2.x) to py3k. Overall, using the notifyPaths configuration (basically formalizing the experts list) seems like the way to go.

[serhiy-storchaka]

It seems to me that the mention bot's algorithm doesn't work good. For example in python/cpython#375 it mentioned people that didn't touch the code for years, but missed me who made most changes in this file in recent years and wrote affected code.

[dstufft]

@benjaminp So it can create a review request instead of (i think?) leave a comment. Since we're not requiring reviews there's not really a downside to that. However I don't know that it's functionally differently in terms of spam since Github will generate an email either way.

Another option is to only generate comments for people who are not members of the repository, but I think possibly prodding people to review (even a post review) even of committers is a good thing.

There are two options for formalizing the experts list, one option will always notify the experts (alwaysNotifyForPaths) and the other option will use the experts as a fallback for whenever it can't locate other people to notify (fallbackNotifyForPaths).

@serhiy-storchaka I was thinking about that, and I think I know what the problem is. I configured it so that it limited it's mentioning to only people who are a member of the Python organization on Github. Looking at facebookarchive/mention-bot#209 it appears like it's ignore anyone who hasn't set their visiblity in the Python org to be Public. I'm poking at it to see if there is a good way to give the mention-bot visibility even if people haven't set themselves to public.

[berkerpeksag]

However I don't know that it's functionally differently in terms of spam since Github will generate an email either way.

GitHub will only send an email to the person we want to notify. For example, I haven't got any "benjaminp requested a review from warsaw" email from python/cpython#390.

[dstufft]

@berkerpeksag Are you subscribed to the repository? That is probably a difference I didn't think about. I am not subscribed.

[berkerpeksag]

@berkerpeksag Are you subscribed to the repository?

Yes, I'm subscribed to python/cpython. Implementing #32 (comment) would probably reduce email rate (and if they don't want to be notified they can either remove themselves from alwaysNotifyForPaths or add themselves to userBlacklist.)

[dstufft]

So requiredOrgs is basically not super workable right now, it can only ever function on public members as it is currently written in mention-bot (and due to the permissions issue, if we wanted it to work on private members as well we would have to run our own bot). This currently limits the scope of people who can be mentioned from 138 down to 50.

So we have three choices:

1. Leave it as it is, if people don't make their membership in the Python org public they will never get mentioned.
2. Remove the required organization, allowing the mention bot to mention anyone. The upside is that this might encourage even non-committers to review and might help notice people who do a good job of reviewing to bring onto as a committer. The downside is that if we want to require reviews again it might not poke anyone who can give it a "real" +1. This is implemented in Don't require the Python org to mention someone cpython#393.
3. Deploy our own bot, configured as a member of the Python org. This requires Filter requiredOrgs by a method that understands private members facebookarchive/mention-bot#212 to actually function correctly (I think). I believe this only requires running a single Dyno on Heroku so it's not very intensive to run. We would need to pick an available name (I registered pymention-bot, but for some reason Github immediately flagged the acount....).

[dstufft]

Huh, interesting. I went to switch the bot over to request reviews instead of comment, but looking at the code it's not written as a "instead of", but rather "in addition too"... make it a far less useful setting.

If we want to do that, we'll need to land a PR in mention-bot to allow toggling the comment off or we'll need to fork it and run it ourselves. It didn't look hard so I went ahead and opened facebookarchive/mention-bot#213 to add a feature to do that.

[berkerpeksag]

Thanks, @dstufft! Let me know if they require a test case to merge facebookarchive/mention-bot#213. I can help with that :)

[serhiy-storchaka]

Thanks @dstufft! I did it and now the mention bot mentions me. I expect to get much "spam" since now. :-)

[ncoghlan]

Writing https://mail.python.org/pipermail/python-committers/2017-March/004281.html to explain the gist of the mention-bot to committers made me realise that there should probably be a devguide addition in relation to this (probably a new subsection in https://docs.python.org/devguide/coredev.html#gaining-commit-privileges )

[dstufft]

@ncoghlan The mention-bot is currently an experiment and is one of the things that will be discussed/decided on whether to keep it or not in a week or two (IIRC). I agree that once that's happened if we decide to keep it we should document it.

[Mariatta]

So I have a very trivial change that doesn't require spamming 5 reviewers (see python/cpython#433)

Maybe we want to consider reducing the maxReviewers to 3?

[dstufft]

I don't have an opinion on the number of reviewers, but one possible option is configuring a skip phrase, like [no-mention] where if added to the title of a PR it won't mention anyone.

[serhiy-storchaka]

Could the mention bot be configured to ignore generated files: Argument Clinic files, frozen importlib modules, Unicode DB, etc?

[dstufft]

Yea. I already have it ignoring a few files like Misc/NEWS. Just make a PR to add whatever other files you think should be ignored.

…

Sent from my iPhone

On Mar 4, 2017, at 9:46 AM, Serhiy Storchaka ***@***.***> wrote: Could the mention bot be configured to ignore generated files: Argument Clinic files, frozen importlib modules, Unicode DB, etc? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[brettcannon]

Closing this issue since the mention bot is still on; we can use new issues to tweak or discuss turning it off if that comes up.