Re: Squash/Merge PRs

[eric xie]

-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  
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  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 <
> > marco.g.ab...@googlemail.com.invalid> 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  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 <
> > > > marco.g.ab...@googlemail.com.invalid> 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  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 
> > > > > 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 

Re: users@mxnet

[Eric Xie]

Neither TF nor Pytorch uses mailing lists though. In fact I can't think of any 
deep learning community that uses mailing lists. Mailing list is an obsolete 
legacy for old projects. No point in bringing it into new projects.

Thanks,
Eric

On 2018/06/18 18:42:12, Sebastian  wrote: 
> I am puzzled by the reluctance of this project to setup a user 
> mailinglist to be honest.
> 
> MXNet has major issues with attracting a community outside of Amazon 
> (whenever I hear folks talking about deep learning, they usually mention 
> tensorflow, pytorch and keras, but I rarely hear someone talk about 
> MXNet). At the same time, there is so much resistance to adopt practices 
> that are successfully used by many high-profile toplevel projects...
> 
> -s
> 
> On 18.06.2018 20:37, Timur Shenkao wrote:
> > Facebook is definitely a bad idea: we will be dependent on third party
> > provider + unclear who & how manages such group etc.
> > Forum + Confluence + Slack is  much better then.
> > 
> > On Mon, Jun 18, 2018 at 7:17 PM, Ivan Serdyuk 
> > wrote:
> > 
> >> Greetings Barber, Christopher. I had an idea to move out some discussions,
> >> covering Java and Scala API, to Facebook. So if somewhere exists a local
> >> JUG or Scala user group - they could reflect the topic of discussion. But
> >> background stuff could take place on mailing lists, Slack, forum, whatever.
> >> The reverse mechanism could be used to involve new committers, as well (so
> >> they would appear as presented newcomers, as for contributions).
> >>
> >> Regards
> >> Ivan
> >>
> >> On Mon, Jun 18, 2018 at 8:40 PM, Barber, Christopher <
> >> christopher.bar...@analog.com> wrote:
> >>
> >>> I don't understand why you would want a users mailing list when you
> >>> already have discussion forums. Users that want to be notified of new
> >> posts
> >>> on the forum can configure their notification preferences appropriately.
> >>> The traffic on the forums is already pretty low. I would think you would
> >>> not want to dilute that further.
> >>>
> >>> Christopher
> >>>
> >>> On 6/18/18, 1:27 PM, "Jim Jagielski"  wrote:
> >>>
> >>>  users@ mailing lists have great societal advantages that one
> >>> shouldn't ignore...
> >>>
> >>>  And it's not like this is the only project with "multiple"
> >>> communication choices for users. Most, if not all, projects have users@in
> >>> addition to such supplemental methods as IRC channels, a forum, etc...
> >> It's
> >>> about making it easy to have as many users as possible and as many
> >>> potential ways for users to communicate. It's not confusing; it's
> >>> empowering :)
> >>>
> >>>  > On Jun 18, 2018, at 1:19 PM, Tianqi Chen  >>>
> >>> wrote:
> >>>  >
> >>>  > The problem of having multiple separate channels of communication
> >> is
> >>> that
> >>>  > users get confused, and the cost of maintenance goes up(people have
> >>> to
> >>>  > watch both). As the current community was at discuss forum and many
> >>> users
> >>>  > prefer it, having a mail-list is only a burden we will bring
> >>>  >
> >>>  > Tianqi
> >>>  >
> >>>  > On Mon, Jun 18, 2018 at 9:48 AM, Jim Jagielski 
> >>> wrote:
> >>>  >
> >>>  >> IMO, that is the wrong way to look at it.
> >>>  >>
> >>>  >> A users@ mailing list is a great, easy, low-cost and low-overhead
> >>> way of
> >>>  >> *increasing* the user community and providing an extra level of
> >>> support.
> >>>  >> Unless there is "strong evidence" that this is NOT the case, I
> >> would
> >>>  >> recommend we create the list.
> >>>  >>
> >>>  >>> On Jun 16, 2018, at 12:28 AM, Tianqi Chen <
> >>> tqc...@cs.washington.edu>
> >>>  >> wrote:
> >>>  >>>
> >>>  >>> So unless there is a strong evidence that our community users
> >>> prefers the
> >>>  >>> mail-list, I would recommend we keep the current way
> >>>  >>>
> >>>  >>> Tianqi
> >>>  >>>
> >>>  >>> On Fri, Jun 15, 2018 at 9:25 PM, Sergio Fernández <
> >>> wik...@apache.org>
> >>>  >> wrote:
> >>>  >>>
> >>>   Are we targeting just Seattle as our community? I really hope we
> >>> are
> >>>   thinking a bit beyond that...
> >>>  
> >>>   On Fri, Jun 15, 2018, 21:22 Tianqi Chen <
> >> tqc...@cs.washington.edu
> 
> >>>  >> wrote:
> >>>  
> >>>  > I remember last time during the mxnet meetup in Seattle, we
> >> did a
> >>>  >> survey,
> >>>  > and most users preferred the current discuss forum. So I would
> >>> say we
> >>>   stick
> >>>  > with that given the user community prefers that
> >>>  >
> >>>  > Tianqi
> >>>  >
> >>>  > On Fri, Jun 15, 2018 at 9:13 PM, Sergio Fernández <
> >>> wik...@apache.org>
> >>>  > wrote:
> >>>  >
> >>>  >> Then, if everybody agree, let's request the mailing list
> >>> creation to
> >>>  > INFRA
> >>>  >> ;-)
> >>>  >>
> 

