Pull Requests Review Delays

I wonder if the email list is better, so people can better look async? Things in Slack can quickly disappear - at least, I think Slack is much more ephemeral than email - and everyone checks their email, not everyone keeps up to date on Slack or forums.

2 Likes

Hey all, just an apology that I don’t have time to read and absorb the thread, but wanted to note a few things:

  1. For repos without a named maintainer that originated from 2U, Ned and I are working with @Michelle_Philbrick and @itsjeyd to facilitate reviews. Sometimes we do request a CC or community reviewer. It’s not a fast or efficient process and it’s temporary until named maintainers are identified.
  2. A few of us recently met and discussed code review workflows and came up with cross-project labels that will allow us to indicate a) that it’s time for code review, and b) that the maintainer needs to look at it. PRs that fall under (a) and not (b) would be candidates for community / CC review.

I think just having these labels will unblock progress on a few fronts. Happy to discuss with anyone on Slack. I’m not able to keep up with Discourse.

From our last Contributors Meetup there were two ideas that I’d like to write here so that they can be discussed further.

First is the something that could be considered a lightweight core contributor role. Currently we have core-contributors in the code contributor role and we have people in the community that have no core contributor status. We talked about adding a simple-contributor category.
This category could serve as a stepping stone for people in the community that want to contribute, but are not yet ready for the commitment of the core-contributors. Normally, the CC role would entail 20h/month of dedication. The new lightweight-contributor role could be removed from this or have a lower count. Probably some limitations would be needed to be placed in the role to mark the point where the person should move into a full core-commiter role instead of skipping the CC process forever. One such limitation could be the ammount of repos that can be granted access through the lightweight-contributor role. The process to grant lightweight-contributor status could as well be simpler and more inviting for people that may not know 5 more CCs in the community to get started.

Talking only from my experience at edunext and even though we support every person that wants to be a core contributor in the current form, some people find it difficult. They would benefit from having an easier to access position.

The second idea is that we acknowledge the differences about the contributing process in different parts of the ecosystem and write about it. It is very different to try and find a reviewer or expert for edx-platform than it is to find it for a MFE that is only used by 2U and is not supported by the BTR. The same if is is a well maintained (OEP55) repo vs one whose last line was the python3 migration made by someone who has long left the community. If we are clear about what the expectations might be for certain parts of our codebase, contributors, new and old, will have a better understanding of how things might work and the process of contributing different things.

2 Likes

In edx-platform, it can be hard to decide what team should review a pull request. Often it requires technical understanding of the changes. We have a CODEOWERS, but it’s incomplete, and GitHub points out that it has errors. I don’t know if CODEOWNERS could do the job even if it was fully fleshed out.

I don’t think it’s productive to make sweeping generalizations about how open source projects work. For example, Python has 1522 open pull requests, only 50 of which are less than two weeks old. Many have comments on them, but 791 have the label “awaiting review.”

We all want to reduce the time it takes to review pull requests. We want to enable and speed contribution. But we have to solve our problems, whatever they are. There isn’t just one way that open source projects work. Making unfair comparisons to unnamed projects doesn’t help us find solutions.

I don’t understand the logic of this. If pull requests need review by the maintainer, then they need review, no matter how old they are. If pull requests can instead be reviewed by core contributors, we should let those reviews happen no matter how new the pull request is.

This sounds roughly like, “review the PR, or we’re going to do it whether you like it or not.” You say, “even if it’s just to say no”, but that could turn into auto-closing pull requests once they are two weeks old. I don’t think anyone would like that.

I would like to understand more about which repos are experiencing the longest delays, and try to understand why that is.

2 Likes

To get from the review to the merge it can take more time, but in the contribution-friendly projects I have looked at, the first review comes in much faster than it does for us. Gitlab is a good example of this – and they manage this even with an open core strategy and a company focused on a specific instance, so with more structural obstacles than we have!

