Call for beta-testing reviewers

[timokau]

I just pushed a new version with a new usage model. This new model now makes it possible to assign reviewers and automate triage. For now I am the only reviewer. I'd like to have a handful more to scale up the test. Not too many though, as everything is still in development.

As a reviewer you can set a limit on how many active nixpkgs PRs you want to be involved in. For example if you have already created 2 PRs today and reviewed one more, you are involved in 3 PRs in the period of one day. If you have set a limit of 4PRs/day, marvin will still request one additional review.

4PRs is likely too much, start slow. Be aware that you're not only signing up for reviews, but also for marvin-mk2 beta testing. Things might go wrong, though I try to avoid that of course.

For now, I will get pinged on every PR that is marked as needs_merger. Your reviews will lead to something. Once that gets too much for me, I'll look for help on that front too.

The easiest way to get started is to just make a PR and add yourself here. The format may not be the most understandable, so I can also do that for you if you prefer.

Any interest @cole-h, @turion, @doronbehar, @glittershark, @symphorien, @fgaz, @evils, @JJJollyjim, @bbigras, @mdlayher?

[evils]

i was under the impression the intended reviewer interface was looking up the label like this
What is the effect of being a listed reviewer?

[timokau]

Yes, its both. Being listed as a reviewer will actively request your reviews on needs_reviewer PRs. It will only do so if you don't already have a lot of activity (determined like this) in nixpkgs recently, respecting everybody's "open source time and energy budget".

That is supposed to establish a stable "baseline throughput" of reviews. I want to shift away a bit from the burst-review model to something more sustainable, like a lot of people each reviewing a PR every day or every couple of days. Actively assigning reviews seems more realistic than expecting people to actively look for a single PR to review every x days.

If the capacity of the registered reviewers is not sufficient (which it probably won't be), that can be complemented by going through the needs_reviewer list actively when you have some additional time.

[doronbehar]

The easiest way to get started is to just make a PR and add yourself here. The format may not be the most understandable, so I can also do that for you if you prefer.
Any interest

You can add me as a reviewer. I think I won't be available to start out as soon as possible. Just to be clear, are we talking about actual Nixpkgs or the playground?

[timokau]

You can add me as a reviewer.

What rate-limit would you like? As described above, the bot will count how many active (within d days) PRs you are currently involved in and only request reviews from you when that number is below a limit l. So you need to set a d and a l. For example, you are involved in 5 PRs with d=1.

I think I won't be available to start out as soon as possible.

So you don't want to be added? ;)

Just to be clear, are we talking about actual Nixpkgs or the playground?

Nixpkgs proper. The playground is only as a first test for basic functionality before deploying. Its a bit too contrived for an actual practical test, only baptism by fire will do here.

[fgaz]

Feel free to add me as well, with a more long-term d=5 l=7

[glittershark]

Perhaps might be a good idea to limit reviewers based on the attributes of PRs, too? For example some reviewers (eg me) have hardware running darwin but not others.

Regardless, happy to review one or two PRs / week, however that translates to the d and l parameters

[turion]

I'd like to participate with d=7 and l=3. This is exciting!

[symphorien]

I'd also like to participate with d=7 and l=3

[doronbehar]

So you don't want to be added? ;)

Yea @timokau I'm a bit internally conflicted as my semester is about to finish and my tests are up coming. I'll ping you in 3 weeks or so when I'll feel ready to fully dive in.

Besides that, I really like:

As described above, the bot will count how many active (within d days) PRs you are currently involved in and only request reviews from you when that number is below a limit l

Which is damn sophisticated. 🥇 :)

[timokau]

Perhaps might be a good idea to limit reviewers based on the attributes of PRs

Maybe, at some point in the future. I'm not entirely sure its desirable though. Usually you won't need a darwin-specific reviewer, only maybe if the darwin build is failing.

Consider yourself a PR shepherd when marvin requests a "review" from you. Your goal is to push the PR forward in some way, that may mean reviewing it yourself or delegating to someone else if there is some specific requirement.

Regardless, happy to review one or two PRs / week, however that translates to the d and l parameters

That leaves some room for interpretation. It could mean that you want two review requests from marvin (not possible to set that since marvin doesn't remember who it requested where) or that you want to be "involved" in two PRs that are not your own or that you want to be "involved" in two PRs period.

I've chosen the most conservative third interpretation for now in #51.

[timokau]

We now have 6 reviewers, great!

I'm a bit internally conflicted as my semester is about to finish and my tests are up coming.

I understand, I'm in a similar situation. No pressure.

Which is damn sophisticated. 🥇 :)

