mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marco de Abreu <marco.g.ab...@googlemail.com.INVALID>
Subject Re: Squash/Merge PRs
Date Thu, 12 Jul 2018 18:40:26 GMT
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 <aaron.s.markham@gmail.com> 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 <hjjn.amzn@gmail.com> 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 <
> aaron.s.markham@gmail.com>
> > 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 <mechernov@gmail.com>
> > > 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.
> > > >
> > > > Best regards,
> > > > Anton
> > > >
> > > > чт, 12 июл. 2018 г. в 10:11, Hao Jin <hjjn.amzn@gmail.com>:
> > > >
> > > > > +1 for Marco's proposal on using the co-author field. IMHO the
> usage
> > of
> > > > > co-author field should not be that often, consider:
> > > > > 1) If a PR is so big that it needs multiple people to contribute
a
> > > > > substantial part of it, why can't that PR be broken down into
> smaller
> > > PRs
> > > > > each submitted by single one of them?
> > > > > 2) If a PR is small enough (for example, < 300 lines), why can't
a
> > > single
> > > > > person complete it?
> > > > > 3) If a majority of PR is done by someone, and there's some minor
> > issue
> > > > > he/she needs help with(a small bug, incomplete doc), why can't that
> > be
> > > > done
> > > > > through code reviews?
> > > > > From the above 3 situations we can see that collaborations on
> > > individual
> > > > > PRs should not be quite often, but I do agree that it could still
> be
> > > > > necessary when someone lacks the related expertise/knowledge on
> some
> > > > > necessary part of that PR given that the required knowledge cannot
> be
> > > > > picked up in a short period of time.
> > > > >
> > > > > I do agree that keeping the commit histories of PRs clean is very
> > > > important
> > > > > as it could be confusing when reviewing PRs, but that really
> depends
> > on
> > > > > personal preferences (For my case, I usually do "git commit
> --amend"
> > on
> > > > > trivial changes and get a new commit for bigger changes such as a
> > > > > checkpoint of my whole PR). With growing the community and
> attracting
> > > > more
> > > > > contributors as a high priority for our project, I would prefer
> that
> > we
> > > > do
> > > > > not put even more burden on the contributors by asking them to
> manage
> > > and
> > > > > squash the commits themselves just for the not-so-often cases of
> > having
> > > > > multiple contributors on a single PR.
> > > > > Best regards,
> > > > > Hao
> > > > >
> > > > > On Thu, Jul 12, 2018 at 12:35 AM, Marco de Abreu <
> > > > > marco.g.abreu@googlemail.com.invalid> wrote:
> > > > >
> > > > > > Hi Naveen,
> > > > > >
> > > > > > I'm in favour of the squashing, considering the number of commits
> > in
> > > > some
> > > > > > PRs and especially because of some people making commit messages
> a
> > la
> > > > > "fix"
> > > > > > "fix" "fix" all the time. Additionally, it gets hard (not
> > impossible,
> > > > > just
> > > > > > more inconvenient) to determine the atomic states of master
-
> aka,
> > > > which
> > > > > > commits are separate from each other. You should consider that
> > > > > intermediary
> > > > > > commits are unstable (fail CI) and thus it could be very hard
to
> > > bisect
> > > > > > failures in future - and the commit history gets cluttered.
> > > > > >
> > > > > > As alternative, I'd like to suggest the co-author field for
these
> > > > cases.
> > > > > > Further documentation is available at
> > > > > > https://blog.github.com/2018-01-29-commit-together-with-co-
> > authors/.
> > > > > >
> > > > > > I definitely agree with the second part. We should all lead
by
> > > example
> > > > > and
> > > > > > maintain a high quality by keeping our commit messages clean
and
> > > > > > meaningful. When I receive an email notification that a new
> commit
> > > has
> > > > > been
> > > > > > added and it only contains "fix" as title, it's not that helpful
> > and
> > > > also
> > > > > > it's hard to track the development of a PR overtime. E.g., why
> has
> > > > > > something been changed? Was there maybe a bug that we didn't
> cover
> > > with
> > > > > > tests but the author just hacked something to get it to work
but
> > the
> > > > > > problem still lays somewhere? We won't know that way and it
makes
> > it
> > > > > harder
> > > > > > for us to review.
> > > > > >
> > > > > > Best regards,
> > > > > > Marco
> > > > > >
> > > > > >
> > > > > > Naveen Swamy <mnnaveen@gmail.com> schrieb am Do., 12.
Juli 2018,
> > > > 10:09:
> > > > > >
> > > > > > > Hi All,
> > > > > > >
> > > > > > > I am seeing that maintainers merge PRs into the repo, they
are
> > > > > squashing
> > > > > > > the commits in the PR, which I understand and agree is
to keep
> a
> > > sane
> > > > > > > commit history, however this is causing problem when there
are
> > > > multiple
> > > > > > > contributors involved on a PR(by contributing to a fork
of the
> > > repo)
> > > > > this
> > > > > > > effectively removes credit for multiple contributors involved
> and
> > > > shows
> > > > > > all
> > > > > > > code as authored by the contributor who created the PR.
> > > > > > >
> > > > > > > Can I request maintainers to not squash PRs if there are
> multiple
> > > > > > > contributors involved on the PR.
> > > > > > >
> > > > > > > Also on the same note, I request contributors(regardless
of
> > > multiple
> > > > > > > contributors or not) to keep a clean commit history by
> squashing
> > > the
> > > > > > > commits and not push all your WIP commits to the PR. this
will
> > help
> > > > us
> > > > > > keep
> > > > > > > our commit history clean and meaningful.
> > > > > > >
> > > > > > > Let me know your thoughts/better approach or If I have
> > > misunderstood
> > > > > how
> > > > > > > this works.
> > > > > > >
> > > > > > > Thanks, Naveen
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message