Also, to be specific about what I said here, the aim is to review PRs as they come in. While for us a goal of 2 weeks is currently perceived as something more or less unattainable in the current schema – it’s not so much the specific time that I want to compare, but the fact that we have room to improve our goals in the first place.

We could try! :+1:

Maybe if we helped prepare a bit the work so it’s easy to do - possibly with some categories or repos we would like to assign? @itsjeyd @Michelle_Philbrick are there some categories of PRs or areas of code that would benefit the most from having more core contributors with rights? Where are we getting OSPRs blocked the most?

+1, that could be a nice stepping stone – and also maybe something some of the core contributors would even want to switch to, if they find the core contributor role too heavy, but still want to contribute. We should just be careful to keep the role/responsibility is proportionate to the commitment and contributions, to still encourage organizations to invest in contributing more.

+1 – that also goes in the direction of further refining the list of who supports what, which @Michelle_Philbrick mentioned would help to improve the triage process.

@nedbat Interesting, thanks for mentioning it. Do you know if the file currently maintained? And how does that relate (if it does) with the process to identify maintainers in 0001 Use Backstage to Support Maintainers — Open edX Proposals 1.0 documentation ?

@nedbat The core contributors can already review any pull request anywhere - the issue is with the ability to give a binding :+1: that allows a PR to be merged, which is currently limited for each core contributor to a few repositories that that person has access to. It does makes sense to try to get the review by the people with the most intimate knowledge of a given repo first – the solution of escalating a review to all core contributors would only be a backup, for when those people are not available, and while we ramp up the number of maintainers. If we enlarge the group who are allowed to give a binding :+1: then, we increase the chances of the PR being reviewed in a timely manner (or at all).

It comes down to trust I think – a core contributor will know if they can reasonably safely give a :+1: on such a PR, or if they can’t. We choose core contributors for trustworthiness and care, as well as conformity to the project’s best practices, and all have plenty of experience with Open edX in general. So when maintainers are not available for reviews, imho we should give the option for other core contributors to unblock the PR, if they judge they can do so.

Now you’re right that the time delay might not be necessary in cases where the maintainer is unavailable and says so (or just doesn’t exist) - so the escalation could also be initiated immediately by the maintainer, it could be a nice way for maintainers to delegate their work when they are overloaded,. But in other cases it might be worth giving the time for the maintainer to see the PR and get a chance to react, before another core contributor can merge it?

That’s too bad that it sounds like that, because my goal here is more to find better ways to work together on this. Getting help from others to get work that we, as a community, need done is a good thing, and part of the charm of working on open source, no? We assign areas of responsibility on the code base based on skills and contribution track history, and because it’s an efficient way to divide responsibility - but ultimately, none of us “own” the code, it’s a community effort. When we don’t have time to do something, someone else can pick it up.