Thanks :) I'm not sure if its scalable enough, since it requires firing of a GH search before requesting anybody as a reviewer. GH ratelimits those pretty tightly. Some caching will probably help there though once we're running into issues.

[turion]

Perhaps might be a good idea to limit reviewers based on the attributes of PRs

Maybe, at some point in the future. I'm not entirely sure its desirable though. Usually you won't need a darwin-specific reviewer, only maybe if the darwin build is failing.

I agree. If the first reviewer thinks there is some special Darwin (or other platform) knowledge necessary, then this is not the first step in the review.

[turion]

Is there any mechanism to find a different "PR shepherd" if the first one isn't fit for the job (e.g. inappropriate responses of some kind)?

[timokau]

Is there any mechanism to find a different "PR shepherd" if the first one isn't fit for the job (e.g. inappropriate responses of some kind)?

You can manually set the status back to needs_reviewer. Marvin will also do this automatically when the reviewer is unresponsive (in awaiting_reviewer or awaiting_merger for 3 days without any update).

Setting the status to needs_reviewer will trigger a triage run, which will assign a new reviewer if available. Older PRs have precedence though.

[timokau]

In case anybody was missing any bot actions: A bug crashed the triage runner. I forgot that you can only request reviews from people who have been "explicitly granted read permissions" to the repo, i.e. people in the maintainer list. Marvin attempted to request a review from someone who wasn't a maintainer. I have implemented a fallback for that case and re-deployed.

[timokau]

By the way, we are currently at capacity. So if a PR isn't immediately being assigned a reviewer, that is probably not a bug:

Jul 09 16:31:38 marvin-mk2: Running triage early
Jul 09 16:31:39 marvin-mk2: Running triage on NixOS/nixpkgs
Jul 09 16:31:39 marvin-mk2: Timing out awaiting_reviewer PRs
Jul 09 16:31:39 marvin-mk2: Timing out awaiting_merger PRs
Jul 09 16:31:40 marvin-mk2: Assigning mergers to needs_merger PRs
Jul 09 16:31:40 marvin-mk2: Assigning reviewers to needs_reviewer PRs
Jul 09 16:31:40 marvin-mk2: Selecting reviewer from candidates: ['ryantm', 'glittershark', 'symphorien', 'timokau', 'fgaz', 'turion']
Jul 09 16:31:40 marvin-mk2: Testing symphorien
Jul 09 16:31:41 marvin-mk2: Active in 18 PRs, limit is 3.
Jul 09 16:31:41 marvin-mk2: Testing fgaz
Jul 09 16:31:41 marvin-mk2: Active in 7 PRs, limit is 7.
Jul 09 16:31:41 marvin-mk2: Testing ryantm
Jul 09 16:31:42 marvin-mk2: Active in 3 PRs, limit is 1.
Jul 09 16:31:42 marvin-mk2: Testing turion
Jul 09 16:31:42 marvin-mk2: Active in 4 PRs, limit is 3.
Jul 09 16:31:42 marvin-mk2: Testing timokau
Jul 09 16:31:42 marvin-mk2: Active in 5 PRs, limit is 1.
Jul 09 16:31:42 marvin-mk2: Testing glittershark
Jul 09 16:31:43 marvin-mk2: Active in 5 PRs, limit is 2.
Jul 09 16:31:43 marvin-mk2: No reviewer found for #92336.

[glittershark]

Does it check authored PRs? I'm not currently active in reviewing any PRs, only PRs I wrote

[timokau]

Yes, it counts everything that github calls "involves" (authored/left a comment/has an outstanding review request/approved/...). You don't have to be active yourself in the PR either, you just have to be "involved" at some point and the PR has to be active. The assumption is that you're probably still following it and are getting notifications about it.

[turion]

Yes, it counts everything that github calls "involves" (authored/left a comment/has an outstanding review request/approved/...). You don't have to be active yourself in the PR either, you just have to be "involved" at some point and the PR has to be active. The assumption is that you're probably still following it and are getting notifications about it.

Just to be sure: But only those PRs count that have had activity in the last d days?

[timokau]

Right, that is what I mean by active. To be precise, anything github counts as an "update" in the last d days. Activity after merge doesn't count (since otherwise it would count as an activity when the PR author decides to delete the branch after merge).

