Most contributors to nixpkgs are probably well-aware of the huge open PR count on nixpkgs. I’ve decided to gather some statistics to determine how bad the situation really is.
Here you can see all PRs grouped by state over time:
PRs created each month grouped by (current) state, relative to total:
About 90% of PRs are resolved in less than six weeks.
You can inspect the plots yourself at Github Pull Request Statistics. I’ll publish the source code for the analysis tool soon.
What dataset did you use/how did you acquire this data?
I acquired the data from the official Github API. You can check out the full program here: GitHub - FliegendeWurst/pr-stats
what’s the net rate of open PRs? it seems that for a long time, PR count has never really decreased, and I feel like having some concrete numbers would help confirm or deny that. doing so for issues as well would be doubly cool, though please don’t feel obliged to do ither, just something I’m curious about.
Depends on how you define “really”, we have reduced the number of PRs from 6700 to 6200 in the past few weeks, and from 6200 to 5700 a few months ago. Of course, the degree of completeness of the online committers varies at different times. It was relatively complete in the last two weeks.
ah, I wasn’t aware of those ongoing effots. to be clear, I didn’t mean there was no effort, just from my own recollection, I hadn’t remembered ever seeing a substantial reduction in open PRs. I’m very glad to have been proven wrong though, thanks to everyone on the committers team for their hard work!
I think people place way too much importance on this metric. I don’t find it to be of any usefulness other than perhaps a measure of growth.
Let’s take a simplified example to show why I think that way: Let’s say there was exactly one PR opened every hour and each opened PR was resolved (closed or merged) exactly 24h after it was opened.
In this scenario, you’d expect to have 24 open PRs at any given moment because there’d be one PR opened and another one closed in every hour. (After an initialisation period of one day of course.)
If the rate of PRs opened and resolved were to rise to 10 times as much but all of them were still processed and each still processed in the same amount of time, you would naturally expect to see 10 times as many open PRs at any given time too.
IOW the number of open PRs at any given moment is directly proportional to the rate of PRs processed if the time it takes to process any given PR remains constant.
It’s much like real estate vacancy in the real world: The more real estate there is, the more of it will be vacant just by the nature of being in the state transfer between residents.
Just like it’s to be expected for a building to be vacant for a little while after its previous resident has moved out, it’s also to be expected for a PR to take a little while to resolve.
We should therefore not measure our success at processing PRs by how many PRs are open at any given time but rather by how long it takes to process any given PR.
If we are able to keep up, you’d expect the time to remain constant. If we are no longer able to keep up, you’d expect that time to rise, eventually to infinity.
I’d consider it a great success if we managed to keep the time to process each PR constant. It’d be even better to slightly improve it but I’d consider that a “stretch goal” and I’d expect diminishing returns to come into play here quickly.
What I also think is important to consider here is that not all PRs are created equal. Getting a refactor merged quickly is not nearly as important as getting an important security or other bug fix merged quickly.
I’m no statistician, so I can’t be of any help at defining these “time to process a PR” metrics in detail or deduce historical data. I am not ignorant of the fact it that it likely isn’t trivial though.
Setting in motion an effort to rigorously categorise PR complexity and priority using issue/PR labels would be something I could help with and it has been on my TODO list for a while but I haven’t had the spoons yet. This would be a rather cheap way to help us prioritise PRs to get the important ones landed quickly and measure our effectiveness in doing it.
A few months ago (~late July) someone had posted relevant stats, indicating that the average PR was merged in about a day. The average PR also came from r-ryantm, which means it’s mostly trivial stuff getting merged, there wasn’t any info on how the situation was when excluding bot PRs. I searched and couldn’t find that thread now, though I think @Aleksanaa also commented on that thread?
Here is the number of days required to merge X% of PRs created each year (only counting PRs ever merged, including bot PRs).
| Year | 25% | 50% | 75% | 90% | 95% | 98% |
|---|---|---|---|---|---|---|
| 2012 | 0 | 0 | 4 | 13 | 22 | 34 |
| 2013 | 0 | 0 | 2 | 9 | 19 | 44 |
| 2014 | 0 | 0 | 2 | 12 | 30 | 52 |
| 2015 | 0 | 0 | 2 | 10 | 25 | 71 |
| 2016 | 0 | 0 | 2 | 8 | 18 | 48 |
| 2017 | 0 | 0 | 2 | 11 | 28 | 81 |
| 2018 | 0 | 0 | 3 | 11 | 26 | 65 |
| 2019 | 0 | 1 | 5 | 21 | 47 | 124 |
| 2020 | 0 | 0 | 5 | 23 | 58 | 138 |
| 2021 | 0 | 0 | 3 | 13 | 31 | 78 |
| 2022 | 0 | 0 | 4 | 17 | 40 | 100 |
| 2023 | 0 | 0 | 2 | 13 | 31 | 86 |
| 2024 | 0 | 0 | 4 | 13 | 27 | 55 |
Also interesting could be how effective asking for review really is
or how effective the checkboxes in the PR template are
To me, these numbers don’t show anything bad really, at least not by themselves.
I’d like to see more clearly articulated norms around time-to-review and time-to-merge.
For instance, @hexa chastises me in this PR for failing to give time for code owners to sign off. (I think this is fair; I’m using it for illustration.)
The core problem is the coordination and expectations.
acceptance acceptable?
request changes acceptable? How long to wait for the original reviewer to dismiss their own review?Most of these things have to do with timing. “Indefinitely” is a terrible answer for project cohesion and fun. No work gets done in the Sejm if there’s a Liberum Veto. But “immediately” is also a bad answer, because it means the first person to say yes (who likely has not thought that much about it!) determines the outcome for the project.
What I hope we can agree on and write down somewhere is norms for Nixpkgs in this area. Here are a few I would like to see.
PR authors stating who they expect to review their patches. “Do not merge unless X approves.”
PR reviewers stating time-to-merge expectations. “I will merge this sometime today unless someone objects strongly.” Doubly so for anything that’s being self-merged.
PR reviewers using the Request Changes status to note what’s blocking. “I request changes for reason 1, 2, and 3; every other comment is not blocking.”
If a reasonable person would believe that the requested changes have been implemented, other PR reviewers should feel free to dismiss their review status.
What do others think?
I don’t think self-merges are particularly necessary, if the PR was that urgent, then someone else would’ve noticed and merged. If it languishes, so be it… Self-merging with no approvals are the worst, it means the author got no feedback and is certain to break something that won’t get caught for (possibly) 4+ days.
Though I’ve tended to dimiss request-change nits as soon as I see them, as they clearly detract from contribution. Same goes for concerns that were addressed.
I think that is a pretty strong word for essentially asking for more time for codeowners in the future.
That being said, I’m all for self-merges for people who’ve done their due dilligence.
more time for codeowners in the future.
I think that’s the rub. For me, it’s not clear what “more time” actually means, and it’s also not clear who exactly is the codeowner from your perspective. According to ci/OWNERS, I’m an owner of some of the code in the PR! Yet it’s clear you didn’t see me as such. (Which is fine!) My commitment to nixpkgs wanes and waxes based on other time commitments, so there are months where I’m less responsive.
That mismatch in expectations drives my desire for clearer norms.
What is “more time”? When has it elapsed?
Who is the code owner? When does the timer on their attention run out?
Who should have merged the PR, from your perspective?
Again, I think your “Hey, please adjust your behavior” message is both appropriate, kind, and relevant. No worries on “strong words” from my side.
The substantial code in that PR was python related, so the relevant codeowner I was in fact waiting on was natsukium.
That is a good question. Certainly not before 24 hours, rather some time after 48 hours.
Ultimately I think of “code ownership” as a tool to make sure that maintainers get a chance to opine on the things that they spend a lot of time on.
