Currently the PR template includes the following commented-out snippet:
To help with the large amounts of pull requests, we would appreciate your
reviews of other pull requests, especially simple package updates.
I really try my best to follow this practice, however I suspect that many people – and myself occasionally – skip right past this important step. This seems like a missed opportunity: requiring every incoming PR to make progress on 2-3 pre-existing PRs would fundamentally tilt the scales towards clearing out the PR backlog.
What are people’s thoughts on including a short mandatory section in the PR template that requires linking to 2-3 other PRs that the author recently reviewed?
that definitely shouldn’t be a rule for first time contributions, or even for contributions of people who contribute very rarely. requiring someone to review something else before their contributions are accepted is a good way of discouraging new people from contributing at all, not least because new folks may just not know enough to effectively review anything.
even requiring it of maintainers may not be that helpful. if someone doesn’t have much time on their hands we might lose them, or even encourage junk reviews just to have reviewed something. checking that the linked reviews are “good” would place even more load on reviewers, which wouldn’t be great either.
while getting more reviews would be great, that doesn’t sound like a good way to get them
I think these are certainly valid points, but it’s worth noting that no system is perfect and IMHO the current system of an ever-increasing backlog is “more worse” than a system whereby people are required to chip in to help with the reviewing workload.
For some context: I read Nadia Eghbal’s book “Working in Public” recently (great book btw!), and it really got me thinking about sustainable OSS models. One big takeaway for me was that “drive-by” contributors can actually drag a project down. Hence my thinking that it may be a worthwhile trade to lose some drive-by PRs for the sake of a more smoothly functioning PR system for the more consistent contributors.
Perhaps the key here is to balance incentives and requirements… Maybe we have an exception for first-time contributors? Or we ask that people list other reviews they’ve completed but make it non-mandatory?
the number of open PRs has been pretty stable in the past five months for which we have records, see Grafana. cleaning out the old PRs that are long dead may be a more effective way to clear the backlog at this point. it actually looks like we’re good on that front?
There are a lot of unactionable “stuck” PRs that make that number seem big.
GitHub does not provide the appropriate tooling for a project of our scope. We would need a lot more granularity and control about the lifecycle of a PR, and how the numbers are used, e.g.:
It’s an interesting idea, well-motivated, and at first glance it seems like it might help.
But I’m dubious, just from basic queuing theory: broadly, increasing the amount of PR review that’s required to land each PR is actually just going to make the pipeline deeper and add latency, delaying the merge of PRs.
I’m not saying it can’t work, if the right balance of tuning and incentives and conditions is found, but I suspect it depends too heavily on doinmg so. It also probably conflates two kinds of work and values only one: some people (or circumstances) are really good at finding issues and raising fixes, while others are really good at review and editorial QA and consistency. We need both, and there’s often overlap - but we shouldn’t force that.
As others have mentioned it is a nice intention behind this but unfortunately wouldn’t work well. One reason github works well for us is drive by contributions: the bar to contribute is so low that we get such a large population of contributors. It doesn’t matter what percent never contribute again, or review, etc… a small percent of these people end up being these insanely amazing long term contributors and we likely never would have landed them if the initial bar to contribute was higher.
Maybe not require, but somehow be looked at earlier.
For Ludum Dare people create games in a weekend. Afterwards games are reviewed by other developers. You have a coolness rating that is based on the number of reviews you have given and the number of reviews you have received. Games are sorted by coolness rating. Games with high coolness still lack reviews received compared to the ones given. It motivates people to give reviews that way. The number of reviews received tend to align with the number of reviews given eventually.
This could also apply for Nixpkgs PRs. I’m not sure how the tooling can drive the rating and sorting of PRs, but I do know such a system did work in a very motivating way for Ludum Dare.
That way it isn’t required to give reviews, but it will be motivating to help with the visibility of your own PR (and thus getting it merged).
In this vein, I was hoping to make GitHub - jonringer/basinix: (WIP) Nixpkgs pull request review website something which make it easier to say “yes” to merging new PRs. And make reviewing really time efficient for people by doing a lot more in-depth analysis and testing of PRs than what is currently being done with just ofborg.
My personal experience with the nixpkgs reviewing process is that either my PRs are reviewed and merged almost immediately (huge shout out to @Sandro!), or they linger unreviewed and unmerged forever. At this point the process has been unreliable enough that I’ve given up and just started maintaining my own fork of nixpkgs. I would really like to contribute many of the things I’ve packaged upstream, but the process has just proven to be untenable IMHO.
The grafana dashboard is great btw! I wasn’t aware that existed, and I’m happy to see that the total number of PRs does not appear to be growing unchecked. I guess my experience may not be representative of the overall nixpkgs contributor.
@samuela my advice is to become annoying I kept pinging people until my PRs were merged.
I don’t recall specifics of reviewing your PRs personally (maybe I have once before, maybe not?) but your PR count is reasonably high, you present yourself as a well intentioned member of the community who intends to stick around and cares about it, your discourse presence is valuable, and you’re maintaining your own nixpkgs fork… I’m pretty sure it’s time you get commit access, right?
There are some things you can do to make your PRs easier to merge:
Make one PR per package/change. Shorter PRs take way less time to review and are more fun to review. It is totally fine to also change things like missing completion scripts in an update PR.
Look on your open PRs after a few hours when hopefully ofborg is done. If not please start the bot manually. That makes a huge difference since the reviewer don’t need to wait and maybe someone is pinged who can help you with the merge. Maybe something failed and you can already fix it without any reviewer looking at the changes. Maybe the rebuild count is high and you can already re-target to staging.
Compare your package to recently new packages from the same eco system. Are they doing something different? Maybe there are newer functions I could adopt?
Are there things you are unsure about or don’t know how to do in a cleaner way? Add a comment on that line. This can avoid back and forth questions why something is how it is and can’t be done in a nicer way. Most of the time such things should have a comment that the next person that looks at that code in 6 months still knows what’s going on. You never know, it might be you.
Thank you so much @aanderse! Becoming a committer was not something I had on my mind, but it def would help to fix my problem. I’m honored, and excited to get more of my work upstreamed!
How would people feel about a simple opt-in service that sends you X randomly selected PRs/week in an email? The types of PRs you get sent could be modified with custom search queries, eg. “only send me python package PRs”.
GH notifications are a firehose so I’m guessing many people just turn them off.
They send a PR every now and then, but only when it’s been inactive for some time. Personally I found that to be less helpful, since those were usually already reviewed - just either the maintainer hasn’t changed anything yet or the reviewer has forgotten about it, since I don’t have commit rights there wasn’t a lot that I could do.
Long story short, code triage doesn’t really help here
I kept pinging people until my PRs were merged.