The criteria could be amended of course (even on a per-reviewer basis), as long as the info is easily obtainable by GitHub. The intention behind the current criteria is to have some approximation of your general "involvement" in nixpkgs and not to overload your plate to the point where reviews feel like too much of an annoying chore. Intuitively, after having received 100 emails from PRs I am subscribed to and having responded to 20 reviews on my own PRs, I don't want to be asked to review 5 more ;)

[timokau]

Marvin briefly ignored the rate limit due to a bug. Fix is deployed (#80). Sorry for the inconvenience.

[turion]

I haven't been notified by the bot for a long time now. Is it working properly?

[fgaz]

Same, but it may be because I'm already over the limit I set just from my normal activity

[kevincox]

Hi, commenting from the issue just referenced. I was the original requested reviewer for that PR but unfortunately I didn't feel adequately able to review the PR. I think there are two main problems here.

1. While I can review most PRs in nixpkgs there are a couple of corners where I don't feel comfortable. I think we need to allow reviews to step out.
2. Your comment seems to suggest that the bot tried to "re-assign" me, which wasn't great in the first place but also gave me no indication of that.

My first thought for how to address this would be something like the following:

1. Try to assign to maintainers first.
   a. There are other bots that do this so maybe we can rely on them?
   b. Current maintainer detection IIUC is by nixpkgs metadata, which doesn't work great for modules, tests and similar. Maybe we could have a fallback mechanism such as CODEOWNERS files that was more reliable for critical parts of the tree (like the Linux kernel).
2. Have an explicit command for a reviewer to opt out, whether they are unable to review due to knowledge or temporary unavailability.

I hope we can keep the bot going. I think it does a lot of good overall but I understand that it can be hard to run it as a 1-man show. (And unfortunately I don't have the ability to help out right now.)

[turion]

1. While I can review most PRs in nixpkgs there are a couple of corners where I don't feel comfortable. I think we need to allow reviews to step out.

I asked that question some time ago on discourse, and the answer was, paraphrased:

It's fine not to be knowledgeable in all parts, you can still be a good reviewer.

• If you come across something unfamiliar, you can always try to summon an additional interviewer who is expert on the topic. You might find them by looking at maintainers of similar files, git history, community activity and so on. They can answer a specific question, while it's still your job to drive the process. In that situation, you're more like the assignee of a MR (i.e. the person responsible for progress) than the reviewer (i.e. the person responsible for a good review). My take: Sometimes this works great, sometimes noone answers.
• Take PRs with unfamiliar content as a chance to learn something new. Take your time, familiarise yourself with everything. Possibly use the previous steps to figure out parts which you're still unsure about. My take: Works great in theory, and sometimes also in practice, but not if you don't have a lot of time and energy at hand. (But if you don't have time and energy, you should probably cut down on volunteering anyways somewhere.)

Another thought for having better matches between PRs and assigned reviewers would be opt-in and opt-out of specific labels. E.g. I might want to review only PRs with the haskell label in them, or never PRs with the python label.

[timokau]

For those who only follow by email notifications, here is the relevant part of my comment [1] that started the discussion:

It seems like the bot is not really sustainable with me as the sole maintainer given my current priorities. Maybe it would be better to shut it down. It would be sad to abort this experiment without exploring it a bit further, but that may still be better than the current state. The bot is too spammy in its current configuration and may actually put off contributors or generate unnecessary stress. I think I will post a comment to timokau/marvin-mk2#34 soon-ish and ask for peoples opinions.

[1] NixOS/nixpkgs#121518 (comment)

[kevincox]

On 13/08/2021 02.58, Manuel Bärenz wrote: It's fine not to be knowledgeable in all parts, you can still be a good reviewer.

I agree. And I definitely plan on following this review. But I was just way over my head. Not only was the code complex (that's fine I can deal with it) but there appears to be some domain-knowledge changes. I don't think I can meaningfully help with those with my current knowledge. I did suggest a reviewer that touched this code recently so that we can get an expert opinion and I could learn. While I agree that you can still be helpful in these cases I think there is a limit. In this case my limit was low enough that I needed a second opinion. I don't think that the goal is to encourage "punt on hard reviews" I just think it is important to have a graceful way to request another opinion. Whether that is due to lack of required domain knowledge or temporary unavailability.

[timokau]

1. While I can review most PRs in nixpkgs ohere are a couple of corners where I don't feel comfortable. I think we need to allow reviews to step out.

I agree with @turion's suggestions, but I also agree that volunteers need much more control over the review requests. That's part of why I am thinking of shutting the bot down: The current way it handles things is likely a source of stress for reviewers.

Reviewers should be able to

• opt out of individual reviews with very low friction. They currently kind of can opt-out, but that doesn't work perfectly (e.g. the bot may re-request the review) and isn't low friction. This needs both technical and non-technical improvements. On the non-technical side we need a social standard that emphasizes it's okay to opt-out any time, without any need to explain yourself. That could be encouraged with proper wording and configuration of the bot.
• configure which PRs they want to review as @turion suggested. There are advantages to everybody being pinged on everything (it spreads knowledge around, non-expert reviewers may actually be in a better position to require good docs), but people may still prefer some kinds of PRs over others. The bot should respect individual preferences.
• configure how many PRs they want to review. This is currently possible, but has way to much friction. It requires a PR to marvin-mk2 and a deployment. Configuration through nixpkgs (as part of the maintainer list?) would be better, but still not ideal. People are unlikely to do that in high-stress phases, and that may be an additional source of stress. At some point I had a prototype that allowed decentralized configuration through a personal github gist. Something like that would be better.

2. Your comment seems to suggest that the bot tried to "re-assign" me, which wasn't great in the first place but also gave me no indication of that.

Yes, that is #86.

My first thought for how to address this would be something like the following:

1. Try to assign to maintainers first.
   a. There are other bots that do this so maybe we can rely on them?
   b. Current maintainer detection IIUC is by nixpkgs metadata, which doesn't work great for modules, tests and similar. Maybe we could have a fallback mechanism such as CODEOWNERS files that was more reliable for critical parts of the tree (like the Linux kernel).

My intention was that the marvin-assigned reviewer is more like a "shepherd" who can then pull-in additional reviewers (like the maintainers) where it makes sense. As I already mentioned, having everyone review everything has some benefits. But I agree that assigning maintainers first where possible is probably a better use of our limited people-power.

2. Have an explicit command for a reviewer to opt out, whether they are unable to review due to knowledge or temporary unavailability.

Yes, definitely.

I hope we can keep the bot going. I think it does a lot of good overall but I understand that it can be hard to run it as a 1-man show. (And unfortunately I don't have the ability to help out right now.)

There are a lot of low-hanging fruit for improvements. The lowest-hanging one would be to finally fix the automatic reset to "needs_reviewer". We should also decrease the nagging frequency, make it easier to contribute (#113) and improve automatic testing.

The problem isn't that there is no room for improvement. It is that it has been a few months now, and I haven't gotten around to making these improvements yet. I feel bad about these long-unfixed rough edges, but at the same time it's unlikely to become a high enough priority for me any time soon.

I already argued that the bot may stress out reviewers. Leaving these issues unfixed stresses me out as well. Then there's the risk of something going wrong with the bot. It should really be maintained more actively, but I'm not in a position to do that right now. That's why I think it may be better to shut it down.

If anyone wants to give it a try, contributions would be very welcome. Unfortunately getting up and running is not trivial right now. #113 would be a good starting point.

[timokau]

I agree @kevincox. As I wrote in my last comment, I think reviewers should have much more flexibility on how they choose to volunteer their time.

(Replying separately because we were apparently writing our comments at the same time.)

[LunNova]

I would like to help maintain this, will look into implementing improvements which improve friction for reviewers.

[timokau]

That would be great! Please make sure you test your changes on a playground repository first though, see #113. Verification is the currently the greatest hurdle when making changes. Making that part easier would probably be the change with the highest leverage and would also be a requirement for leaving the experimental stage at some point.

[timokau]

I have just disabled marvin-mk2 by setting the github webhook to inactive.

Marvin was an interesting experiment, but it is not without its issues right now. It tends to spam PRs, does not have an easy mechanism to temporarily opt-out in stressful times and in general feels a bit rough around the edges. See this post for details on its current shortcomings.

It was never intended to exist in this state for as long as it has. It was supposed to be a proof of concept, to be iterated on and eventually discussed by the wider community. Unfortunately I do not have the bandwidth to properly maintain it right now, let alone improve it.

My thanks to all the "beta-testing reviewers" and everybody else who has helped along the way. We have certainly helped some PRs move along faster than they otherwise would have. Thank you for volunteering your time!

[turion]

Thank you @timokau for spending a lot of time and effort into this experiment! I think it was very helpful to understand better what kind of infrastructure nixpkgs might profit from.