search

qgis / QGIS-Enhancement-Proposals

QEP's (QGIS Enhancement Proposals) are used in the process of creating and discussing new enhancements for QGIS
117 stars 37 forks source link

Implicit PR acceptance after 2 weeks with no review #303

Open nyalldawson opened 2 days ago

nyalldawson commented 2 days ago

QGIS Enhancement: Implicit PR acceptance after 2 weeks with no review

Date 2024/10/24

Author Nyall Dawson

Contact nyall dot dawson at gmail dot com

Summary

Currently, pull requests must be be reviewed by a QGIS developer with commit rights prior to the main repo prior to being merged into master. While this is an ideal approach for code stability, unfortunately the QGIS development community has a workload issue and it is extremely difficult to get a pull request reviewed. This can take many weeks, months or even years.

In this QEP I propose a change to policy to permit pull request merging by core developers after a two week period with no comments.

This QEP is part of a series of proposals designed to improve the QGIS development workflow and document existing policies.

!!!!!PLEASE NOTE THAT THIS QEP TARGETS PULL REQUESTS OPENED BY DEVELOPERS WHO HAVE CORE COMMIT RIGHTS ONLY. PULL REQUESTS WILL STILL REQUIRE AN EXPLICIT APPROVAL IF THEY ARE OPENED BY A DEVELOPER WHO HAS NOT BEEN GRANTED COMMIT RIGHTS!!!!!!

Proposed Solution

