LORIS PR workflow improvement ideas

[maximemulder]

Motivation

There have been many pull requests made for LORIS in the past few weeks. Yet, few of them get reviewed and merged. As LORIS gets rid of its overrides, there will likely continue to be a lot of pull requests in the future, and we should look into how to improve our PR workflow.

This discussion tries to identify existing issues with the current PR workflow and propose solution to solve these issues. Some of the solutions are inspired by the Bevy open source project, whose I sometimes follow the developement.

Problems

Here are the problems I could identify with the current process, based on observations and my personal experience.

Most PRs are unassigned

When I open a PR, I often do not know who should review them. It seems I am not the only one, as most PRs are unassigned. As a result, there is usually little progress made on PRs until someone actively goes to review them or they are discussed at meetings (which takes time in said meetings !).

PRs are hard to track

It is very hard to track the state of PRs, and doing so usually requires to read the discussion. Which PRs need review ? Which need an external opinion ? Which need testing ? Which are still being worked on ? Which are ready for merge ?

Solutions

PR workflow documentation

Although there are some pages dedicated to opening a PR and reviewing one in the LORIS wiki. There is no page dedicated to the PR workflow. The changes adopted from this discussion should be documented in the wiki and made discoverable by linking them where it is appropriate (README ? contributing page of the wiki ? PR template ?).

Clear up the difference between reviewer and assignee

GitHub allows to assign "reviewers" and "assignees" to a PR. There is no general description of these roles as far as I know (especially the second one), and projects can instead choose their own definitions. It seems to me that LORIS uses assignees more than reviewers, but we should probably explicitely describe what are these roles in LORIS.

Streamline reviewer selection

LORIS developers are often busy with their projects, so it is likely unavoidable that LORIS PR reviews tend to be slow. Nevertheless, PRs need to be assigned first in order to be reviewed !

The process for assigning PRs should be clarified and documented. Ideally, the author chooses a reviewer if they know who they want to review their PR, if they do not know which reviewer to choose, there should be a process to assign an appropriate reviewer to the PR. This can take several forms:

• The author can advertise their PR on Slack, in the hope that someone will volunteer to review their PR.
• At the LORIS meetings, PRs that are still unassigned could be assigned (which is sometimes what we do when author advertize their PRs in these meetings).
• More extreme, reviewers could be assigned randomly, with the freedom to re-assign the PR if they think they are not fit to review them.

Document area experts

I think a reason why it is often hard to assign PRs as an author is that I do not know who is knowledgeable in which areas, and who should therefore be assigned to the PRs that are related to such areas.

@sagaar made a good point that testing unknown modules can help to learn them, and although I agree for simple PRs that are obvious to review and test, I personally find myself less confident reviewing non-trivial changes to modules I do not know, especially as I do not know which choices are controversial or not.

GitHub tags

GitHub tags can be used to categorize and filter PRs, which can be useful to track PRs state and implement an efficient PR workflow. Though tags are only effective if they are indeed used in practice, and the more tags there are the more work it takes to use them.

We currently have a few GitHub tags that we sometimes use for PRs, such as Priority: Projects, Testing, Needs work, Passed manual tests... However, their use is not systematic, the tags are not very discoverable, and some important tags are missing IMO. It should also be noted that many tags can be shared between issues and PRs.

A good technique to make tags more discoverable is to group them per category (with a prefix so that same category tags are together when sorted alphabetically). The following sections describe some categories that I think can be useful for the PR workflow. The tags do not need to be adopted exactly as described, but do bring ideas for what we might want. Some of these tags also already exist, but are described nonetheless.

Step tags

These tags describe what are the next steps in a PR cycle.

• S: Blocked (currently Blocked): The PR needs an external event (such as another PR) to move forward before advancing.
• S: Stale (currently Stale): The PR has not had recent interaction and is basically in pause or abandonned.
• S: Needs assignment: The PR needs someone to be assigned to review them.
• S: Needs review: The PR needs its assigned reviewer to review the PR.
• S: Needs testing: The PR needs to be tested by someone.
• S: Needs work (currently Needs Work): The PR needs more code work by the author.
• S: Needs design: The PR needs more design work before progressing.
• S: Needs discussion (currently Discussion Required): The PR needs a discussion on some subject (usually controversial) before progressing.
• S: Needs rebase (currently Needs rebase): The PR has diverged and needs to be rebased on main.
• S: Ready for final review: The PR has been reviewed and can be merged pending the final review by the merger.

Some of these tags are redundant by looking at which PRs have assignment, or accepted reviews. I do not have a strong opinion on whether or not these redundant tags should be added.