Re: Reverting pull request

[Eric Xie]

The way I see it:

1) you are just complaining and have never written code that fixes flaky tests
2) you are actively introducing bugs to the CI that causes it to fail in ways 
unrelated to any tests

Eric

On 2018/06/15 22:03:29, Marco de Abreu  
wrote: 
> CI doesn't fail for no reason but because some people prefer to push new
> features than to get our codebase actually stable. We currently have 51 [1]
> flaky tests and I have only seen a few people (thanks Sheng, Alex and
> Pedro) work on the problem. So instead of complaining, take part and help
> improving the situation.
> 
> The CCache/EFS failure lasted for 12 hours and was an error - these things
> happen when you run a service. This is not a blame-game.
> 
> -Marco
> 
> [1]:
> https://github.com/apache/incubator-mxnet/issues?q=is%3Aopen+is%3Aissue+label%3AFlaky
> 
> On Fri, Jun 15, 2018 at 2:55 PM Eric Xie  wrote:
> 
> > Hi Marco de Abreu,
> >
> > CI has been totally broken recently. It randomly fails for no good reason
> > more often than it passes. For example the ccache/efs failure has been
> > really annoying.
> >
> > Looks like there has been many changes to Jenkins and Docker lately. Do
> > you think we should revert all of the recent changes to get a stable CI or
> > do you think someone should find a fix for the bugs?
> >
> > Thanks,
> > Eric
> >
> > On 2018/06/15 21:21:50, Marco de Abreu 
> > 
> > wrote:
> > > We revert a PR because it should not have been merged in the first place.
> > > So far, I have been ignoring the fact that our committers are constantly
> > > breaking our own rules (which we expect contributors to follow). But
> > since
> > > this caused an impact twice (1.2 breaking change about model
> > import/export
> > > as well as this regression), I'm now being more strict and enforcing
> > them.
> > >
> > > I could've also made a script that prevents any PR from being
> > self-merged,
> > > but I thought our committers are responsible enough to follow our own
> > rules
> > > without systems actually enforcing them. I won't waste my time working on
> > > that script, but from now on I will revert every single PR (except
> > > emergency cases) that has been self-merged without approval.
> > >
> > > -Marco
> > >
> > > On Fri, Jun 15, 2018 at 2:15 PM Mu Li  wrote:
> > >
> > > > Why reverting instead of fixing the bugs? Static memory aims to reduce
> > > > memory allocation, it's a key feature to bridge the perf gap between
> > gluon
> > > > and symbol.
> > > >
> > > > On Fri, Jun 15, 2018 at 2:06 PM, Marco de Abreu <
> > > > marco.g.ab...@googlemail.com.invalid> wrote:
> > > >
> > > > > Hello,
> > > > >
> > > > > I'm reverting https://github.com/apache/incubator-mxnet/pull/10817
> > as of
> > > > > https://github.com/apache/incubator-mxnet/pull/11311 due to
> > regressions
> > > > > described in https://github.com/apache/incubator-mxnet/issues/11171
> > and
> > > > > https://github.com/apache/incubator-mxnet/pull/10817.
> > > > >
> > > > > The pull request has been self-merged without proper review and
> > > > introduced
> > > > > regressions. Committers should act as role models in this project and
> > > > > adhere to software engineer best practices.
> > > > >
> > > > > Best regards,
> > > > > Marco
> > > > >
> > > >
> > >
> >
> 