Currently, for an developer with commit rights to the QGIS repo to get a pull request merged, they must:

  1. Ensure that the PR passes all reasonable CI checks (some exceptions exist to this rule, eg when a check is broken for unrelated reasons (such as broken third party services, or a lint/code analysis/spell check test failing from other parts of a modified file).
  2. The PR must be approved by a different developer with core commit rights

(There's an additional, informal but general followed practice that the developer will address all opened concerns / suggestions / etc, even if approval has been given by someone else)

This proposal would revise the second guideline, so that it becomes:

  1. The PR can be merged immediately if it has been approved by a different developer with core commit rights.
  2. If NO comments are raised within a two week period following the PR being submitted, the original developer is free to self-approve and merge the PR. This implicit acceptance ONLY applies to PRs submitted by those with core commit rights, and ONLY to PRs with no comments raised. It is sufficient for another developer to comment "i require more time to review this", "I have thoughts but am on break and cannot provide feedback for another month", etc to invalidate the implicit acceptance for a pull request. Comments with no relation to code review, eg "this looks great!", "finally!", etc have no impact on the implicit acceptance.

Please note that the implicit acceptance is only valid for pull requests targeted to non-LTR branches. Changes being submitted to the LTR branches will still require explicit approval prior to merge.

Example(s)

https://github.com/qgis/QGIS/pull/58337 would be considered implicitly approved -- it was opened by a core developer (@wonder-sk ), has been opened for over two weeks, and the only comments are non-review related ones.

https://github.com/qgis/QGIS/pull/58528 is not considered implicitly approved -- review comments are present, even though they are over two weeks old

https://github.com/qgis/QGIS/pull/58699 is not considered implicitly approved -- while there have been no review comments within two weeks of the PR creation, it was not submitted by a developer with core commit rights

rouault commented 2 days ago

+1, with just the following remark:

2. The PR can be merged immediately if it has been approved by a different developer with core commit rights.

Can we amend this to recommend granting at least one day of review in the general case, except:

Otherwise, another core dev might have interesting remarks on the pull request, and depending on their time zone not have the chance to even notice it if has been merged in between.

nyalldawson commented 2 days ago

@rouault

I'm personally ok with both those changes, but I'm nervous about making this particular proposal include multiple independent tweaks to the policies. I think we risk the discussion becoming bogged down with everyone's individual wishlists and we won't get any actual final decisions made as a result.

Given that your changes are applicable (and would be beneficial) regardless of whether this proposal is accepted, could we submit them as a different QEP?

rouault commented 2 days ago

Given that your changes are applicable (and would be beneficial) regardless of whether this proposal is accepted, could we submit them as a different QEP?

yes, that make sense.

So +1 as it is stated

elpaso commented 2 days ago

+1, thank you for raising this topic.

Guts commented 2 days ago

Thank you @nyalldawson for bringing up this kind of "meta" topic, it's truly valuable.

I want to clarify from the start that I am not a core developer of QGIS, let alone a core committer. I'm sharing my opinion here as a developer and advocate of the QGIS ecosystem and, more broadly, FOSS4G.

I agree with the observation. I often have to follow up on PRs related to end-user needs/issues to prevent them from being forgotten. It's a real cat-and-mouse game with the "stale bot." I can imagine how frustrating it must be for developers submitting their own changes.

However, the proposed solution to the problem of workload and PR processing time doesn't fully convince me and deserves discussion.

I see that this could introduce some biases:

In conclusion, I'm not sure it's the solution to the problem. But I'm all for trying to address the problem rather than contemplating and feeding the contemptors. In any case, the key point is that this should help reduce favoritism among developers from the same company, and stop commits into master or automerges happening without discussion within less than three days (sometimes within the same 24-hour window... in a single time zone! sic). So, it's definitely a step in the right direction. Next steps: standardizing commits (commitzen)? introducing “beta” flags in features that are too fresh and therefore disabled by default in LTRs (even in PyQGIS)?

nyalldawson commented 2 days ago

@guts

Thanks for the feedback! Here's my thoughts....

It tends to prioritize PRs from core committers since they are directly involved. This creates a kind of privilege tied to their access rights.

This is 100% the case, and I don't see this as a bad thing. They've been granted this privilege because the project trusts them, and they've demonstrated an in depth knowledge of qgis development and have a proven track record.

There are times when two weeks is quite short, considering people's availability (vacations, work schedules, etc.). Maybe two weeks is too brief. Without wanting to bargain, why not extend it to three weeks?

Mmm... Three weeks is quite a long time. It'll get the stale label before this condition is triggered 🤣. Realistically, Is it really unlikely that a developer would go two weeks without checking through the list and making just a "wait!" comment on an interesting pr?

To introduce a counterbalance, perhaps there could be a mechanism where, if PRs are easier to merge, they should also be easier to revert?

What would this practically look like?

Similarly, should we limit the number of PRs eligible for automerge by a single core committer in the same QGIS version? Or based on the number of files modified?

Is there a good reason for that? I'm trying to cut down the amount of red tape here, not add extra overhead.

troopa81 commented 2 days ago

I'm -1 on this.

I'm completely aware of the review issue, and I have tried my best recently to increase my review effort, though it's probably still not enough.

It looks like to me that we, as a community, failed to solve a problem, so we make it legit. Still, I understand this is a big and long standing issue, but I would have liked that we try harder, and maybe take a look on how other project manages to deal with it (Is there any other known OS project that choose a way alike as the one proposed here?)

Review is important and has proved to increase code quality, so I think it should not be avoided in any case.

I have the feeling that we favor quantity over quality, and it endangers QGIS durability (there is already code parts difficult to maintain).

I don't have great counter proposal. I think each core committers should review as much as it ask for review. We have started to estimate Pull Request effort in order to split review budget accordingly, maybe we could use that metric to ascertain a balance?

rouault commented 2 days ago

and maybe take a look on how other project manages to deal with it (Is there any other known OS project that choose a way alike as the one proposed here?)

Not the testimony you probably expect, but my sympathy for this QEP mostly comes from the fact that GDAL is struggling even more than QGIS to get reviews, obviously due to its strongly imbalanced contributor code base. So GDAL doesn't even have a rule that a pull request must be approved for someone with merge rights to be able to merge it. And this is definitely far from being ideal. Even with a well balanced contributor code base, most people will tend to specialize in parts of the code base and will not feel competent to review contributions in parts they are not. At least, that's my own feeling when looking at QGIS PR queue, and I must fight hard against my imposter syndrome to sometimes give reviews on PRs of topics I don't master (if my stats aren't wrong, I'm familiar with something like 3 to 5% of QGIS code base). And I have a huge bias in doing that for PRs coming from core contributors (because the consequence of me missing something is less likely to have bad impacts than missing something in a contribution from an occasional contributor)

If you look at huge projects like the Linux kernel, they have this concept of subsystem maintainers who are the ones effectively responsible for approving contributions in the area they are responsible for, and doing the in-depth reviews. But those maintainers are a very scarce resource, and I don't know how the economics of that work. And Linux maintainer burnout is a recurring topic of discussion

ptitjano commented 2 days ago

Thanks for your proposal @nyalldawson. However, I don't agree with it. I'm not a core committer. However, I think that this policy change will have an impact beyond the core committer group. Therefore, It seems important to me to comment on it.

I think that this the wrong solution to a real problem. As stated in the QEP, there is a lack of review. Most of (if not all) the regular contributors to the project are fully aware of it. However, this proposal seems to be a last resort solution: "We tried a lot of solutions, nothing worked, let's do this". 2 reasons:

why aren't there enough reviews?

From earlier discussions, I understood that the lack of reviews is a long stand issue and some decisions have already been taken. For example, there is a paid review mechanism in place. Are there any other policies?

More generally, do we know why there are not enough reviews? Some open source projects have found solutions to this issue. But they work because they have chosen rules adapted to their situation. We could probably take inspirations from other projects but only if we understand the "why". Is the PSC involved in this discussion? Were they consulted? I would be curious to know their position.

There is no written review policy

To my knowledge, there is no review policy. I just reread the developers guide and it never mentions what a review is, who is allowed to make reviews, what we should expect from reviews, what should happen when the author and the reviewer don't agree on a change, etc.

This might seem strange to some other contributors or core committers, but as a somewhat new contributor (a little more that 2 years), I don't know if I'm allowed to make reviews. I tried to do it a few times, but it seems to me that most of my remarks were ignored or answered with a somewhat "I won't/can't do it because it would take too long". It gave me the impression that my opinion does not really matter because I'm not a core committer. Honestly, this did not encourage me to do reviews even if I had the right.

I think we should first clearly define what a review is we should help and encourage non core committers to do reviews.

Current code quality policy is not fully endorsed

As mentioned earlier, I'm afraid that this policy change will result in code quality decrease. According to the developers guide, there are mostly 2 rules as far as code quality is concerned:

If we look at the recent merged Pull Requests, some of them do not contain unit tests. Even, some PRs from core committers. It rarely happens, but it does happen. Is it a serious issue? Does it matter? I honestly don't know. However, this shows that we, as a community, are failing to fully respect our own code quality rules. And I'm afraid that this policy change will make the situation worse.

This might improve the situation in the short term, but it will create other issues in the long term

Of course, this policy change will improve the situation for the core committers. However, this won't improve the situation for regular/casual/new contributors. One could argue that this solves part of the problem. However, with this new policy, we will have the core committers who can merge things without a review and other contributors who will get even more frustrated.

As a regular contributor, non core committer, I also suffer from the lack of reviews. I am particularly frustrated when my 2 month old review does not get any review while a more recent Pull Request is quickly reviewed and merged. I'm afraid that this new policy will only increase this frustration. In the longer term, I'm also afraid that this new policy will discourage new or casual contributors. I tend to think that that we approach this problem the wrong way. We should first solve the lack of reviews for non core committers, and then for core committers.

At the very least, there should be safeguards

I agree that "[core committers] have been granted this privilege because the project trusts them, and they've demonstrated an in depth knowledge of QGIS development and have a proven track record." But this also means, that they have more obligations (please don't make me quote spider-man ;-) ). I think that, at least, this new policy should be changed by adding something like "In return, core committers agree to regularly carry out reviews".

In addition, some other mechanisms could be tested. For example, "ACK" or "TESTED BY":

For core committers, we could relax the "review" constraint and use a "ACK" or "TESTED BY" constraint instead if there is no review in the 2 weeks period.

I don't think this would be a problem in practice, but some of the core committers are not active contributors anymore. With this new policy, this would mean that a core committer who has not done any contribution to the project for a long time could have a PR merged in 2 weeks without a proper review.

Side note: where is written the current policy?

It seems to me like the current policy is supposed to be known by everyone. Is it written somewhere? Where can I find it? I could not find it.

To sum it up:

elpaso commented 2 days ago

If we look at the recent merged Pull Requests, some of them do not contain unit tests. Even, some PRs from core committers. It rarely happens, but it does happen. Is it a serious issue? Does it matter? I honestly don't know. However, this shows that we, as a community, are failing to fully respect our own code quality rules. And I'm afraid that this policy change will make the situation worse.

AFAIK there is no such a rule: tests are strictly required for core changes only.

I'm not saying that tests in gui or app aren't useful/welcome but sometimes they do more harm than good (look at the growing list of disabled tests in CI) or just won't worth the effort.

Also, when I'm in volunteer mode and I have a trivial fix for a bug I sometimes ask myself: is it better for QGIS users to make a PR for it without a test or is it better to leave the bug because I don't have the time to write a test?

troopa81 commented 2 days ago

Not the testimony you probably expect, but my sympathy for this QEP mostly comes from the fact that GDAL is struggling even more than QGIS to get reviews, obviously due to its strongly imbalanced contributor code base. So GDAL doesn't even have a rule that a pull request must be approved for someone with merge rights to be able to merge it. And this is definitely far from being ideal.

I think that if we go the way proposed here, we would land on a situation that could be pretty close of the one in GDAL no? I'm not sure it enforces new contributor to come and play (and review).

Even with a well balanced contributor code base, most people will tend to specialize in parts of the code base and will not feel competent to review contributions in parts they are not. At least, that's my own feeling when looking at QGIS PR queue, and I must fight hard against my imposter syndrome to sometimes give reviews on PRs of topics I don't master (if my stats aren't wrong, I'm familiar with something like 3 to 5% of QGIS code base). And I have a huge bias in doing that for PRs coming from core contributors (because the consequence of me missing something is less likely to have bad impacts than missing something in a contribution from an occasional contributor)

You're right, I have been there too. But I would rather try to improve the situation than accept there is nothing we can do about it. We should accept stupid questions from people that try to review out of their comfort zone. Why not also trying to improve our design documentation to lower the review and contribution barrier entry. If there are stupid questions, that's maybe the design/code is not clear enough.

ptitjano commented 2 days ago

If we look at the recent merged Pull Requests, some of them do not contain unit tests. Even, some PRs from core committers. It rarely happens, but it does happen. Is it a serious issue? Does it matter? I honestly don't know. However, this shows that we, as a community, are failing to fully respect our own code quality rules. And I'm afraid that this policy change will make the situation worse.

AFAIK there is no such a rule: tests are strictly required for core changes only.

I'm not saying that tests in gui or app aren't useful/welcome but sometimes they do more harm than good (look at the growing list of disabled tests in CI) or just won't worth the effort.

From the current developer guide: "As of November 2007 we require all new features going into master to be accompanied with a unit test. Initially we have limited this requirement to QGIS_core, and we will extend this requirement to other parts of the code base once people are familiar with the procedures for unit testing explained in the sections that follow."

My understanding is that 17 years later "people are familiar with the procedures for unit testing" and therefore this policy should apply to all the core changes.
I don't want to nitpick or derail the discussion. My point is that we don't fully embrace the current code quality policy and I'm afraid that this new policy will make the situation worse.

Also, when I'm in volunteer mode and I have a trivial fix for a bug I sometimes ask myself: is it better for QGIS users to make a PR for it without a test or is it better to leave the bug because I don't have the time to write a test?

Sorry for the misunderstanding. I was only referring to the new features PRs as written in the developer guide.

nyalldawson commented 1 day ago

@rouault

Given that your changes are applicable (and would be beneficial) regardless of whether this proposal is accepted, could we submit them as a different QEP?

yes, that make sense.

Submitted as https://github.com/qgis/QGIS-Enhancement-Proposals/issues/304