Difficulty tags

Difficulty tags describe the complexity of a PR, and how much work is needed to compete and review them.

• D: Trivial: Changes that are so trivial that they can be reviewed by a quick glance (like a typo or doc change).
• D: Simple: Simple changes that require reviewing and testing that should be easy and should not take up too much time.
• D: Medium: Changes that require non-trivial reviewing and testing that might take some time.
• D: Complex: Large or complex changes that require a thorough review and advanced testing to be approved.

The lines between each difficulty can obviously be a little blurry. Nevertheless, it is not important for those tags to be exactly right, but I think they can help small PRs to be merged fast and large PRs to receive the carefulness they need. They can also help to mark beginner-friendly PRs. I listed four difficulties here but three can be enough.

Area tags

We currently specify the module focused by a PR in its title. It could also be useful to specify which more general part of LORIS is focused by a PR, with tags such as A: Imaging, A: Instruments, A: CI, A: UI... I think it is rather hard to exhaustively list the useful area tags, and that they should be rather defined on a case by case basis or based on exerience.

Mapping of existing areas:

• A: API (currently API)
• A: UI (currently UI)
• A: CI
• A: Imaging
• A: Instruments

I see we currently have the (unused) PHP, Javascript and SQL tags, that describe which part of the code is updated. I think those are false good ideas. Most non-trivial PRs touch all of these. I think what is really important is the focus of a PR: A PR that changes a common JSX file should be tagged A: UI, but not one that just changes the UI in one module.

Type tags

Type tags describe the goal of an issue or PR.

• T: Bug (currently Bug): The PR is fixed an existing bug.
• T: Security (currently Security): The PR fixes an existing security vulnerability or improves security in general.
• T: Feature (currently Feature): The PR implements a new feature.
• T: Refactor: The PR changes the logic of the current code to make it cleaner or more extensible.
• T: Clean-up: The PR cleans up the current code without changing its logic.

Release tags

Release tags annotate PRs that should be considered when updating LORIS or writing release notes.

