Thanks Aaron for restarting the discussion. A vote or decision about voting should not be called in the middle of a discussion either. It is not reasonable that everybody can follow every discussion. A vote should be a separate thread with [VOTE] in the subject and concise description what is the vote about. I suggest to follow such practice as well for lazy votes. Steffen On Fri, Jul 13, 2018 at 7:03 AM Aaron Markham <[email protected]> wrote:
> @naveen - Hanlon's razor applies, so I am not concerned with any > individual's action or inaction here. Most people don't even realize its > happening (I didn't until recently), and the common practice doesn't manage > collaboration well at all. Let's fix that, and turn a less-than-ideal > common practice into a best practice. > > @eric - We talked about several things in this thread, and your -1 seems to > have stymied the dialogue. So I'm going to try to clarify your vote and > your proposal to get the discussion going again. > > Your vote was to never use rebase-merge on pull requests to master, > regardless of the number of contributors, even if the commit hygiene is > good. Your vote was to maintain the status quo with regard to pull > requests. You didn't comment on usage of the co-author field, so please > clarify if your vote includes not using this either, or if you agree that > the co-author option should be utilized. > > Your proposal was for contributors to request of a committer, the creation > of a branch when there are multiple collaborators on a feature, for the > collaborators to work on this branch, then, and only then, committers would > use rebase-merge when bringing this branch into master. Contributors who > are working alone, or don't mind their contributions squashed should > continue to create pull requests to master. > > I can assume that the collaborators guide would need to be updated and the > process for requesting branches be formalized and expeditious. I can also > imagine that we might fall into the same discussion about commit hygiene > here, and there would need to be formalized expectations in the > collaborators guide. > > Did I get that mostly right? I have more questions about your proposal, but > I'd like to hear what other people think first and their responses may > provide further clarifications. > > Cheers, > Aaron > > On Thu, Jul 12, 2018 at 4:46 PM, eric xie <[email protected]> wrote: > > > -1 > > > > We should stick with always using squash. It maintains PR reference in > > commit history. It is also common practice. > > > > If you want to included commits from multiple contributors in a single > PR, > > open a branch and make PRs to that branch. Then only use rebase when > > merging that branch to master. > > > > Thanks, > > Eric > > > > On 2018/07/12 23:38:54, Marco de Abreu <[email protected] > .INVALID> > > wrote: > > > Excellent :) > > > > > > I don't think we need a formal vote for this but rather let this be > lazy > > > consensus if nobody minds. > > > > > > Could we maybe revisit this decision in 1 or 2 months and then assess > the > > > state of commit history in all PRs (including squashed ones) and with > > focus > > > on rebase merged PRs? > > > > > > -Marco > > > > > > Naveen Swamy <[email protected]> schrieb am Fr., 13. Juli 2018, > 01:33: > > > > > > > You are right, its already enabled :) I saw that option greyed out > for > > one > > > > of the PR(for a different reason) and assumed we need INFRA to > enable. > > > > > > > > On Thu, Jul 12, 2018 at 4:22 PM, Marco de Abreu < > > > > [email protected]> wrote: > > > > > > > > > Coukd you elaborate why we would need a ticket for that? GitHub > > supports > > > > it > > > > > out of the box and it is enabled as far as I can tell. You just > have > > to > > > > > press the little arrow besides the merge button. > > > > > > > > > > -marco > > > > > > > > > > Naveen Swamy <[email protected]> schrieb am Fr., 13. Juli 2018, > > 00:54: > > > > > > > > > > > Yes to insist on commit hygiene when rebase-merge. We have to > open > > a > > > > JIRA > > > > > > with Apache Infra to enable rebase-merge on the repo. > > > > > > > > > > > > On Thu, Jul 12, 2018 at 3:21 PM, Marco de Abreu < > > > > > > [email protected]> wrote: > > > > > > > > > > > > > Fully agree, if we can get our commit hygiene up to industry > > > > standard, > > > > > I > > > > > > > don't see any problems in using rebase merge instead. For the > > short > > > > > term > > > > > > I > > > > > > > agree that it should be fine to rebase merge individual PRs > with > > > > > multiple > > > > > > > contributors. But in my opinion we should then insist on people > > > > > amending > > > > > > > their commit history if we deem it below our standard. A PR > > should > > > > not > > > > > be > > > > > > > rebase merged if the history is not proper - verifying that is > > the > > > > > > > responsibility of the merging committer, but ideally, we'd make > > > > people > > > > > > > aware of problems early on. What do you think? > > > > > > > > > > > > > > In general, I think we should gradually start taking this into > > > > account > > > > > > when > > > > > > > reviewing with the goal of all PRs having a proper commit > > history. > > > > This > > > > > > > would allow us in the long term to completely move away from > > > > squashing. > > > > > > > > > > > > > > -Marco > > > > > > > > > > > > > > Pedro Larroy <[email protected]> schrieb am Fr., > 13. > > Juli > > > > > > 2018, > > > > > > > 00:07: > > > > > > > > > > > > > > > This is a great discussion, thanks for raising the question > > Naveen. > > > > > > > > > > > > > > > > From my experience working in all kinds of software project > of > > > > > varying > > > > > > > > sizes and flavours: > > > > > > > > > > > > > > > > 1. People should be aware of git rebase interactive and > have > > > > more > > > > > > > > hygiene in their PRs. If a PR is hygienic and is separated > > in a > > > > > set > > > > > > of > > > > > > > > semantic commits, it's better not squashed as it helps > > finding > > > > > bugs > > > > > > / > > > > > > > > bisecting. A "dirty" PR with WIP commits, is better > > squashed, or > > > > > > > > requested > > > > > > > > to rebase interactively on CR. > > > > > > > > 2. Mixing different semantic changes on a single PR is an > > > > > > > anti-pattern, > > > > > > > > for example fixing whitespace or reformatting many lines > and > > > > > > changing > > > > > > > > some > > > > > > > > code which is hidden in a bunch of trivial changes and > could > > > > > cause a > > > > > > > > bug or > > > > > > > > major change of behaviour. > > > > > > > > 3. Agreed what with others have mentioned about small > > > > incremental > > > > > > > steps > > > > > > > > better than huge PRs. We also have JIRA or issues to > relate > > a > > > > set > > > > > of > > > > > > > PRs > > > > > > > > together. If not possible, PR with a set of non squashed > > commits > > > > > is > > > > > > > also > > > > > > > > good. > > > > > > > > > > > > > > > > Hope it helps. > > > > > > > > > > > > > > > > Pedro. > > > > > > > > > > > > > > > > On Thu, Jul 12, 2018 at 11:47 PM Naveen Swamy < > > [email protected]> > > > > > > > wrote: > > > > > > > > > > > > > > > > > @Aaron, I do not think most contributors(SDE or not) were > > even > > > > > aware > > > > > > > that > > > > > > > > > their commits are getting squashed into one and thereby > > others > > > > > losing > > > > > > > > > credit on that work. I would assume no bad intentions > there. > > > > > > > > > > > > > > > > > > @Hao, > > > > > > > > > Agree to breaking down into small and reasonable sized PRs, > > but I > > > > > > think > > > > > > > > > very small PRs will overwhelm the CI as it stands since it > > runs > > > > > > > > > everything(this is a separate topic and needs fixing). > > > > > > > > > > > > > > > > > > For cases similar to Aaron's he can raise the PR for the > doc > > > > > > > > > work(regardless of fork or not) and add other contributors > as > > > > > > > co-authors. > > > > > > > > > > > > > > > > > > @Marco, co-author might not be suitable always suitable and > > agree > > > > > > with > > > > > > > > Hao > > > > > > > > > that should be used on exceptions. co-author also makes it > > hard > > > > to > > > > > > see > > > > > > > > the > > > > > > > > > contributions individually. > > > > > > > > > > > > > > > > > > @Marco, why can we not have Rebase merge enabled on the > repo > > and > > > > > use > > > > > > > that > > > > > > > > > when there are multiple contributors. This discussion is > only > > > > about > > > > > > Not > > > > > > > > > Squashing when there are multiple contributors and agree to > > > > > continue > > > > > > > the > > > > > > > > > practice of Squashing in general. > > > > > > > > > > > > > > > > > > Until the tooling is fixed, I suggest to use the co-author > > > > feature > > > > > > when > > > > > > > > > collaborating on a fork. > > > > > > > > > > > > > > > > > > Also, I just want to reiterate and request contributors to > > have > > > > > > > > meaningful > > > > > > > > > and fewer commits on PRs. > > > > > > > > > > > > > > > > > > Thanks, Naveen > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Jul 12, 2018 at 11:40 AM, Marco de Abreu < > > > > > > > > > [email protected]> wrote: > > > > > > > > > > > > > > > > > > > As of now it will require the usage of a merge bot as far > > as I > > > > > > > > understand > > > > > > > > > > this issue: https://github.com/python/ > > miss-islington/issues/16 > > > > . > > > > > > > > Instead > > > > > > > > > of > > > > > > > > > > using the web interface do merge, we'd then trigger the > > bot to > > > > do > > > > > > the > > > > > > > > > merge > > > > > > > > > > on our behalf. There are pre-made solutions on the > > internet we > > > > > > might > > > > > > > be > > > > > > > > > > able to leverage, but it would take some time to get into > > it > > > > and > > > > > > > adapt > > > > > > > > > our > > > > > > > > > > process. > > > > > > > > > > > > > > > > > > > > Additionally, GitHub has this feature request tracked > > > > internally. > > > > > > > Let's > > > > > > > > > see > > > > > > > > > > when they get to add it. > > > > > > > > > > > > > > > > > > > > -Marco > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Aaron Markham <[email protected]> schrieb am > Do., > > 12. > > > > > Juli > > > > > > > > 2018, > > > > > > > > > > 21:33: > > > > > > > > > > > > > > > > > > > > > My point was about collaboration, or the lack thereof, > > and > > > > how > > > > > > the > > > > > > > > > > tooling > > > > > > > > > > > and apparent rewards from contribution statistics can > > > > reinforce > > > > > > > lone > > > > > > > > > > ranger > > > > > > > > > > > behavior. People can and should be proud of their work, > > but > > > > why > > > > > > > does > > > > > > > > it > > > > > > > > > > > have to be alone? Working within the context of a team > is > > > > > > something > > > > > > > > to > > > > > > > > > be > > > > > > > > > > > proud of too, but if your stats and standing are graded > > by > > > > how > > > > > > the > > > > > > > > > > commits > > > > > > > > > > > and merges land, and counting lines of code, then > > incentives > > > > of > > > > > > the > > > > > > > > > > system > > > > > > > > > > > are skewed. > > > > > > > > > > > How do we go about implementing the co-author process? > It > > > > > sounds > > > > > > > like > > > > > > > > > > > something worth doing! > > > > > > > > > > > > > > > > > > > > > > On Thu, Jul 12, 2018 at 11:15 AM, Hao Jin < > > > > [email protected] > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi Aaron, > > > > > > > > > > > > To be fair, this discussion has nothing to do with > > "pride" > > > > of > > > > > > > SDEs, > > > > > > > > > but > > > > > > > > > > > > rather a discussion on what is a better software > > > > engineering > > > > > > > > practice > > > > > > > > > > for > > > > > > > > > > > > the project. Breaking a large project into smaller > > tasks > > > > is a > > > > > > > good > > > > > > > > > > > software > > > > > > > > > > > > engineering practice. This article( > > https://arxiv.org/pdf/ > > > > > > > > > > 1702.01715.pdf) > > > > > > > > > > > > on > > > > > > > > > > > > Google's software engineering practice says: > > "Engineers are > > > > > > > > > encouraged > > > > > > > > > > to > > > > > > > > > > > > keep each individual change small​, with larger > changes > > > > > > > preferably > > > > > > > > > > broken > > > > > > > > > > > > into a series of smaller changes that a reviewer can > > easily > > > > > > > review > > > > > > > > in > > > > > > > > > > one > > > > > > > > > > > > go.". If you have concerns or your comments on this > > > > practice, > > > > > > we > > > > > > > > can > > > > > > > > > > take > > > > > > > > > > > > the discussion offline. On the other hand, an > important > > > > > spirit > > > > > > of > > > > > > > > > > Apache > > > > > > > > > > > > community is that "one must interact with others, and > > share > > > > > > > vision > > > > > > > > > and > > > > > > > > > > > > knowledge"( > https://community.apache.org/contributors/), > > if > > > > > you > > > > > > > > > > observed > > > > > > > > > > > > that a majority of the contributors have serious > > problems > > > > > with > > > > > > > > their > > > > > > > > > > > > writing maybe you can share some tips and hints on > how > > to > > > > > write > > > > > > > > > better > > > > > > > > > > > > documentations, in that way not only a lot of us > > within the > > > > > > > > community > > > > > > > > > > can > > > > > > > > > > > > have some growth but also you can save some time for > > > > writing > > > > > > more > > > > > > > > > good > > > > > > > > > > > > documentations and blogposts for MXNet, don't you > > think so? > > > > > Or, > > > > > > > if > > > > > > > > > you > > > > > > > > > > > only > > > > > > > > > > > > have to fix the doc for someone once in a while, I > > > > definitely > > > > > > > agree > > > > > > > > > > that > > > > > > > > > > > > you should be given the credit for that, and that's > > where > > > > the > > > > > > > > > co-author > > > > > > > > > > > > field can be useful, which was exactly what I was > > saying in > > > > > my > > > > > > > > > previous > > > > > > > > > > > > email. Thanks! > > > > > > > > > > > > Hao > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Jul 12, 2018 at 8:16 AM, Aaron Markham < > > > > > > > > > > > [email protected]> > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > This is a great discussion, and close to my heart, > > > > > because I > > > > > > > > want > > > > > > > > > to > > > > > > > > > > > > > include more writers and editors in our community, > > and > > > > I'm > > > > > > > > > struggling > > > > > > > > > > > to > > > > > > > > > > > > > see how to best manage this. It seems like being > the > > sole > > > > > > > > > contributor > > > > > > > > > > > on > > > > > > > > > > > > a > > > > > > > > > > > > > feature is like an engineer's Lone Ranger badge of > > > > pride! I > > > > > > > feel > > > > > > > > > like > > > > > > > > > > > it > > > > > > > > > > > > > should be a red flag. > > > > > > > > > > > > > > > > > > > > > > > > > > I work in collaboration with dozens of engineers to > > > > improve > > > > > > > their > > > > > > > > > > > > > documentation, explain their features, change flows > > to > > > > > > improve > > > > > > > > user > > > > > > > > > > > > > experience, and sometimes fix bugs and write code. > > When > > > > the > > > > > > > PR's > > > > > > > > > docs > > > > > > > > > > > has > > > > > > > > > > > > > great coverage, is clear, and not loaded with bad > > grammar > > > > > and > > > > > > > > > > spelling > > > > > > > > > > > > > mistakes, I will put comments in a review. But > > sometimes > > > > > > there > > > > > > > > > needs > > > > > > > > > > to > > > > > > > > > > > > be > > > > > > > > > > > > > a complete rework such that hundreds of review > > comments > > > > > isn't > > > > > > > > > > feasible, > > > > > > > > > > > > and > > > > > > > > > > > > > they can't be properly addressed. In a lot of these > > > > cases, > > > > > I > > > > > > > > commit > > > > > > > > > > my > > > > > > > > > > > > > changes to someone else's fork. Now I know the > > people I > > > > > work > > > > > > > with > > > > > > > > > on > > > > > > > > > > > > their > > > > > > > > > > > > > PRs appreciate my help, but when all of this work > > gets > > > > > > squashed > > > > > > > > > down > > > > > > > > > > to > > > > > > > > > > > > one > > > > > > > > > > > > > commit, I'm pretty much regularly getting squashed > > out. > > > > I'm > > > > > > not > > > > > > > > > sure > > > > > > > > > > > who > > > > > > > > > > > > > else is in this boat, but does the community > > recognize > > > > our > > > > > > > > > > > contributions > > > > > > > > > > > > > when this is the general mode of operation? > > > > > > > > > > > > > > > > > > > > > > > > > > Regarding co-author - I'm not sure how people would > > feel > > > > > > about > > > > > > > me > > > > > > > > > > > being a > > > > > > > > > > > > > co-author on their work. Documentation and clarity > in > > > > your > > > > > > work > > > > > > > > > > product > > > > > > > > > > > > is > > > > > > > > > > > > > important, but people's views on their personal > > *code* > > > > > > > > contribution > > > > > > > > > > > seems > > > > > > > > > > > > > to be more important than the overall code & > feature > > > > > quality > > > > > > > > itself > > > > > > > > > > > when > > > > > > > > > > > > > docs are part of the package. I feel that if I do > > > > follow-on > > > > > > PRs > > > > > > > > > > instead > > > > > > > > > > > > of > > > > > > > > > > > > > fixing things before they are merged, that we would > > be > > > > > > > releasing > > > > > > > > > > > > incorrect, > > > > > > > > > > > > > incomplete, and inferior product. But, in absence > of > > a > > > > > better > > > > > > > > > > solution, > > > > > > > > > > > > > maybe that's the pill we have to swallow. > > > > > > > > > > > > > > > > > > > > > > > > > > -1 on lots of small PRs (until we expand our range > of > > > > > > > committers > > > > > > > > > and > > > > > > > > > > > > reduce > > > > > > > > > > > > > the latency in reviews and merges). > > > > > > > > > > > > > +1 on improving the quality of commit messages, so > we > > > > don't > > > > > > > have > > > > > > > > to > > > > > > > > > > > > squash > > > > > > > > > > > > > & merge all of the time. > > > > > > > > > > > > > +1 on improving the practice of better commit & > merge > > > > > > > management > > > > > > > > so > > > > > > > > > > > that > > > > > > > > > > > > > the commit history is concise and meaningful. (I'm > > guilty > > > > > of > > > > > > > > this, > > > > > > > > > > and > > > > > > > > > > > > > certainly need to improve here.) > > > > > > > > > > > > > +1 on co-author - assuming that's what most > everyone > > > > thinks > > > > > > is > > > > > > > a > > > > > > > > > good > > > > > > > > > > > > > solution for now. I'm unclear on how this gets > > managed > > > > > > though. > > > > > > > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > > Aaron > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Jul 12, 2018 at 5:19 AM, Anton Chernov < > > > > > > > > > [email protected]> > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > Unfortunately there has been significant push > back > > for > > > > > > small > > > > > > > > > > > iterative > > > > > > > > > > > > > PR's > > > > > > > > > > > > > > and as a result they have grown substantially > > involving > > > > > > > > multiple > > > > > > > > > > > > > > contributors, multiple commits, sometimes > multiple > > > > > branches > > > > > > > to > > > > > > > > be > > > > > > > > > > > > worked > > > > > > > > > > > > > > on. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I fully agree and support all points that Jin > > raised: > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1) Most contributions should be broken down into > > > > smallest > > > > > > > > > possible > > > > > > > > > > > > PR's. > > > > > > > > > > > > > > 2) If a PR is small enough a single person can > > complete > > > > > it. > > > > > > > > > > > > > > 3) If a majority of PR is done by someone, and > > there's > > > > > some > > > > > > > > minor > > > > > > > > > > > issue > > > > > > > > > > > > > > he/she needs help with it could be done in a > > follow up > > > > PR > > > > > > by > > > > > > > > > > anybody, > > > > > > > > > > > > > > including the reviewer. > > > > > > > > > > [message truncated...] >
