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 16:48:40 GMT
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