That would mean that the maintainers would trust so little the rest of the core contributors that they would prefer to block all contributions on a repo rather than allow another core contributor to handle reviews? That would be pretty disappointing! Imho this would be more something to reserve for cases where it’s either an obvious “no” (unwanted feature, horrendous code), or code/areas so tricky that it would be impossible for a core contributor to do a proper review (though, here again, the maintainer could also trust that core contributors will do the right thing - if a maintainer comments “this is tricky code, I would recommend against merging without a review from me”, I’m pretty certain that core contributors would heed the warning.

Again - the key ingredient is trust. And the more opportunities we have for interactions and collaborations like that, the more trust can develop.

+1 - it could help in multiple ways to know that. @itsjeyd would you have some time to devote on your core contributor time to help figuring this out?

For me, it would definitely be edx-platform. Right now there’s no good way to determine 1. Who should review, and 2. Can a CC review and merge on edx-platform (if we know a CC has expertise in a particular area).

@nedbat currently I run the OSPRs that need review on edx-platform by Ed, and he helps to guide me to someone to review/merge. Would it be helpful to post these in the Maintainer channel instead so you can weigh in too? I typically do them in batches every couple of weeks.

1 Like

In general I would say that:

  • The more PRs a repository gets, the more it would benefit from having a larger number of core contributors with merge rights.
  • Smaller repositories (with fewer PRs) that haven’t joined the maintainership program yet would benefit from getting some dedicated CCs with merge rights.
    • By “dedicated CCs” I mean people who have a lot of experience with the code on these smaller repositories, and time available to regularly review incoming PRs. (On paper, we already have a bunch of CCs with write access to these repos – for example, everyone with “Axim On-Call” access listed on the core contributors page would be able to review and merge PRs there. But it seems like these CCs are often spread thin between many different roles and responsibilities, and their review queues can get very long.)

However, based on what I’ve experienced so far, OSPRs mostly get stuck on repos that aren’t part of the maintainership program yet:

As mentioned previously, these repos still have owning teams, and @Michelle_Philbrick and I can look up who they are and ping them on PRs. But there is no guaranteed response time, and I’ve had to use the escalation path to @Kelly_Buchanan and @nedbat in a number of cases to ultimately get the engineering review process started. :bulb: At this point in the process, CCs aren’t involved at all yet, so having more of them wouldn’t help with this particular bottleneck.

So, along the lines of what @Michelle_Philbrick said above about edx-platform, it would be really helpful to figure out how we can unblock CC reviews early on in the OSPR process. Especially (but perhaps not exclusively) for unmaintained repositories, how can we get eyes :eyes: from someone in the owning team quickly, so they can say:

  1. This is good for CC-only review, or
  2. This is good for CC review, but someone from my team will need to give final :+1:, or
  3. This is not a good candidate for CC review; my team will need to review this.

Unmaintained repositories (including edx-platform, which @Michelle_Philbrick already mentioned) seem to be experiencing the longest delays.

I can put together some more detailed stats for the repositories that I’m assigned to in an upcoming sprint (most likely the one after next).

In the meantime it might be helpful to have a look at stats from Maintainer Pull Request Reporting. That board isn’t limited to OSPRs but it can be used to identify long-running PRs and corresponding repositories.

1 Like

That would be ideal yes – the issue is with cases where there are no people with a lot of experience on the repo and which also have time available, no? If we limit the reviewers to those criteria when nobody matches them, then nobody else can gain experience on these repos… Doing reviews (and contributing PRs, when they can be reviewed) are good ways to acquire experience with a repository – when nobody else is available to review, getting core contributors to gain that experience by reviewing would allow to break the vicious circle, and build review capacity.

My understanding is that there are many cases where it’s hard to even get an answer at all on this, no? A solution for this would be to default option (1), unless there is an answer asking for (2) and (3). It’s not quite to the level of trust I would like to see given to core contributors – which should also be capable to choose between the 3 options, by giving or withholding a binding :+1: or :-1: in any case. But it would be a step in the right direction already, end ensure we are able to unblock situations like this quickly even with unresponsive reviewers. It would also allow to start gathering stats about the proportion of PRs that fall in the different buckets.

Yep, I was describing an ideal scenario, and to achieve it we’d need to get around the problem that you mentioned.

Agreed, this could be a way to bootstrap increased CC coverage for individual repos.

That’s what I was trying to express, yes :slightly_smiling_face:

If I were to summarize the current situation I’d say that both the CC program and the maintainership pilot are great initiatives. There just seems to be a bit of a disconnect between the two. We could probably gain a lot by implementing a process that would bring the two groups closer and enable them to support each other better, as well as by increasing CC coverage.

Gathering some data about where the longest delays are currently happening will probably yield additional and/or more specific insights.

As mentioned here, @Michelle_Philbrick, @gabrieldamours and I are

planning to meet next week to talk about reporting needs for highlighting and better understanding delays and impediments in PR review workflows.

CC @e0d @nedbat @sarina @Kelly_Buchanan @Felipe

1 Like

There have been various discussions in different places related to this topic since the last post, so I’m giving a quick update here, for those who might not have seen them.

Maintainer program targeting edx-platform fully maintained

The maintainer pilot program is now entering phase 3, and a good news is that getting edx-platform fully maintained is being considered as one of its goals, as that it has been identified as one of the main blockers for PR reviews currently. Thanks for the input on this @Michelle_Philbrick & @itsjeyd !

Core contributors as backup reviewers

Blocker - automated deployments on edx.org

On the front of getting core contributors involved as a backup when maintainers aren’t available, one of the blocker discussed with @Kelly_Buchanan is the fact that currently, trusting someone to merge on the project, equals to giving deployment rights on edx.org – so it’s definitely much harder to trust someone with merge rights, given the implications.

The usual solution discussed for this is forking, but many are wary of the impact on the project contributions and the amount of drift that would end up remaining on the fork – understandably so imho.

Alternative to edx.org fork

It’s still an important blocker to resolve though, as it will hinder collaboration and trust within the project until we can solve it. There is another other option though would not require to fork, would be to keep the automated deployments of the project master as they currently are, but introduce an approval/review step of the deployments on edx.org, when the code to be deployed has been merged without anyone from 2U being involved:

  • If a PR merges on master that is from someone from 2U or has been approved by someone from 2U => automated deployment, like now
  • If a PR merges on master without anyone from 2U involved => automated deployment stopped, until someone double-checks the merge, and greenlights the deployment

It could be automatic for most PRs only to add an approval of code deployments by someone from 2U, . Ie block the deployments until someone checked that the code merged is safe to deploy on edx.org (or has worked fixes/reverts to allow the build queue to resume). This way you’re sure that no code that hasn’t been reviewed by 2U ever reaches edx.org, without any of the risk of an actual fork, and with a strong incentive to quickly double-check the community’s independent review work. At the same time, merges by other community members become less risky, because they don’t immediately go on production without anyone from 2U looking.

@jristau1984 @nedbat @Kelly_Buchanan What do you think? Would that help with trusting core contributors more easily with reviews?

1 Like

TBH, I don’t see how this reduces risk or increases capacity. It delays the review until post-merge when the deployment pipeline is now blocked. Will the review then be somehow quicker than a review pre-merge? I don’t see how it would. But now it would be under the pressure of getting something else out the door. The overall capacity to review changes going to production hasn’t changed. The reviews have simply been delayed. Maybe I’m missing some subtlety here.

This also requires that the deployment tooling be changed to recognize the origin of the changes in the pipeline to be deployed.

If we can do quick reviews of community review work, let’s do it pre-merge, and keep things simple.

It doesn’t delay the review until post-merge – to be clear, the review would be the one done by the core contributor who would :+1: the merge, and would replace the maintainer’s review when maintainers aren’t available. The merge is what decides if a specific piece of code is accepted in the code base and the project. What would change is the edx.org-specific part, where you would get a second chance to do a review before it gets deployed to edx.org, and monitor the deployment itself – to avoid getting code pushed to production without anyone from 2U being aware or there to handle issues/revert if needed.

Of course, it’s better if a maintainer reviews before the merge itself - and there would still be an opportunity to do that pre-merge. What you would gain is a second opportunity before deployment, even if the code review is passed on to someone who isn’t from 2U.

It might also be useful for areas of the code where 2U isn’t the maintainer or where core contributors merge without anyone from 2U being involved, already - the instructions at https://openedx.atlassian.net/wiki/spaces/COMM/pages/3334635570/Merge+Guidelines+for+Coding+Core+Contributors show that this is also creating constraints there, not just with the current discussion of review backups/escalation.

The 2U review might not be quicker than a review pre-merge, but it would allow the rest of the community to be unblocked in the meantime. When we have to wait for months for a review, it means having to keep rebasing the work against the other evolutions of the codebase, and it prevents other community members from benefiting from the contributed work. The time & energy cost of a delayed review is borne entirely by the contributor.

Having to deal with the backlog of unreviewed upstream changes when wanting to push new changes isn’t the ideal situation, but it would balance the burden in these cases, and realign everyone’s incentives around getting all reviews done within reasonable delays.

Happy to contribute time/work to help with that. :slight_smile:

@antoviaque @e0d @gabrieldamours Following yesterday’s conversation about PR reporting I added two date fields to the Contributions board:

  • Date opened
  • Ready for review since
    • Note that review is short for engineering review here.

For the repos that I’m responsible for these new fields have already been populated. (@Michelle_Philbrick I’m out of time for spending more time on this this week, but I could help update the fields for your repos starting next week.)

So you can now filter OSPRs based on how long they’ve been open/in the queue for engineering review. Examples:

For the record, I also renamed the Notes field to Last PM check to make the contents of that field easier to understand for people less familiar with the Contributions board.

It didn’t take a lot of time to populate the new fields retroactively. So if we wanted to be able to filter OSPRs by how long it took for them to get merged/closed, I think we could add another field for recording the merge date and populate that as well.

CC @nedbat

Syntax for date filters for reference.

2 Likes

Thanks, @itsjeyd!

Also to add, we’d like to collect more use cases for reporting in order to know where we should focus reporting creation/effort.

We’ve added a page here for people to add use cases to, so anyone here who would like to contribute, please do!

3 Likes

Thanks for getting the wiki page set up @Michelle_Philbrick :+1:

I added some more use cases to the list.

CC @e0d @antoviaque @gabrieldamours @nedbat

2 Likes

It has been a long thread with many different proposals to improve the project’s management of pull request reviews, so I thought it would be useful to list them summarized:

If anyone would like to help, mention it here, or on one of the tasks above.

I’ve summarized some of the points and potential steps in each of the task description - it’s rough so don’t hesitate to expand/split/add.

@antoviaque Thanks for summarizing all these points and creating GitHub issues for them :+1: :slightly_smiling_face:

I can give it a shot, yep. Just noting that it might take a few sprints to complete, as I’ll have to timebox the work to leave enough time for OSPR management itself.

CC @Michelle_Philbrick @e0d

1 Like

@itsjeyd @Michelle_Philbrick Thanks for the work on Faciliate monitoring and guiding of OSPRs · Issue #103 · openedx/wg-coordination · GitHub ! It’s great to be able to review the stuck PRs.

And kudos for the follow-ups on the PRs themselves, it looks like the pile has shrunk quite a bit since we last discussed, right? Do you think the reason why it could be reduced that much is the fact that we can now focus on the stuck PRs? Or was there also a better turn-around time generally and/or when you would ping people?

For the PRs currently stuck, it seem to be mostly e-commerce related PRs, correct? I see three of them: 1, 2, 3. Do we know the reason why they are stuck? And do we know of any core contributor with expertise on the e-commerce repo? It could be a good opportunity to suggest to the team responsible for the reviews, to delegate them to a CC, as an experiment?

Also, I have heard that @ndespujol has had troubles with PR delays on enterprise PRs recently - is this related?

1 Like

Hi @antoviaque!

Tim and I have been escalating things in the last few weeks, and the teams have been helping us chip away at stalled PRs, which is great!

  • The “e-commerce” PRs are in a bit of a limbo as 2U might take those off our hands completely. They weren’t a priority for 2U, and I think e-comm in general is a bit in flux for them at the moment. I’ve escalated these prior, and last week they were escalated again, but not sure when we’ll have a decision on what happens with them, though I will follow-up and keep you posted :grinning:

  • For the “enterprise” PRs for @ndespujol, I escalated some of these to 2U last week, but I don’t think they were related to the ones you may be referring to. @ndespujol - would you mind letting me know the specific PR(s) that need attention? I can look into it for you.

2 Likes

Thank you very much @antoviaque and @Michelle_Philbrick.

@Felipe from Edunext knows the exact PR, that are going slower than anticipated. I am going to tell him to speak with you @Michelle_Philbrick.

Best