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.ab...@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 <mnnav...@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.ab...@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 <lanking...@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
> > >
> > >
> >
>