Re: Reverting pull request

[Eric Xie]

Hi Marco de Abreu,

CI has been totally broken recently. It randomly fails for no good reason more 
often than it passes. For example the ccache/efs failure has been really 
annoying.

Looks like there has been many changes to Jenkins and Docker lately. Do you 
think we should revert all of the recent changes to get a stable CI or do you 
think someone should find a fix for the bugs?

Thanks,
Eric

On 2018/06/15 21:21:50, Marco de Abreu  
wrote: 
> We revert a PR because it should not have been merged in the first place.
> So far, I have been ignoring the fact that our committers are constantly
> breaking our own rules (which we expect contributors to follow). But since
> this caused an impact twice (1.2 breaking change about model import/export
> as well as this regression), I'm now being more strict and enforcing them.
> 
> I could've also made a script that prevents any PR from being self-merged,
> but I thought our committers are responsible enough to follow our own rules
> without systems actually enforcing them. I won't waste my time working on
> that script, but from now on I will revert every single PR (except
> emergency cases) that has been self-merged without approval.
> 
> -Marco
> 
> On Fri, Jun 15, 2018 at 2:15 PM Mu Li  wrote:
> 
> > Why reverting instead of fixing the bugs? Static memory aims to reduce
> > memory allocation, it's a key feature to bridge the perf gap between gluon
> > and symbol.
> >
> > On Fri, Jun 15, 2018 at 2:06 PM, Marco de Abreu <
> > marco.g.ab...@googlemail.com.invalid> wrote:
> >
> > > Hello,
> > >
> > > I'm reverting https://github.com/apache/incubator-mxnet/pull/10817 as of
> > > https://github.com/apache/incubator-mxnet/pull/11311 due to regressions
> > > described in https://github.com/apache/incubator-mxnet/issues/11171 and
> > > https://github.com/apache/incubator-mxnet/pull/10817.
> > >
> > > The pull request has been self-merged without proper review and
> > introduced
> > > regressions. Committers should act as role models in this project and
> > > adhere to software engineer best practices.
> > >
> > > Best regards,
> > > Marco
> > >
> >
> 

Re: Vote to stop using JIRA

[Eric Xie]

for a single task, I think
> > > everyone who
> > > has used JIRA so far would share part of my experience.
> > > Thanks,
> > > Hao
> > >
> > > Appendix: what I need to do when I'm working on a task with JIRA:
> > > 1. Before I start working on something:
> > > 1) create a JIRA ticket for that, choose the correct type and
> > > define a
> > > proper name for it
> > > 2) go to backlog page to add it to a sprint if you want to use
> > the
> > > sprint management.
> > > 3) go to sprint management page to assign story point value if
> > you
> > > need
> > > the functionality (we recently started doing that)
> > > 2. When I start working on the task:
> > > 1) dig my ticket up (on the sprint page or backlog page or search
> > > for
> > > it)
> > > 2) click "open and start progress" to move it to "IN PROGRESS"
> > > phase.
> > > 3. When I finish the code, for every new PR I'll have to:
> > > 1) dig my ticket up
> > > 2) copy the ticket number so that I can add it to the PR title
> > > 3) an extra click to move the ticket to REVIEW phase
> > > 4) create a PR on github, paste the "MXNET-xxx" I just copied
> > > inside an
> > > extra pair of square brackets "[]"
> > > 5) still need to refer the related github issue in the PR if I'm
> > > solving one of them
> > > 4. For every code review or comment I receive on the new PR:
> > > 1) the JIRA bot logs 10 mins on the ticket no matter what kind of
> > > comment it is
> > > 2) JIRA sends me an email for every single one of such logs (so
> > if
> > > you
> > > receive 10 code review comments in a single code review you get 10
> > > emails,
> > > my highest record was 17, while github would bundle all of them in
> > > only 1
> > > mail)
> > > 5. After the PR is merged I get an email from JIRA saying this is
> > > merged
> > > (for the bot, merge is like another comment on the PR) but I still
> > > have to:
> > > 1) dig my ticket up
> > > 2) manually move it to DONE phase (bot does not do that
> > > automatically).
> > > 6. The task is considered finished at this point, but any new
> > comments
> > > on
> > > the PR will trigger the bot once again to log 10 mins on your ticket
> > > together with another email coming to your mailbox, while github is
> > > already
> > > sending an email for that.
> > >
> > > On Fri, Jun 8, 2018 at 2:23 PM, Naveen Swamy 
> > > wrote:
> > >
> > > > Eric/Mu/Tianqi,
> > > >
> > > > A couple of contributors  have used JIRA for the Scala project --
> > > this is
> > > > the first time, so there is some learning for us, We started off
> > > with a
> > > > design proposal, followed by a call for contribution. We kept our
> > > progress
> > > > open on the dev list and we found one contributor to help us during
> > > > debugging/testing/maven package creation and also one of them is
> > > working on
> > > > the Memory allocation issue in Scala not as a part of their day
> > job;
> > > This
> > > > is where I find value in managing the project features on JIRA,
> > > designs on
> > > > the public wiki, etc.,. I am not claiming that this is perfect,
> > this
> > > is a
> > > > WIP and as a community we should give it a chance and try it out.
> > > >
> > > > I don't think most of us here have even looked at JIRA.
> > > >
> > > > P.S: I am traveling, my response will be delayed.
> > > >
> > > > -Naveen
> > > >
> > > >
> > > > On Fri, Jun 8, 2018 at 12:34 PM, Anirudh 
> > > wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > I am not a big fan of JIRA either. But I would like to raise this
> > > issue
> > > > of
> > > > > committing to a decision after a VOTE has passed. I saw in PRs
> > > that there
> > > > > was a disregard for JIRA and ignoring

