Re: [scikit-image] How to move pull requests forward (devs, please read)
Hi Stéfan, I think figuring this out is certainly a high priority when it comes to being contributor-friendly. One thing I don’t know what to do is figure out whether a contributor has turned on “collaboration”. What am I missing? I can see when *I’ve* turned it on, but for other pull requests I can’t get any info. Is the only way to know to try to push? A related point of failure that I’ve see is where we have an API discussion on the PR, and the core members have a back and forth over it, with confusion about the final outcome for the PR author. I’m still traumatised by this comment. And the current PRs on manual segmentation and nD rescaling risk the same fate. Stéfan and I briefly discussed this off-list and we came up with the following solution (which we now request comments on): - new functions of dubious API go to skimage.future. That’s what the package was created for. *However*, RAG has languished there for several releases, so this is not a long-term solution. So, in addition: - make a note in the TODO.txt for two releases down (would be v0.15 currently), forcing an API discussion and a move out of skimage.future for that release. - For existing functions that require an API change, merge a “good enough” API, raise an issue on GitHub for the API discussion, and tag it with the next release. For rebasing, if there is nothing left to do in the PR but the rebase, I suggest that we merge the PR manually in the command line. Incidentally, regarding optimistic merging: I do think there is a place for it in scikit-image, namely, for any PRs that pass tests and coverage checks, but need stylistic updates. Frankly, it is faster to do these oneself than to write them in the PR, *and* they inevitably feel petty to both the reviewer and the contributor. Therefore, I might suggest that we do perform optimistic merging when there are minor** style issues, and then fix those issues ourselves. However, if you merge in this situation, you *must* submit the fix immediately. Otherwise it’s certain to be lost. **: by minor, I mean not something like "class Filter: …” "class Gaussian(Filter): …” =P So, to summarise, my proposal when dealing with non-core contributors: - avoid massive API discussions. Settle on the simplest solution and add a to-do. If it’s new functionality, put it in skimage.future. - avoid all kinds of style nitpicking — it’s faster and easier to fix these things ourselves. I suggest even doing optimistic merging. - don’t ask users to rebase unless it’s clear that they are git experts. Follow github’s advice and merge on the command line. Comments are very welcome! Juan. On 7 Apr 2017, 10:32 AM +1000, Stefan van der Walt <stefanv@berkeley.edu>, wrote:
Hi everyone, and especially devs
This afternoon, Nelle Varoquaux gave a short presentation at BIDS on optimistic merging, a workflow suggested by the late Pieter Hintjens. In optimistic merging, all PRs are merged into a project, and fixes happen via additional patches or reverts. While, in my mind, this is not a viable model for scikit-image, it made me think of our PR review process.
A failure mode I observe often is the following:
- user submits patch - review happens, often either a) nitpicking on minor things or b) asking for technically difficult tasks to be performed (rebasing, can be tricky)
This introduces several problems, including discouraging new contributors, and wasting a lot of time (many PRs are left hanging for months, while waiting for a one-line fix from the original author).
I'd like to request the following:
- Ask contributors to open their branches for collaboration (this is on by default already, probably). - While reviewing a PR, pull down the branch, look at the diff, and fix any trivial errors you see. Push the results back up or, if collaboration is switched off, make a PR against theirs. - Keep review comments to bigger picture changes, and avoid getting bogged down in trivial fixes and stylistic changes.
Let's keep it collaborative and friendly, grow our team of contributors, and get going on 0.14 :)
Thanks! Stéfan
P.S. A question for the git experts:
I've been looking for ways of making the above process easier. I found that I can easily download pull requests:
- In my ~/.gitconfig, I add the following alias (replace 'upstream' with whatever you normally call the scikit-image main repository):
[alias] pullpr = "!f() { git fetch upstream pull/$1/head:pr$1 ;}; f"
I can now grab a PR with:
git pullpr 2447
and it will appear in the branch pr2447.
Unfortunately, I don't have a similarly easy way of pushing back, other than adding the contributor's remote, and pushing back to the PR branch. Any ideas?
_______________________________________________ scikit-image mailing list scikit-image@python.org https://mail.python.org/mailman/listinfo/scikit-image
Hello, I agree with most of the points put by Juan. Here are some points that I would like to put forward- 1) If there is a PR which has a merge conflict in a single file, we can tell the contributor to fix it through Github PR through the merge conflict button. 2) If the problem of merge is severe, core devs can come forward to fix the problem if the contributor is not a git expert. 3) For long standing PRs, first-time contributors could come forward and handle the problem if they know what needs to be done. Comments are very welcome! Regards, Sourav Sent from my Phone.Excuse any mistakes and brevity.
Hi Juan On Wed, Apr 12, 2017, at 23:39, Juan Nunez-Iglesias wrote:
One thing I don’t know what to do is figure out whether a contributor has turned on “collaboration”. What am I missing? I can see when *I’ve* turned it on, but for other pull requests I can’t get any info. Is the only way to know to try to push?
It's enabled by default (still, odd that they do not show it).
A related point of failure that I’ve see is where we have an API discussion on the PR, and the core members have a back and forth over it, with confusion about the final outcome for the PR author. I’m still traumatised by this comment[1]. And the current PRs on manual segmentation[2] and nD rescaling[3] risk the same fate. Stéfan and I briefly discussed this off-list and we came up with the following solution (which we now request comments on):
- new functions of dubious API go to skimage.future. That’s what the package was created for. *However*, RAG has languished there for several releases, so this is not a long-term solution. So, in addition: - make a note in the TODO.txt for two releases down (would be v0.15 currently), forcing an API discussion and a move out of skimage.future for that release. - For existing functions that require an API change, merge a “good enough” API, raise an issue on GitHub for the API discussion, and tag it with the next release.
The one concern I have is that we're going to run into the situation where each release is blocked by a bunch of hard-to-make API decisions. So perhaps raise an issue for API review of that function, tagged with next release? That way, we can start gathering feedback, and even close before the next release comes along.
For rebasing, if there is nothing left to do in the PR but the rebase, I suggest that we merge the PR manually in the command line.
Why don't we make the following simple rule: . Never request rebases---instruct contributor to merge, or do that for them 1. Always squash merges This is easy for the contributor, and does not pollute the master branch history. I know this goes against the original decision to retain all patches in a PR, but the problem of engaging contributors is much larger than my somewhat anal preference for well groomed patch-sets. If a squashed PR generates an overly large patch, it probably should be broken up anyway.
Incidentally, regarding optimistic merging: I do think there is a place for it in scikit-image, namely, for any PRs that pass tests and coverage checks, but need stylistic updates. Frankly, it is faster to do these oneself than to write them in the PR, *and* they inevitably feel petty to both the reviewer and the contributor. Therefore, I might suggest that we do perform optimistic merging when there are minor** style issues, and then fix those issues ourselves. However, if you merge in this situation, you *must* submit the fix immediately. Otherwise it’s certain to be lost.
Whenever you have a queuing system that grows more quickly than you can consume (e.g., the number of emails in your INBOX), you have to make an adjustment. And I think this is a very reasonable compromise. Stéfan Links: 1. https://github.com/scikit-image/scikit-image/pull/953#issuecomment-43598826 2. https://github.com/scikit-image/scikit-image/pull/2584 3. https://github.com/scikit-image/scikit-image/pull/1522
Hello I wonder if the scope of optimistic merging can be broader. Let's say we have a working implementation of an algorithm which is not the best for one or more of the following reasons. - Suboptimal data structure choice (linear instead of logarithmic complexity) which results in a minor speed delay during invocation. - Not having type checks or conversions. - Duplicate code which might be refactored out. - Lack of or no documentation But apart from this the PR does what the algorithm it implements correctly. Can we merge such a PR in master and keep it as a private unexposed function while raising an issue to address it's shortcomings ? The function can de decorated to exclude it from the main docs and maybe raise a warning when invoked. I believe the flaws I mentioned can be taken up by someone else in the community. Like a user might contribute documentation or a newcomer might do the refactoring. Then later down the line core Devs can agree and make the function public. It really sucks when there is a working patch for a feature you might want to use but you cannot use it because the PR isn't merged. For example, I don't care if a function takes more time to execute if all I want to do is use it to generate a few statistics or figures while writing a publication. Thanks Vighnesh On 13 Apr 2017 3:17 am, "Juan Nunez-Iglesias" <jni.soma@gmail.com> wrote:
Hi Stéfan,
I think figuring this out is certainly a high priority when it comes to being contributor-friendly.
One thing I don’t know what to do is figure out whether a contributor has turned on “collaboration”. What am I missing? I can see when *I’ve* turned it on, but for other pull requests I can’t get any info. Is the only way to know to try to push?
A related point of failure that I’ve see is where we have an API discussion on the PR, and the core members have a back and forth over it, with confusion about the final outcome for the PR author. I’m still traumatised by this comment <https://github.com/scikit-image/scikit-image/pull/953#issuecomment-43598826>. And the current PRs on manual segmentation <https://github.com/scikit-image/scikit-image/pull/2584> and nD rescaling <https://github.com/scikit-image/scikit-image/pull/1522> risk the same fate. Stéfan and I briefly discussed this off-list and we came up with the following solution (which we now request comments on):
- new functions of dubious API go to skimage.future. That’s what the package was created for. *However*, RAG has languished there for several releases, so this is not a long-term solution. So, in addition: - make a note in the TODO.txt for two releases down (would be v0.15 currently), forcing an API discussion and a move out of skimage.future for that release. - For existing functions that require an API change, merge a “good enough” API, raise an issue on GitHub for the API discussion, and tag it with the next release.
For rebasing, if there is nothing left to do in the PR but the rebase, I suggest that we merge the PR manually in the command line.
Incidentally, regarding optimistic merging: I do think there is a place for it in scikit-image, namely, for any PRs that pass tests and coverage checks, but need stylistic updates. Frankly, it is faster to do these oneself than to write them in the PR, *and* they inevitably feel petty to both the reviewer and the contributor. Therefore, I might suggest that we do perform optimistic merging when there are minor** style issues, and then fix those issues ourselves. However, if you merge in this situation, you *must* submit the fix immediately. Otherwise it’s certain to be lost.
**: by minor, I mean not something like "class Filter: …” "class Gaussian(Filter): …” =P
So, to summarise, my proposal when dealing with non-core contributors:
- avoid massive API discussions. Settle on the simplest solution and add a to-do. If it’s new functionality, put it in skimage.future. - avoid all kinds of style nitpicking — it’s faster and easier to fix these things ourselves. I suggest even doing optimistic merging. - don’t ask users to rebase unless it’s clear that they are git experts. Follow github’s advice and merge on the command line.
Comments are very welcome!
Juan.
On 7 Apr 2017, 10:32 AM +1000, Stefan van der Walt <stefanv@berkeley.edu>, wrote:
Hi everyone, and especially devs
This afternoon, Nelle Varoquaux gave a short presentation at BIDS on optimistic merging, a workflow suggested by the late Pieter Hintjens. In optimistic merging, all PRs are merged into a project, and fixes happen via additional patches or reverts. While, in my mind, this is not a viable model for scikit-image, it made me think of our PR review process.
A failure mode I observe often is the following:
- user submits patch - review happens, often either a) nitpicking on minor things or b) asking for technically difficult tasks to be performed (rebasing, can be tricky)
This introduces several problems, including discouraging new contributors, and wasting a lot of time (many PRs are left hanging for months, while waiting for a one-line fix from the original author).
I'd like to request the following:
- Ask contributors to open their branches for collaboration (this is on by default already, probably). - While reviewing a PR, pull down the branch, look at the diff, and fix any trivial errors you see. Push the results back up or, if collaboration is switched off, make a PR against theirs. - Keep review comments to bigger picture changes, and avoid getting bogged down in trivial fixes and stylistic changes.
Let's keep it collaborative and friendly, grow our team of contributors, and get going on 0.14 :)
Thanks! Stéfan
P.S. A question for the git experts:
I've been looking for ways of making the above process easier. I found that I can easily download pull requests:
- In my ~/.gitconfig, I add the following alias (replace 'upstream' with whatever you normally call the scikit-image main repository):
[alias] pullpr = "!f() { git fetch upstream pull/$1/head:pr$1 ;}; f"
I can now grab a PR with:
git pullpr 2447
and it will appear in the branch pr2447.
Unfortunately, I don't have a similarly easy way of pushing back, other than adding the contributor's remote, and pushing back to the PR branch. Any ideas?
_______________________________________________ scikit-image mailing list scikit-image@python.org https://mail.python.org/mailman/listinfo/scikit-image
_______________________________________________ scikit-image mailing list scikit-image@python.org https://mail.python.org/mailman/listinfo/scikit-image
participants (4)
-
Juan Nunez-Iglesias -
Sourav Singh -
Stefan van der Walt -
Vighnesh Birodkar