mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From YiZhi Liu <eazhi....@gmail.com>
Subject Re: The operator check for Scala Package
Date Thu, 21 Jun 2018 02:32:13 GMT
Hi Qing,
What is the exact risk of changing / adding operators? could you
provide an example? I also feel the way you proposed is little bit too
heavy to developers, and not quite friendly to new contributors.
On Wed, Jun 20, 2018 at 10:22 AM Haibin Lin <haibin.lin.aws@gmail.com> wrote:
>
> I appreciate the effort and understand the motivation. However, I'm
> concerned that it basically means merging operator PRs becomes sequential.
> Developers who work on operators have to update their PR every time a new
> operator is merged to master, the burden becomes significant if there're 20
> ONNX/sparse operators to add and many PRs are submitted/reviewed in
> parallel.
>
> On Wed, Jun 20, 2018 at 10:13 AM, Qing Lan <lanking520@live.com> wrote:
>
> > Hi Haibin,
> >
> > The operator change is any changes on the operator on C++ side. Trigger
> > the check failed?
> >    - change the documentation of operator in C                          Yes
> >    - change the documentation such as README.md                 No
> >    - add/remove/modify operator                                 Yes
> >    - add/remove/modify operator parameter                               Yes
> >
> > Thanks,
> > Qing
> >
> > ´╗┐On 6/20/18, 10:01 AM, "Haibin Lin" <haibin.lin.aws@gmail.com> wrote:
> >
> >     Could you elaborate what you mean by operator change? Does it check the
> >     operator interface? Would updated operator documentation fail the
> > check?
> >     Would adding a new operator fail this check?
> >
> >
> >
> >     On Wed, Jun 20, 2018 at 9:48 AM, Qing Lan <lanking520@live.com> wrote:
> >
> >     > Hi Macro,
> >     >
> >     > Thanks for your feedback! I believe this should not be a block for
> >     > contributor in most of the cases.
> >     > Firstly, this would only be triggered if there is an operator changes
> >     > (Only general operators).
> >     > Secondly, it is simple to go through. Just need to change 1 line of
> > code
> >     > would make the PR passed. However it do requires contributor to do
> > this or
> >     > the Scalatest will fail. I have made the error message instructive
> > that
> >     > would help the contributor to dive in and make the changes.
> >     >
> >     > I also updated the design document to explain in detail.
> >     >
> >     > Thanks,
> >     > Qing
> >     >
> >     >
> >     > On 6/19/18, 12:09 PM, "Marco de Abreu" <marco.g.abreu@googlemail.com
> > .INVALID>
> >     > wrote:
> >     >
> >     >     Okay, thanks for elaborating. I definitely see your point there
> > and we
> >     >     definitely don't want these changes to pile up.
> >     >
> >     >     I don't feel strongly about this and won't stand in the way, I
> > just
> >     > want to
> >     >     express my concern that this could lead to people having to
> > touch all
> >     >     language interfaces although they might not familiar with them
> > at all.
> >     > On
> >     >     the other hand we got enough contributors who could help them
> > then
> >     > before
> >     >     the PR can get merged. So either way works, but I just wanted to
> >     > highlight
> >     >     that this could make it harder to make changes in the backend for
> >     > people
> >     >     who are not familiar with our frontend API languages. If we got
> > enough
> >     >     people who could actively support our contributors in such a
> > case, we
> >     >     should be totally fine with blocking a PR until the APIs have
> > been
> >     > adapted.
> >     >
> >     >     -Marco
> >     >
> >     >     On Tue, Jun 19, 2018 at 11:58 AM Naveen Swamy <
> > mnnaveen@gmail.com>
> >     > wrote:
> >     >
> >     >     > Marco,
> >     >     >
> >     >     > Qing and I are working together on this. The idea is that we
> > fail
> >     > the build
> >     >     > if there is a operator change on the backend and have not
> > synced to
> >     > the
> >     >     > Scala API. We want to catch this before breaking the user's
> > code
> >     > which will
> >     >     > be a pretty bad experience.
> >     >     >
> >     >     >
> >     >     >
> >     >     > On Tue, Jun 19, 2018 at 11:54 AM, Marco de Abreu <
> >     >     > marco.g.abreu@googlemail.com.invalid> wrote:
> >     >     >
> >     >     > > Hi Qing,
> >     >     > >
> >     >     > > thank you for working on improving the compatibility of our
> > APIs!
> >     >     > >
> >     >     > > Your linked proposal does not describe the mentioned
> > FILEHASH.
> >     > Could you
> >     >     > > elaborate a bit? Would this be a hash of the entire file,
> > some hash
> >     >     > created
> >     >     > > based on the signature of the underlying C++ methods or
> > maybe a
> >     > different
> >     >     > > approach?
> >     >     > >
> >     >     > > Also, at which step would developers be notified of the
> > change? I'd
> >     >     > propose
> >     >     > > that we make this check a nightly job to prevent it from
> > blocking
> >     > a PR
> >     >     > and
> >     >     > > forcing contributors who are not familiar with Scala having
> > to
> >     > make a
> >     >     > > change to the Scala package. This would allow us to follow
up
> >     >     > > asynchronously but still provide actionable events that one
> > can be
> >     >     > > subscribed to.
> >     >     > >
> >     >     > > Best regards,
> >     >     > > Marco
> >     >     > >
> >     >     > > On Tue, Jun 19, 2018 at 11:00 AM Qing Lan <
> > lanking520@live.com>
> >     > wrote:
> >     >     > >
> >     >     > > > Hi all,
> >     >     > > >
> >     >     > > > I am one of the maintainer for MXNet Scala package.
> > Currently I
> >     > am
> >     >     > > > building up a hash-check system on the generated API
> > through C.
> >     > The PR
> >     >     > > is
> >     >     > > > in this URL:
> >     >     > > > https://github.com/apache/incubator-mxnet/pull/11239
> >     >     > > > A file named FILEHASH will be added to the Scala that
> > created
> >     > the MD5
> >     >     > > > string of the generated API document. It will check
the
> >     > signature of
> >     >     > all
> >     >     > > > the operators during Scala CI testing. The reason I
am
> > doing
> >     > this is to
> >     >     > > > make sure Scala developers will always be reminded if
> > there is an
> >     >     > > operator
> >     >     > > > name/argument changes happened in different PRs. More
> > detailed
> >     > info
> >     >     > > > explained in here:
> >     >     > > >
> >     >     > > > https://cwiki.apache.org/confluence/display/MXNET/
> >     >     > > MXNet+Scala+API+Usability+Improvement
> >     >     > > >
> >     >     > > > Pros:
> >     >     > > > This method will always help us keep backwards
> > compatibility of
> >     >     > operators
> >     >     > > > for Scala
> >     >     > > > Cons:
> >     >     > > > Require update on the FILEHASH with new contents when
> > there is an
> >     >     > > operator
> >     >     > > > change. Developers who changed the operator should `make
> >     > scalapkg` to
> >     >     > > > update that file.
> >     >     > > >
> >     >     > > > Please leave any thoughts you may have for this design
and
> > feel
> >     > free to
> >     >     > > > review the code.
> >     >     > > >
> >     >     > > > Thanks,
> >     >     > > > Qing
> >     >     > > >
> >     >     > > >
> >     >     > >
> >     >     >
> >     >
> >     >
> >     >
> >
> >
> >



-- 
Yizhi Liu
DMLC member
Amazon Web Services
Vancouver, Canada

Mime
View raw message