RFC 10: PR Size Limitations

[Kelvinrr]

• Feature/Process Name: Enforcing PR Size Limits
• Start Date: 10-12-23
• RFC PR: added new changelog limitations #5366
• Author: Kelvin Rodriguez

Summary

We should consider adding language to the ISIS contributing docs on how to go about limiting PR sizes.

Motivation

We want to encourage people to contribute. With new contributions, large PRs can negatively impact reviewers and authors. The review time can increase exponentially when a PR has dozens of changes scattered widely over the code that are not necessarily directly related to each other. The high cognitive load required to review such a PR without creating more errors compounds the time it takes to complete a review compared to if the same changes came in parts.

Authors also lose out on having their PR addressed in a timely manner. Or rather, have their PR reviewed but have large windows between getting feedback and re-reviewing.

Proposed Solution / Explanation

• Add language to our contributing docs to limit PRs to one changelog entry per PR.
• If a PR requires more than one entry, it should be closed and split into multiple PRs.
• There should be consideration for the order. If feature A requires Bug fix B, B should be PRed and merged before feature A can be reviewed and merged.

Drawbacks

It will complicate the contribution process further and may cause friction with users with large feature branch merges.

Alternatives

We allow users to create PRs with multiple bug fixes and features in one contribution and accept that those PRs will take a long time to address.

Unresolved Questions

Should we consider a programmatic approach to enforcing this?

[jlaura]

I agree 100% with the proposed change.

I would consider including language indicating that a contributor should coordinate, if and when a large feature branch is needed.

I think a contributor with a single change log entry change that requires a large feature branch (adding some new, novel capability comes to mind) should be warned up front about that large feature branches are discouraged because time to merge is high, reviews are hard, and issues are likely to arise. The contributor and the core devs can then decide is many small contributions or one larger (still single CHANGELOG entry) enhancement is appropriate.

We might also consider noting that if and when reviewing these large feature branch changes is unfunded work, the contributor should also be aware that the side of the contribution will negatively impact the speed with which it might land. We could go as far as to say, if you are proposing to a funding agency to make a large change, please coordinate with us, to ensure core developers on this project are adequately funded to literately review, support, and ultimately merge the contribution (assuming all contributing guidelines are met). This way the core devs have time AND the original proposer can be confident that their large change has a high likelihood of landing.

[Kelvinrr]

@jlaura the coordination part seems like it would complicate things quite a bit. But I agree that, in the rare instance a PR must be large, we should have some way to coordinate. IDK how to make this a policy, though? At least without starting to create estimates on review times. Maybe explicitly say that large feature branches have no guarantee of being merged anytime soon even if it's one changelog entry? Which makes sense given we only have so much funding to review PRs in a year.

[jlaura]

Yeah, it is tricky. I think saying something like 'Large contributions, for example those in a feature branch, can sometimes be described in a single ChangeLog entry. They can also be large enough that the project can not make a guarantee of being able to review and merge these on a particular timeline.'

I think this is all about expectations. If someone spends a ton of time on a large PR, they are expecting a review of that, it's important to them (as it should be!). The problem is that folks with merge rights on the repo also have all of their own competing responsibilities. Anything we can do to warn potential contributors that reviewers have finite capacity and the larger the PR, the longer it is likely to take to find bandwidth to offer a review.

[Kelvinrr]

So, what I am seeing so far is, add to the contributing docs lines that say:

1. Limit PRs to one changelog entry per PR, if there are multiple changelogs (e.g., mix of bugs and feature adds), then it should be split up in PRed in some priority order. PRs that a reviewer determines has (or should have) multiple changelog entries will be rejected.
2. If PR is one changelog, but high-volume changes such as a large feature branch (many changes across many files requiring several hours of review time), then the PR isn't guaranteed to be reviewed in a timely manner. Authors should contact the repo owners, see code.json, to coordinate reviews outside normal USGS Astro support hours ahead of time.

I'll close this at the end of the week and PR the update in.

[Kelvinrr]

PR up #5366