We need more official PR reviewers
- Wuzzy
- Member
- Posts: 4804
- Joined: Mon Sep 24, 2012 15:01
- GitHub: Wuzzy2
- IRC: Wuzzy
- In-game: Wuzzy
- Contact:
We need more official PR reviewers
I think I have finally found the #2 reason* why Minetest development is so slow: We have a major shortage in reviewers with approval rights!
What I mean with that is that there is only a tiny amount of people who are allowed to review Minetest submissions (pull requests on GitHub) and also give an official approval. As far I know, only core devs have official approval rights. On the other hand, PRs are pouring in at an alarming rate, the number of open (!) PRs hasn't dropped below 100 for a very long time. Also, a lot of PRs are very, very old, but still neither accepted nor rejected. >100 open PRs is a problem, because older (but high-quality) PRs get forgotten fast and it's hard to keep track of all of them in your mind.
Now add 1 and 1 together: We have a tiny handful of official reviewers (=core devs) while simultanously Minetest is bombarded with new PRs. Now it's pretty obvious why progress is so slow. The reviewers are just being overwhelmed.
Therefore, I propose one simple thing: Add more people with official approval rights. These people can give an approval to PRs that has the same weight as a core dev's approval.
How to decide who to make an official reviewer? I don't really know, but obviously these should be skilled people who know what they're doing and are aware of the Minetest development rules and they should also be not be afraid of rejecting bad PRs. Don't read this proposal as a sneaky way to sneak in every possible PR into Minetest, no matter how crappy. It's more about speeding up the process which decides whether a PR gets green light or red light.
Anyway, I think it shouldn't be too hard to find good reviewers. Our community is actually quite large, surely there are some good people. Right?
I imagine if the amount of official reviewers increases, Minetest will be able to significantly deal with a lot of PRs faster, and eventually get the number of open PRs below 100, below 50, below 25, below 10. Once the number of simultanously open PRs is low, dealing with new PRs will also be much easier, since they all fit on a single page, reducing cognitive load drastically. And the danger of PRs dying of old age will hopefully also be much lower.
Overall, my hope is that development will be more efficient that way.
(* = What is the #1 reason, you ask? Simple: Most core devs have very limited time. I don't blame them, but that's a different problem anyway …)
What I mean with that is that there is only a tiny amount of people who are allowed to review Minetest submissions (pull requests on GitHub) and also give an official approval. As far I know, only core devs have official approval rights. On the other hand, PRs are pouring in at an alarming rate, the number of open (!) PRs hasn't dropped below 100 for a very long time. Also, a lot of PRs are very, very old, but still neither accepted nor rejected. >100 open PRs is a problem, because older (but high-quality) PRs get forgotten fast and it's hard to keep track of all of them in your mind.
Now add 1 and 1 together: We have a tiny handful of official reviewers (=core devs) while simultanously Minetest is bombarded with new PRs. Now it's pretty obvious why progress is so slow. The reviewers are just being overwhelmed.
Therefore, I propose one simple thing: Add more people with official approval rights. These people can give an approval to PRs that has the same weight as a core dev's approval.
How to decide who to make an official reviewer? I don't really know, but obviously these should be skilled people who know what they're doing and are aware of the Minetest development rules and they should also be not be afraid of rejecting bad PRs. Don't read this proposal as a sneaky way to sneak in every possible PR into Minetest, no matter how crappy. It's more about speeding up the process which decides whether a PR gets green light or red light.
Anyway, I think it shouldn't be too hard to find good reviewers. Our community is actually quite large, surely there are some good people. Right?
I imagine if the amount of official reviewers increases, Minetest will be able to significantly deal with a lot of PRs faster, and eventually get the number of open PRs below 100, below 50, below 25, below 10. Once the number of simultanously open PRs is low, dealing with new PRs will also be much easier, since they all fit on a single page, reducing cognitive load drastically. And the danger of PRs dying of old age will hopefully also be much lower.
Overall, my hope is that development will be more efficient that way.
(* = What is the #1 reason, you ask? Simple: Most core devs have very limited time. I don't blame them, but that's a different problem anyway …)
Re: We need more official PR reviewers
That is a very good suggestion Wuzzy!
+1
+1
- Hugues Ross
- Developer
- Posts: 72
- Joined: Mon May 06, 2019 22:52
- GitHub: Df458
- Location: Kitchens, Pantries, etc.
- Contact:
Re: We need more official PR reviewers
+1 from me as well.
Speaking personally, I would be interested in a position like this (although ultimately this is a decision for the coredevs, not myself). As someone with other hobbies and a day job, I could never offer the dedication and responsibility to the project that a coredev needs, but I could spare time for reviews in my areas of expertise if I felt like they had any tangible impact on the process.
I suspect there are plenty of others in the community who feel the same way.
Speaking personally, I would be interested in a position like this (although ultimately this is a decision for the coredevs, not myself). As someone with other hobbies and a day job, I could never offer the dedication and responsibility to the project that a coredev needs, but I could spare time for reviews in my areas of expertise if I felt like they had any tangible impact on the process.
I suspect there are plenty of others in the community who feel the same way.
- rubenwardy
- Moderator
- Posts: 6978
- Joined: Tue Jun 12, 2012 18:11
- GitHub: rubenwardy
- IRC: rubenwardy
- In-game: rubenwardy
- Location: Bristol, United Kingdom
- Contact:
Re: We need more official PR reviewers
Oh, boy, wish I'd thought of this /s
The reason why this doesn't happen much is because it is hard to find people who are trusted, have the skills, and are willing to invest sufficient time. V-rob and pyrollo were added to the team last year, but the number of PRs is still mostly unchanged.
Anyone can suggest anyone to become a core developer to celeron55. There is also the possibility of people being given review rights over certain areas, like a more specialised core dev. For example, we previously had someone who had the ability to approve and merge documentation PRs
The reason why this doesn't happen much is because it is hard to find people who are trusted, have the skills, and are willing to invest sufficient time. V-rob and pyrollo were added to the team last year, but the number of PRs is still mostly unchanged.
Anyone can suggest anyone to become a core developer to celeron55. There is also the possibility of people being given review rights over certain areas, like a more specialised core dev. For example, we previously had someone who had the ability to approve and merge documentation PRs
-
- Moderator
- Posts: 4095
- Joined: Wed Aug 24, 2011 09:44
- GitHub: sfan5
- IRC: sfan5
- Location: Germany
Re: We need more official PR reviewers
What I'm missing myself currently is another coredev who is reasonably active on IRC, which can discuss ideas with me, give a second opinion, etc.
What we're missing in general is to finally put this proposal into place. If we have the ability (or rather duty) to judge and reject/accept PRs based on a defined roadmap there will be less PRs left lying around in an unknown state.
What we're missing in general is to finally put this proposal into place. If we have the ability (or rather duty) to judge and reject/accept PRs based on a defined roadmap there will be less PRs left lying around in an unknown state.
- v-rob
- Developer
- Posts: 971
- Joined: Thu Mar 24, 2016 03:19
- GitHub: v-rob
- IRC: v-rob
- Location: Right behind you.
Re: We need more official PR reviewers
I know, and I feel kind of bad about this. I've said this before, but I work better if I have a set goal instead of a vague idea. I'd probably review more PRs if I had a list of ones I should review. In concrete terms, I review PRs that have been requested of me (for instance, with the "request review" button on GitHub, on IRC, or with a PM). So, if there's a PR I can/should review, I'd appreciate it if people simply asked.rubenwardy wrote: ↑Fri Apr 02, 2021 22:22V-rob and pyrollo were added to the team last year, but the number of PRs is still mostly unchanged.
Honestly, if you think that you'd like to be a reviewer without the responsibilities of a core dev, I'd suggest you find c55 on IRC and ask him if you could. If he says yes, then you're in. That's probably the best way to get this started: by actually getting someone doing it.Hugues Ross wrote: ↑Fri Apr 02, 2021 21:53Speaking personally, I would be interested in a position like this
- philipbenr
- Member
- Posts: 1897
- Joined: Fri Jun 14, 2013 01:56
- GitHub: philipbenr
- IRC: philipbenr
- In-game: robinspi
- Location: United States
Re: We need more official PR reviewers
I've never done any development for MT but I thought I'd throw my 2 dev-cents in the ring
---
In general, I like seeing the developer roadmaps in the forum sections, but if I'm being real, the best place is on GitHub and within the project itself. To put it bluntly, there are less children, and more people that care and are going to put the time in to understanding what exactly is going on. Not to say things are perfect (I've seen some of the GitHub issues and PR's) but it is a better platform.
Personally, I decided to be less invested in development (I just don't have the time) and be more invested in the community and philosophy of the project, as that is easier to segment into small chunks of time as needed. That's what the forums should be for imo. But for people that care about specific development, GitHub is the place to be.
Having someone that is fairly familiar with enough of the Minetest codebase will be a significant barrier to entry. This seems like a more reasonable solution, but you will still need people who possess a high enough understanding of the engine to make sure things don't diverge and become a mess (assuming that's not happening already...)rubenwardy wrote: ↑Fri Apr 02, 2021 22:22There is also the possibility of people being given review rights over certain areas, like a more specialised core dev. For example, we previously had someone who had the ability to approve and merge documentation PRs
Documentation and roadmaps are lovely :)sfan5 wrote: ↑Sat Apr 03, 2021 13:53What we're missing in general is to finally put this proposal into place. If we have the ability (or rather duty) to judge and reject/accept PRs based on a defined roadmap there will be less PRs left lying around in an unknown state.
Based on what you said above, I suggest you look into agile methodologies, specifically Kanban boards, if you haven't already. GitHub has built-in "project" support, which I've used to create and manage my projects using Kanban boards. It will help you organize better while letting other people see what you're working on (they can be public or private). If you invite other people to join your board as collaborators, they should be able easily add things to your backlog / todolist.v-rob wrote: ↑Sat Apr 03, 2021 19:56I know, and I feel kind of bad about this. I've said this before, but I work better if I have a set goal instead of a vague idea. I'd probably review more PRs if I had a list of ones I should review. In concrete terms, I review PRs that have been requested of me (for instance, with the "request review" button on GitHub, on IRC, or with a PM). So, if there's a PR I can/should review, I'd appreciate it if people simply asked.
---
In general, I like seeing the developer roadmaps in the forum sections, but if I'm being real, the best place is on GitHub and within the project itself. To put it bluntly, there are less children, and more people that care and are going to put the time in to understanding what exactly is going on. Not to say things are perfect (I've seen some of the GitHub issues and PR's) but it is a better platform.
Personally, I decided to be less invested in development (I just don't have the time) and be more invested in the community and philosophy of the project, as that is easier to segment into small chunks of time as needed. That's what the forums should be for imo. But for people that care about specific development, GitHub is the place to be.
- Wuzzy
- Member
- Posts: 4804
- Joined: Mon Sep 24, 2012 15:01
- GitHub: Wuzzy2
- IRC: Wuzzy
- In-game: Wuzzy
- Contact:
Re: We need more official PR reviewers
I have just looked at the the PRs that got closed (merged or not) in the recent months (from ca. February until now). I noticed something interesting:
The ONLY approvers in the last months were sfan5 and Krock/SmallJoker and nerzhul. Nerzhul approved only rarely, the bulk approvals came from sfan5 and Krock/SmallJoker. rubenwardy made a few justified rejections of bad PRs, but never approved anything.
Note: I'm not judging anyone here, I'm just analyzing the situation.
In other words, currently, the entire PR approval process mostly rests on the shoulders of only two people right now. If one of them goes missing, the PR pipeline comes to a screeching halt because you usually need 2 approvals to get anything merged.
I didn't realize we only have 2-3 active approvers right now. This explains so much now.
The ONLY approvers in the last months were sfan5 and Krock/SmallJoker and nerzhul. Nerzhul approved only rarely, the bulk approvals came from sfan5 and Krock/SmallJoker. rubenwardy made a few justified rejections of bad PRs, but never approved anything.
Note: I'm not judging anyone here, I'm just analyzing the situation.
In other words, currently, the entire PR approval process mostly rests on the shoulders of only two people right now. If one of them goes missing, the PR pipeline comes to a screeching halt because you usually need 2 approvals to get anything merged.
I didn't realize we only have 2-3 active approvers right now. This explains so much now.
Oh, this is actually a very good point. I feel like the roadmap should be given top priority. Everything else depends on it. This makes complete sense, I feel like many PRs are left open forever simply because nobody has the guts to close vaguely low-quality PRs because someone MIGHT like them. ;-)sfan5 wrote:What we're missing in general is to finally put this proposal into place. If we have the ability (or rather duty) to judge and reject/accept PRs based on a defined roadmap there will be less PRs left lying around in an unknown state.
- v-rob
- Developer
- Posts: 971
- Joined: Thu Mar 24, 2016 03:19
- GitHub: v-rob
- IRC: v-rob
- Location: Right behind you.
Re: We need more official PR reviewers
AFAIK, rubenwardy is taking something of a break.
Paramat was very good at this, even though it earned him a lot of flack. It's really too bad he disappeared without a trace; it would have been nice to know why he left. Maybe all the hate added up or something. According to the forums, he last visited one month ago, although I haven't seen him say anything at all for multiple months.
- Wuzzy
- Member
- Posts: 4804
- Joined: Mon Sep 24, 2012 15:01
- GitHub: Wuzzy2
- IRC: Wuzzy
- In-game: Wuzzy
- Contact:
Re: We need more official PR reviewers
I think that closing bad or off-topic PRs is also a very important of the review process. It's just that it is not clear at all when a PR is off-topic in the first place.
Having a clear roadmap will definitely make it easier (psychologically) to close bad or off-topic PRs with peace-of-mind.
Having a clear roadmap will definitely make it easier (psychologically) to close bad or off-topic PRs with peace-of-mind.
Re: We need more official PR reviewers
Better than having reviewers is having developers who develop, know and understand the code. Having one to rule them all is better still.
That the majority of development should happen as contributions by random people isn't a great idea. Such contributions tend to suffer tunnel vision, focusing only on one isolated aspect without much regard for the big picture. Also they tend to add complexity where it could be avoided. The code rots and grows unnecessarily, bugs come and go, the developers understand less and less of it, they get more reluctant to merge PRs because "it's risky" because they can't tell any better, some beneficial PRs get starved to death, some crap PRs get merged anyways. The code gets even worse, rinse, repeat.
MT revewers don't grow on trees, the ability to do meaningful reviews comes from experience developing MT and understanding the code. Even if there were such people on the team, their time would be better spent coding rather than reviewing other's PRs, which is a PITA and no one can be blamed for not being enthusiastic about it.
That the majority of development should happen as contributions by random people isn't a great idea. Such contributions tend to suffer tunnel vision, focusing only on one isolated aspect without much regard for the big picture. Also they tend to add complexity where it could be avoided. The code rots and grows unnecessarily, bugs come and go, the developers understand less and less of it, they get more reluctant to merge PRs because "it's risky" because they can't tell any better, some beneficial PRs get starved to death, some crap PRs get merged anyways. The code gets even worse, rinse, repeat.
MT revewers don't grow on trees, the ability to do meaningful reviews comes from experience developing MT and understanding the code. Even if there were such people on the team, their time would be better spent coding rather than reviewing other's PRs, which is a PITA and no one can be blamed for not being enthusiastic about it.
-
- Member
- Posts: 172
- Joined: Sat Mar 27, 2021 22:06
- In-game: DOOM_possum
Re: We need more official PR reviewers
well, the Game is Great, and suitable for anyone, the development test, has incredible new additions like "mossy cobblestone", but seems no more beyond That, like advanced World Generation, after a player has set foot into an Old Biome, or updates using a new TOOL, it also does not seem to have backup in the case of an emergency, like if a player decides to rip out a MOD and there is nothing in It's place, like "NPC" or "Villager"
- rubenwardy
- Moderator
- Posts: 6978
- Joined: Tue Jun 12, 2012 18:11
- GitHub: rubenwardy
- IRC: rubenwardy
- In-game: rubenwardy
- Location: Bristol, United Kingdom
- Contact:
Re: We need more official PR reviewers
The name for an official PR reviewer is "core developer". Having official PR reviewers that aren't core developers doesn't really make sense, unless they're limited to an area like documentation. In order to review code, you need to know how it works which is why having experience with the code is important.
The only differences between a core developer and a normal contributor is that core developers can approve/reject PRs and issues, and perform releases. Reviewing each others' and contributor's code is one of the main responsibilities, if not the. They tend to be the most active contributors because they were active contributors before, and because they can self-approve.
Minetest doesn't need more code, it needs higher quality code. There are many issues with architecture, correctness, and performance that need to be resolved. Adding reviewers that don't know what they're doing won't help with that
The solution to the bottleneck is to adopt a roadmap and to better prioritise time
The only differences between a core developer and a normal contributor is that core developers can approve/reject PRs and issues, and perform releases. Reviewing each others' and contributor's code is one of the main responsibilities, if not the. They tend to be the most active contributors because they were active contributors before, and because they can self-approve.
Minetest doesn't need more code, it needs higher quality code. There are many issues with architecture, correctness, and performance that need to be resolved. Adding reviewers that don't know what they're doing won't help with that
The solution to the bottleneck is to adopt a roadmap and to better prioritise time
Who is online
Users browsing this forum: No registered users and 23 guests