I submitted a Pull Request: who should/could review it?

[WilkAndy]

In the distant past, it seemed like @permcody often assigned people to review PRs. That doesn't often happen these days (at least for my PRs). I'm never sure who to ask, especially as occasionally my PRs are huge (like the 16k lines #15698). Often that means my PRs languish for a few days or weeks before they're reviewed. (That also means they keep growing in size because i'm on a deadline to implement something and can't wait for the review, which is a viscous cycle.)

Can we work out some sort of roster system (eg, @aeslaughter takes all PRs ending with "1", @loganharbour takes all PRs ending with "2", @andrsd takes all PRs ending with "3", @dschwen all those ending with "4", etc) or list of available people, or something?

a

[dschwen]

First step: Getting your PR to pass the tests ;-)

[aeslaughter]

I will second this, I generally won't look at something until the tests are passing.

[lindsayad]

I'm curious about a viscous cycle.

I think that @aeslaughter has been doing a lot of assigning in the last week. @loganharbour has also done some good assignment work in the past. I don't know if a numbering system would work well.

Who should/could review it? I'm of the general opinion that the PR author probably knows the best person for that job. You should always feel free to mention someone who you think is a good candidate for review. If you ping a MOOSE team member, that person will at a minimum make sure someone gets assigned to the job. I think @WilkAndy would also be a candidate to join the change-control-board which would give you the ability to assign.

I'm also hopeful that in the not too distant future we as a MOOSE team have a more established/documented practice for rapidly assigning PRs. I'm bad at that task myself.

[aeslaughter]

I think @WilkAndy would also be a candidate to join the change-control-board which would give you the ability to assign.

I agree. The new process for joining the CCB requires the candidate to review 5 PRs with supervision from a current CCB member. We just need to note these 5 items and when they are all complete you can be upgraded.

[YaqiWang]

Can I join CCB? How do we apply this?

[YaqiWang]

I'd say the assigned reviewer should be another person in your institute. Reviewing should be part of the code development. Reviewing can be volunteering but cannot expected to be. The trade-off of being a moose module has to be considered carefully.

[permcody]

Yeah, back when I was acting as the unofficial integrator I really tried to keep things moving. It wasn't cheap though. I estimate I spent a third to half of my time watching, reviewing, and assigning PRs, and helping CIVET stay running due to transient failures. We almost need a hierarchy like the Linux kernel has. Maybe I need to assign lieutenants and integrators. Even without the hierarchy, our project really could use a dedicated integrator resource. I feel like @loganharbour may be a good candidate for that role as he has the discipline and eye to stick with it. Maybe when he finishes up his PhD. I'm open to other suggestions though. For now as we continue to settle, feel free to tag devs for assistance.

[lindsayad]

Is it unreasonable to think that this could actually be a responsibility for someone like new Jason? @loganharbour does do a great job with these things, but he's also extremely proficient at writing framework and application code and I would love to see him be able to dedicate all his time to such tasks. I feel the same way about all our current MOOSE team members (and I felt the same way when @permcody was doing most of the reviewing). It's sad to lose framework developers to maintenance, if it can be potentially avoided.

[WilkAndy]

Whatever happens, and maybe no scheme will be formalised, we should strive to make reviewing as prestigious and respectable as writing code, if not more so. Already it is quite difficult to contribute to MOOSE modules, in my opinion, because we require tests and documentation (which is good and necessary, but burdensome), and the interfaces to MOOSE objects are becoming increasingly complex (again good, but burdensome), and so much has been written already (while writing a kernel for the diffusion equation is nice-and-easy in MOOSE, no one is doing that any more - i suspect many contributions are much more convoluted - mine certainly are). Reviewing helps shift this burden from people like me, because you help me with aspects of C++, and you explain MOOSE interfaces and tell me that such-and-such has already been done in other areas of MOOSE, and you encourage new developers by helping with git/QA/etc teething issues. Reviewing allows the MOOSE team to keep a birds-eye view on how the code is growing, allowing you to eliminate duplication, and to promote cross-fertilisation between different parts of the code, and it's good for the framework developers to understand what is needed by the modules. So, reviewing is an important part of overall architecture and design, and should be done mostly by the core MOOSE team, in my opinion.

Every day or week i get asked to a review a paper, and mostly the papers are rubbish and my heart sinks at the thought. Let's try to make MOOSE reviewing a more positive experience!

[YaqiWang]

I have to admit that changes in MOOSE can trigger changes in modules and applications, which application developers often do not want because they typically do not add new capabilities and require non-trivial efforts. This is the reason I pushed MOOSE to remove deprecation/legacy messages in applications caused by MOOSE changes. But that is not quite successful. If a large PR can be split into smaller PRs or well communicated with the reviewer in the design stage, the reviewing can be faster and not so costly. I think that this comes from a fact that high quality codes do not come free. MOOSE is trying to bring to a very high level SQ, which may not be needed by applications. One way is to move codes in modules as much as possible to your applications, then you can have less restrictive rules.