Vote to stop using JIRA

[Eric Xie]

Hi,

Since all of MXNet's development happens on Github, I think it's sufficient to 
use Github Issues and Github Projects for tracking. There are also many other 
plugins you can add to Github if issues and projects are not enough.

It's very easy to cross reference PRs and issues for tracking. In comparison, 
JIRA is an outdated system with very little features and no integration with 
Github. I think using it achieves nothing but additional overhead.

Thanks,
Eric

Re: MXNet C++ package improvements

[Eric Xie]

I see several issues with the design. I've commented in the document but for 
record here:

1. cpp-package is almost only used for inference. since you are planning a 
rewrite that's almost certainly non-backward-compatible, we might as well 
create a new interface that's inference only.

2. The hour-glass CAPI design should be kept.

On 2018/03/14 18:07:49, Anton Chernov  wrote: 
> Dear MxNet Community,
> 
> please find here
>  
> the
> design document for the proposed MXNet C++ package improvements for review
> and consideration.
> 
> Feedback is welcome and highly appreciated. Thank you!
> 
> BR
> Anton
> 

Re: Request for comments - Keras-MXNet as submodule in MXNet

[Eric Xie]

-1

If you make MXNet a submodule of keras, then you should PR that to keras.
If you want something like mxnet.keras, then you should do a full rewrite that 
only keeps the keras interface.

On 2018/03/23 05:49:07, sandeep krishnamurthy  
wrote: 
> Hello MXNet Community,
> 
> Along with Lai, Karan and other MXNet contributors, I am working on adding
> MXNet backend for Keras. Currently supporting around ~70% of Keras APIs
> across CNNs and RNNs.
> https://github.com/deep-learning-tools/keras/tree/keras2_mxnet_backend
> 
> We wanted to gather the community feedback on the proposal for including
> this keras-mxnet package as a submodule in Apache MXNet. This will enable
> providing the Keras interface for MXNet users. MXNet users can choose Keras
> interface for building their Neural Networks in Symbolic Mode (Ex:
> mx.keras).
> 
> *Advantages:*
> 
> 1. Keras is widely popular interface that many DL practitioners are
> familiar. By including keras interface within MXNet natively, we enable
> many users to use MXNet with 0 learning curve.
> 
> 2.  Adding as submodule and exposing natively within MXNet pip package,
> would greatly enhance user experience and get more users as compared to
> releasing a fork repository independently.
> 
> 3. Why submodule? - Helps in easily managing with patching the latest
> parent keras-team/keras developments and releases. Thereby helping us
> provide users the core keras experience. Operational management.
> 
> 4. Other minor advantages - Operational maintenance, pip, CI and quality
> control.
> 
> Please do share your comments on the proposal.
> 
> Best,
> Sandeep
> 
> *Note: *We tried merging with keras-team/keras and we created a PR
>  as well. However, due to
> multiple design incompatibility challenges, we need significant re-work on
> MXNet Module, KVStore, Optimizers to address keras-team design concerns.
> Since, we are adhering to keras API interface exposed to users, we are
> planning release on the forked repo for now. More details on the design
> challenges and workaround tried -
> https://docs.google.com/document/d/1Vn5ip5MzCKcN29KCCnwjB2d59y-VevdLrdn_eNd3nE4/edit?usp=sharing
> 