• R: Release notes (currently Release notes: The PR contains a change that should be added to the the release notes.
• R: Breaking change: The PR contains breaking changes that should be considered by existing projects.
• R: Database patch: The PR contains an SQL patch that should be ran by existing projects.

Project tags

Project tags indicate which projects an issue or PR is related to.

• P: CCNA: Features wanted by the CCNA project.
• P: IBIS: Features wanted by the IBIS project.
• P: HBCD: Features wanted by the HBCD project.
• P: CBIG: Features wanted by the CBIG project.

All of these currently fall under the umbrella of the Priority: projects tag. In addition to being able to track which issues and PRs there are for a given project, I feel like these tags could help new team members to identify faster which projects are using LORIS.

Priority tags

There are already some existing priority tags. I am under the impression that only the high priority PRs and issues need to be marked. Both priority and project tags use the P prefix though, not sure what to do, maybe merge them ?

Conclusion

I am always too lazy to write a conclusion at the end of these posts. I guess read, comment, and eventually discuss that at a next meeting.

[ridz1208]

What I like most about this post is that you bring up the proposal and the issue with the proposal you just brought in the next paragraph... So this is basically just me agreeing with you!

Here we go...

Preface: All of us here who have been around for more than 5 years know that we have been through several rounds of ups and downs in terms of procedures and success in reviews and testing. In fact not so long ago I was the one organizing and facilitating PR review and testing until I gave up... So kudos for taking that on, I will forward you the letter of resignation I wrote shortly after trying myself (joke joke, its a noble cause)

1. Most PRs are unassigned: Yes we need to up our game in terms of assignments and generally that is done by the roadmap team and the only reason for that is because individual developers can't easily determine how busy others are with their own PRs and their project work so we try to balance the load as much as possible but we never stood in the way of people self-assigning PRs. For this point, I am against authors assigning their own reviewers and assignees because in the past this has always ended up with 2 people being assigned everything, no balance. One of the reasons you see unassigned PR is generally because the entire group has already been assigned to a PR and its useless to assign all the PRs before people actually go through ther assignment. I say "one of the reasons" because of course it happens that we either miss some PRs or drop the ball on assignments in very busy weeks.

2. PRs are hard to track: This one I agree a little less with, at least from my perspective, GitHub offers sufficient filters to know the stages of a PR... we use those filters in fact when assigning PRs... Drafts are being worked on, "needs work" are being worked on, no review means it needs a reveiwer and no assignee means it needs to be assigned..

3. Document area experts: Although this one is a good idea, we have been down that path in the past and the result was those "experts" being quickly overwhelmed with the number of PRs assigned to them very quickly. At the roadmap meeting we know who the experts are and we decide if a PR is in need of an expert or can serve as an opportunity for someone to learn instead, so this goes back to item 1.

4. GitHub tags: I fully support that idea, I have tried in the past to group and organize these labels and they have changed over time and they can still change. I like what I see in your proposal but what I would like to see is a list of existing tags being mapped to a list of proposed tags because several of the ones you are proposing either already exist or they just need re-wording. Once that mapping is written out we can then discuss it and agree/disagree one item at a time.

   • Difficulty tags: I especially like the difficulty tags although as you mentioned those are sometimes hard to determines given the broad range of skill sets and expertises in the lab.
   • Step tags: Alot of those already exist... the ones that dont is most likely because the info can be inferred by other github fields like reviewers assignees...
   • Area tags: I support this although some do already exist, are severely misused and very rarely updated. They can go to overcrowd the tag list and provide little gain? I've had bad experiences with those in the past but I'm not opposed to them
   • Type tags: Sure! I think those are already there but yeah
   • Release tags: sure! but 90% of the time authors cant tell the difference, we used to have "Major", "Minor", "Bugfix" to determine how "breaking" a change was and these were massacred constantly. (see note below)
   • Project tags: I'm not opposed to although I would revise what you added in that section cause its pretty confusing, I thing you were tired when you got there haha.

NOTE: The most critical part of this proposal is not if we agree to it or not, it is actually adoption of the procedure. We have had several procedures like this one in the past and they were dropped because of lack of adoption rather than a flaw necessarily. We can have all the tags in the world but if developers don't set them consistently they are useless. Yes we can police people in short and medium terms but you will find that policing ends up being a full time job and its in nobody's job description! We can do more in terms of making the procedure simpler and document it more but authors would still need to read the documentation (which they don't btw).

Conclusion: Yes to a lot of the above!! and it takes new blood to boost these kinds of efforts and procedures and I guarantee you no one likes these proposals more than the roadmap team because they would indeed make our life easier but the devil is in the details so lets keep this conversation going

[maximemulder]

I know many of these tags already exist, although they do not seem to be very used currently. I think adding category to the tags is a change that make them a lot more discoverable. I have the feeling that it is a little tedious to use tags when they are unsorted as they are now. Regarding "trackiness" and unassigned PRs, it is true that after looking into it deeper, the process is more trackable than I thought.

For the project tags description I was not tired, I just wrote random stuff since the name of the tags were kinda self-explanatory haha, but I've changed it now. In addition to be able to filter the PRs there are for a single project, those may have helped me to better understand which projects use LORIS when I joined.

I'll update the message with a mapping with the current tags.

[skarya22]

Area tags missing:

• Tools
• Modules

[maximemulder]

I think area tags make the most sense when there are people specialized in this area, so that we can easily find the PRs they should pay attention to. Why not "tools", but I don't think "modules" is desirable, most LORIS PRs are already related to a module or another and I don't think it adds a lot of information.

[maximemulder]

@driusan @ridz1208 As said on Slack, I added/renamed some labels on the LORIS repo last week. Quick thoughts:

• I want to delete the label "Priority: Projects", as it is strictly less descriptive than using "Project: X" and optionally "Priority: High".
• I don't really like the "php" and "javascript" labels. Most PRs touch PHP, and the label "Area: UI" (previously just "UI") is very often redundant with "javascript".
• I do not like the label "sql". Is it a PR that has SQL code in it (= most PRs) or a PR that has an SQL patch. The description seems to suggest the latter (depending on what "sql script" means). I think having a "Release: SQL patch" label would be more descriptive.
• There is also a "python" label. And as much as I would like to see LORIS integrate some Python (notably to replace PHP in tools). The only Python code in LORIS see to be the for the Wiki, which I think does not justify a "Python" label (though a "Area: Wiki" label is another story !).

[driusan]

I agree with the first and third point but I'm not sure about the second. The labels can help people find PRs dealing with the the languages that they're comfortable with, but I don't feel strongly about it.

[maximemulder]

Hhhhm, the second point is the one I feel the less strongly about, so I guess I will let them for now then. @driusan I edited my message and there is a fourth point now.

[maximemulder]

Now that I think about it, I said I did not really like the language labels mainly because it is too much work for too little benefit. However, the labeling of PHP and Javascript PRs could be automated (not SQL in PHP). Not saying I will do it but just writing the idea here.