mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jun Wu <wujun....@gmail.com>
Subject Re: The operator check for Scala Package
Date Wed, 20 Jun 2018 17:19:24 GMT
I don't think it's reasonable to fail on purpose in Scala when changing
documentation or adding operators in C++. At least, it should follow the
same behavior as we have for Python binding.

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
>     >     > > >
>     >     > > >
>     >     > >
>     >     >
>     >
>     >
>     >
>
>
>

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