mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qing Lan <lanking...@live.com>
Subject Re: The operator check for Scala Package
Date Wed, 20 Jun 2018 17:13:22 GMT
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
    >     > > >
    >     > > >
    >     > >
    >     >
    >
    >
    >
    

Mime
View raw message