mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Haibin Lin <haibin.lin....@gmail.com>
Subject Re: The operator check for Scala Package
Date Wed, 20 Jun 2018 17:01:20 GMT
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