Re: move entirely to CMakefiles for building MXNet, drop Makefiles

[Eric Xie]

We want as few dependencies as possible.
CMake alone is enough trouble for our users. We don't want to burden them with 
other stuff.

On 2018/03/06 17:21:15, kellen sunderland  wrote: 
> Short term solution sounds good to me Chris.  Converting the CI should be
> pretty easy.  One thing we should keep in mind is that there's going to be
> a bunch of doc's we'll have to update.
> 
> Warning, slight thread hijack ahead:
> As a more long term change I was wondering if we had considered using
> hunter for third party packages?  It seems like a good system, and while it
> likely won't have support for all our projects, we can contribute back
> support for the ones we care about.
> 
> For me the primary benefit would be that it would conditionally fetch
> source at build time based on your cmake configuration.  This would mean it
> could say, detect you want opencv/mp/protobuf (if you're using onnx) and
> then it'd check out the pinned version we specify and build for your
> platform.
> 
> 
> On Tue, Mar 6, 2018 at 6:19 PM, Chris Olivier  wrote:
> 
> > Here is discussion:
> >
> > https://github.com/apache/incubator-mxnet/issues/8702
> >
> > On Tue, Mar 6, 2018 at 9:14 AM, Chris Olivier 
> > wrote:
> >
> > > This was agreed upon some time ago in a github issue thread, unless there
> > > are new objections to it.
> > >
> > > As far as I know, it's just a matter of someone putting in the work to
> > add
> > > more functionality to cmake or to fuse the two builds.
> > >
> > > One solution for the short term might include having the Makefile launch
> > > cmake for most of the build and fall back to Makefile for some of the
> > > remaining stuff, like scalapkg, rpkg, etc.
> > >
> > > btw, cmake uses the openmp in 3rdparty
> > >
> > >
> > >
> > > On Tue, Mar 6, 2018 at 8:51 AM, Pedro Larroy <
> > pedro.larroy.li...@gmail.com
> > > > wrote:
> > >
> > >> Hi
> > >>
> > >> I would like to raise the issue that maintaining two overlapping build
> > >> systems is too much overhead. It adds unnecessary complexity and
> > >> differences on how the project is built.
> > >>
> > >> For example, openmp is used differently from CMake and Make, in the
> > former
> > >> the one provided by gcc is used and in the later is compiled from the
> > >> 3rdparty folder.
> > >>
> > >> I think this situation is not sustainable for the project, and specially
> > >> if
> > >> we add that we want to support compilation and cross compilation on
> > >> devices.
> > >>
> > >> My proposal would be to identify any gaps that are not covered by the
> > >> CMake
> > >> build system, cover them and make CMake the single build system for
> > MXNet,
> > >> well tested and fully supported.
> > >>
> > >> Pedro.
> > >>
> > >
> > >
> >
> 

Re: Following semantic versioning

[Eric Xie]

One potential problem is with libstdc++. Specifically with vectors. Our 
operator interface uses vectors, which breaks when core lib and extension are 
compiled with different compiler version

On 2018/03/06 22:45:16, Chris Olivier  wrote: 
> The static init of your module should register your operator just as it
> does for the operators in mxnet (NNVM_REGISTER_OP).  While I haven't done
> it personally, I see no reason why it wouldn't work like any other operator
> at that point.
> 
> On Tue, Mar 6, 2018 at 1:28 PM, Barber, Christopher <
> christopher.bar...@analog.com> wrote:
> 
> > There are two separate issues: how to compile against the MXNet source and
> > how to dynamically load external dynamic libraries. While it would be nice
> > to have all necessary headers in the same place, it doesn't really matter
> > that much if people building extensions have to have access to the entire
> > MXNet source tree. The real issue is whether you support the ability to
> > dynamically load such extensions.
> >
> > - Christopher
> >
> > On 3/6/18, 4:12 PM, "Chris Olivier"  wrote:
> >
> > This was discussed in the past and so far, it seems possible for Unix
> > builds, although it’s going to be messy since the compile would
> > generally
> > need access to all a large portion of the headers in the source tree,
> > since
> > the “things needed to create your own op” aren’t necessarily all
> > contained
> > in the include/mxnet directory.
> >
> > On Tue, Mar 6, 2018 at 11:40 AM kellen sunderland <
> > kellen.sunderl...@gmail.com> wrote:
> >
> > > Could we actually just define a mechanism so the libs could register
> > their
> > > ops at runtime?  No linking required?
> > >
> > > On Tue, Mar 6, 2018, 8:36 PM Pedro Larroy <
> > pedro.larroy.li...@gmail.com>
> > > wrote:
> > >
> > > > This is a good point. What additional blockers would there be for
> > linking
> > > > against a user provided library with custom operators?
> > > >
> > > >
> > > >
> > > > On Tue, Mar 6, 2018 at 5:16 PM, Barber, Christopher <
> > > > christopher.bar...@analog.com> wrote:
> > > >
> > > > > To avoid this kind of problem, you really need to support
> > features that
> > > > > allow MXNet to be extended without having to resort to forking.
> > There
> > > is
> > > > > currently no way to add C++ custom operators without forking,
> > and no
> > > way
> > > > to
> > > > > share such operators across projects. This creates a perverse
> > incentive
> > > > to
> > > > > try to get changes that may not belong into the main product.
> > > > >
> > > > > On 3/5/18, 6:26 PM, "Marco de Abreu" <
> > marco.g.ab...@googlemail.com>
> > > > > wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > we recently had a few cases in which it has been attempted
> > to add
> > > new
> > > > > functionality to old branches because a customer of somebodys
> > > > $DAY_JOB
> > > > > requested it and was unable to switch to the latest release
> > or that
> > > > > certain
> > > > > feature did not make it into the release. This lead to quite
> > a lot
> > > of
> > > > > discussions and there was no clear standing within the
> > community.
> > > > >
> > > > > Just to cite semantic versioning:
> > > > >
> > > > >1. MAJOR version when you make incompatible API changes,
> > > > >2. MINOR version when you add functionality in a
> > > > > backwards-compatible
> > > > >manner, and
> > > > >3. PATCH version when you make backwards-compatible bug
> > fixes.
> > > > >
> > > > >
> > > > > We as a community agreed on following this system and I
> > think we
> > > > should
> > > > > either stick to it or drop it entirely - exceptions to
> > SemVer are
> > > > > usually
> > > > > discouraged. While I see that adding functionality might be
> > a minor
> > > > > thing,
> > > > > I don't think that we should educate our users into
> > expecting us to
> > > > > backport new features. The development happening on the
> > Apache
> > > MXNet
> > > > > repository should not be influenced by something like this;
> > > > especially
> > > > > considering that whoever supports that customer on their
> > $DAY_JOB
> > > can
> > > > > assist them at creating a fork and cherrypicking that
> > feature. But
> > > I
> > > > > don't
> > > > > see much reason why we're running against best pracitices.
> > One
> > > > > important
> > > > > thing to note here is that we're not maintaining CI on old
> > branches
> > > > and
> > > > > neither are we making patch releases - so what do these
> > users do?
> > > > > Compile
> > > > > off a 

Re: [RESULT][VOTE] tracking code changes with JIRA by associating pull requests

[Eric Xie]

-1

JIRA is ancient and arcane. This adds unnecessary overhead.

On 2018/03/03 06:11:12, CodingCat  wrote: 
> This vote passes with 6 +1 votes (6 bindings) and no 0 or -1 votes.
> 
> Binding +1:
> Chris Olivier
> Indhu Bharathi
> Suneel Marthi
> Yuan Tang
> Marco de Abreu
> Sebastian Schelter
> 
> 
> 
> Vote thread:
> https://lists.apache.org/list.html?d...@mxnet.apache.org:lte=1M:tracking%20code%20changes%20with%20JIRA%20by%20associating%20pull%20requests
> 
> I will continue with pushing the content to wiki and take it into practice
> 

Re: Protected master needs to be turned off

[Eric Xie]

Since committers voted for +1. We consider this vote passed.

Thanks,
Eric



On 2017-11-19 12:51, "Eric Xie"<j...@apache.org> wrote: 
> Hi all,
> I'm starting this thread to vote on turning off protected master. The reasons 
> are:
> 
> 1. Since we turned on protected master pending PRs has grown from 40 to 80. 
> It is severely slowing down development.
> 
> 2. Committers, not CI, are ultimately responsible for the code they merge. 
> You should only override the CI when you are very confident that CI is the 
> problem, not your code. If it turns out you are wrong, you should fix it 
> ASAP. This is the bare minimum requirement for all committers: BE RESPONSIBLE.
> 
> I'm aware of the argument for using protected master: It make sure that 
> master is stable.
> 
> Well, master will be most stable if we stop adding any commits to it. But 
> that's not what we want is it?
> 
> Protected master hardly adds any stability. The faulty tests that breaks 
> master at random got merged into master because they happened to succeed once.
> 
> Thanks,
> Junyuan Xie
> 

Re: 3rdparty packages as submodules

[Eric Xie]

I'm fine with a 3rdparty folder. Not sure about apache legal.

On 2017-11-17 10:25, Chris Olivier  wrote: 
> All,
> 
> I often find it desirable to have a method for 3rdparty packages to be
> included (possibly optionally) in a 3rdparty directory.   We do this with
> 'cub' to some degree, but it's in the root and is actually a fork in the
> dmlc repository.  Some samples of what might go in there:
> 
> 1) Intel OpenMP (llvm-openmp) -- In order to use Intel OMP by default
> 2) gperftools -- In order to build statically with -fPIC, which isn't the
> case with the general distribution
> 3) mkl-dnn -- In order to build and have debug information available for
> mkl-dnn (and possibly submit bugfixes)
> 
> What do you all think?
> 
> -Chris
> 

