Re: [scikit-image] How to move pull requests forward (devs, please read)
Hey Vighnesh, I'm -1 on this level of optimism. =P I think code in master should reflect with probability 1 the best that we can make it. What that means is that "quick fix" PRs can get merged because the maintainer who merges it can issue a fix instantly. For large fixes, the most likely scenario is that it stays in the code for a long time, and then come release time there is a huge burden for the maintainers. Regarding new contributors pitching in, my take is that most new contributors are using the stable release, and so are unlikely to have even encountered the "dirty" function. Finally, for "single use" functions, it is straightforward enough for one to grab them off the contributor's fork, I think. Juan. On 14 Apr 2017, 2:30 PM +1000, Vighnesh Birodkar <vighneshbirodkar@gmail.com>, wrote:
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. 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
_______________________________________________ scikit-image mailing list scikit-image@python.org https://mail.python.org/mailman/listinfo/scikit-image
Hey Juan I understand where you are coming from. But don't you think there are instances when a contributor , after working on a new feature or function, does not have the time or energy to drive their work towards completion. The pending work might be too much for a single developer but it may be something that multiple developers working together might be able to complete. These features need not have to be part of the next release. But my assumption is that if one these features is useful, it will quickly get polished and be good enough to be exposable. If a feature works and is kept isolated from the public API, its tests/docs/optimizations can come from different other developers at different points of time. On 14 Apr 2017 2:12 am, "Juan Nunez-Iglesias" <jni.soma@gmail.com> wrote:
Hey Vighnesh,
I'm -1 on this level of optimism. =P I think code in master should reflect with probability 1 the best that we can make it. What that means is that "quick fix" PRs can get merged because the maintainer who merges it can issue a fix instantly. For large fixes, the most likely scenario is that it stays in the code for a long time, and then come release time there is a huge burden for the maintainers.
Regarding new contributors pitching in, my take is that most new contributors are using the stable release, and so are unlikely to have even encountered the "dirty" function.
Finally, for "single use" functions, it is straightforward enough for one to grab them off the contributor's fork, I think.
Juan.
On 14 Apr 2017, 2:30 PM +1000, Vighnesh Birodkar < vighneshbirodkar@gmail.com>, wrote:
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 (2)
-
Juan Nunez-Iglesias -
Vighnesh Birodkar