Re: Protected master needs to be turned off

[Eric Xie]

Committer right is a privilege. If someone keeps merging untested code and 
refuses to fix the problems they have caused then he or she shouldn't be a 
committer.

Using protected master doesn't guarantee that code is well tested. You can 
disable the test and merge code instead of actually fixing the bug. Using 
protected master actually encourages this behavior, since people are focused on 
getting tests to pass. It doesn't do much good towards improving code quality.

As far as I know, no other team managing large code bases uses protected master 
without the ability to override.

Thanks,
Junyuan Xie


On 2017-11-19 13:51, Marco de Abreu <marco.g.ab...@googlemail.com> wrote: 
> Hello,
> 
> -1 (non binding)
> 
> Who is going to be responsible for changes breaking tests and having other
> side effects after they have been merged? I'm afraid that this will harm
> further development. At the moment I'm the responsible person for setting
> up the new CI and so far have my results shown that not the CI itself is
> the problem but also the stability of our code as well as the tests
> themselves. At the moment we are having big issues to get a stable CI
> because MXNet seems to be relying on so specific architectures,
> dependencies and other factors which I'm not even able to track down that
> this causes everything to be unstable.
> 
> Just to point it out: If we encounter so many problems while setting up a
> CI system, doesn't that mean that our users and customers are also going to
> face those issues as soon as things are getting more complicated? This is a
> red flag in my opinion and I'm really looking forward to the usability
> Sprint, but at the moment I'm afraid that an unprotected master will make
> the situation even worse. It's already enough work to isolate and fix the
> current issues, but if new untested changes get merged, this is going to be
> like fighting a wildfire with a bottle of water.
> 
> So please revise your thoughts. If anybody is blocked by the protected
> master, I would really appreciate it if they could approach me personally
> in order to help stabilising the current situation. Just feeding in more
> and more changes on one end while we're fixing issues on the other end
> won't get us anywhere.
> 
> Best regards,
> Marco
> 
> Am 19.11.2017 10:08 nachm. schrieb "Chris Olivier" <cjolivie...@gmail.com>:
> 
> > Revised:
> >
> >
> > +1 at least until new CI is implemented. Then reevaluate.
> >
> > On Sun, Nov 19, 2017 at 1:07 PM Chris Olivier <cjolivie...@gmail.com>
> > wrote:
> >
> > > +1
> > >
> > >
> > > On Sun, Nov 19, 2017 at 12:52 PM Zha, Sheng <zhash...@amazon.com> wrote:
> > >
> > >> +1
> > >>
> > >> Best regards,
> > >> -sz
> > >>
> > >> On 11/19/17, 12:51 PM, "Eric Xie" <j...@apache.org> wrote:
> > >>
> > >> Hi all,
> > >> I'm starting this thread to vote on turning off protected master.
> > The
> > >> reasons are:
> > >>
> > >> 1. Since we turned on protected master pending PRs has grown from 40
> > >> to 80. It is severely slowing down development.
> > >>
> > >> 2. Committers, not CI, are ultimately responsible for the code they
> > >> merge. You should only override the CI when you are very confident that
> > CI
> > >> is the problem, not your code. If it turns out you are wrong, you should
> > >> fix it ASAP. This is the bare minimum requirement for all committers: BE
> > >> RESPONSIBLE.
> > >>
> > >> I'm aware of the argument for using protected master: It make sure
> > >> that master is stable.
> > >>
> > >> Well, master will be most stable if we stop adding any commits to
> > it.
> > >> But that's not what we want is it?
> > >>
> > >> Protected master hardly adds any stability. The faulty tests that
> > >> breaks master at random got merged into master because they happened to
> > >> succeed once.
> > >>
> > >> Thanks,
> > >> Junyuan Xie
> > >>
> > >>
> > >>
> > >>
> >
> 

Re: Disable 'required status check' for master

[Eric Xie]

Its the failed ones that's the problem. They fail because CI is not working 
properly. 
For example this trivial change to doc cannot be merged because CI failed: 
https://github.com/apache/incubator-mxnet/pull/8555/files

On 2017-10-31 13:36, Chris Olivier <cjolivie...@gmail.com> wrote: 
> I see 73 PR's for mxnet.
> 
> 17 of those have successfully passed CI and are waiting for merge (assuming
> the CR passed human inspection).
> 21 Show some form of CI in progress.
> *Only 8 are in the queue in Jenkins*
> 
> The rest have failed CI for one reason or another.
> 
> 
> 
> 
> On Tue, Oct 31, 2017 at 1:17 PM, Eric Xie <j...@apache.org> wrote:
> 
> > The number of pending PRs is growing very fast. At the current rate it
> > will reach 200 before we can fix jenkins.
> >
> > Stability is not the only goal. Master will be most stable if we don't
> > push anything, but that's not what we want.
> >
> > Committers should be responsible for their commits. Good judgement is the
> > ultimate guarantee of good code.
> >
> > Thanks,
> > Eric
> >
> > On 2017-10-31 12:38, Chris Olivier <cjolivie...@gmail.com> wrote:
> > > -1
> > >
> > > I personally think it's a necessary evil and a good forcing factor to get
> > > CI fixed.
> > > Before that requirements, things that failed unit tests were being pushed
> > > into master daily.  It was a big problem.  At least now master is stable.
> > >
> > > On Tue, Oct 31, 2017 at 12:22 PM, Hen <bay...@apache.org> wrote:
> > >
> > > > This makes sense to me. CI isn't of value if it isn't continuous. +1.
> > > >
> > > > On Tue, Oct 31, 2017 at 12:07 PM, Indhu <indhubhara...@gmail.com>
> > wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > We have been having issues getting CI to work fast enough. Currently
> > the
> > > > CI
> > > > > is being the bottleneck to get commits in. This is severely impacting
> > > > > development. I propose we disable 'required status check' for the
> > master
> > > > > branch so that we can development is not impacted. We can work on
> > fixing
> > > > > the CI situation in parallel.
> > > > >
> > > > > Committer,
> > > > > Please vote if you are okay with disabling 'required status check'
> > for
> > > > > master.
> > > > >
> > > > > Once we have enough votes, I'll request a mentor to file a ticket
> > with
> > > > > infra.
> > > > >
> > > > > Thanks,
> > > > > Indu
> > > > >
> > > >
> > >
> >
> 

Re: Disable 'required status check' for master

[Eric Xie]

The number of pending PRs is growing very fast. At the current rate it will 
reach 200 before we can fix jenkins.

Stability is not the only goal. Master will be most stable if we don't push 
anything, but that's not what we want.

Committers should be responsible for their commits. Good judgement is the 
ultimate guarantee of good code.

Thanks,
Eric

On 2017-10-31 12:38, Chris Olivier  wrote: 
> -1
> 
> I personally think it's a necessary evil and a good forcing factor to get
> CI fixed.
> Before that requirements, things that failed unit tests were being pushed
> into master daily.  It was a big problem.  At least now master is stable.
> 
> On Tue, Oct 31, 2017 at 12:22 PM, Hen  wrote:
> 
> > This makes sense to me. CI isn't of value if it isn't continuous. +1.
> >
> > On Tue, Oct 31, 2017 at 12:07 PM, Indhu  wrote:
> >
> > > Hi,
> > >
> > > We have been having issues getting CI to work fast enough. Currently the
> > CI
> > > is being the bottleneck to get commits in. This is severely impacting
> > > development. I propose we disable 'required status check' for the master
> > > branch so that we can development is not impacted. We can work on fixing
> > > the CI situation in parallel.
> > >
> > > Committer,
> > > Please vote if you are okay with disabling 'required status check' for
> > > master.
> > >
> > > Once we have enough votes, I'll request a mentor to file a ticket with
> > > infra.
> > >
> > > Thanks,
> > > Indu
> > >
> >
>