I think we should go with an asychronous approach using a nightly job that detects the changes and reports them accordingly. We could then send out a report if there is a mismatch.
I agree with the already stated points that we should not put the burden of adding frontend API bindings to somebody who adds a Backend function. This is not scalable and rather poses a risk in reduced quality or increased review overhead. The sparse support is the perfect example. I don't know if haibin knows Scala, but he would have had to make the entire mapping for Scala without being very familiar with it (sorry if I'm wrong on this haibin, it's just an example). Imagine we do this for even more APIs - we would basically block ourselves because everyone who makes a change in the Backend has to know a dozen languages. While in a few cases it might be just a few lines, I'd guess that especially for new operators it would require a proper design, coding and review to map an API to the frontend languages. This should rather be done by an expert in my opinion. We could define it as a requirement for a release that all APIs have had to be transfered before a release can be cut (We already have it as requirement that all nightly jobs have to pass anyways). -Marco Naveen Swamy <mnnav...@gmail.com> schrieb am Mi., 20. Juni 2018, 19:50: > I understand the concerns here. However the problem here is that since the > Scala APIs are generated using Macros and we do not(may not ever) have full > test coverage on each of the APIs, we will not know for example if an > operator/API changes on the backend and that propagates to Scala users, > their code might very well fail. So what we are trying here is to find out > early if there is an operator/API change and avoid breaking user's code. > > I think there is value in co-ordinating addition of APIs across bindings, > as an example the sparse APIs introduced was(still not) exposed to Scala > users. This begs the question of how do we co-ordinate such changes? Should > we turn addition of new APIs also as warnings ? > > I agree that we shouldn't fail on documentation change, may we should turn > that into warnings and make sure to pick it up on the Scala APIs, this is > low risk since it documentation does not change. > > Open for suggestions. > > > On Wed, Jun 20, 2018 at 7:32 PM, YiZhi Liu <eazhi....@gmail.com> wrote: > > > Hi Qing, > > What is the exact risk of changing / adding operators? could you > > provide an example? I also feel the way you proposed is little bit too > > heavy to developers, and not quite friendly to new contributors. > > On Wed, Jun 20, 2018 at 10:22 AM Haibin Lin <haibin.lin....@gmail.com> > > wrote: > > > > > > I appreciate the effort and understand the motivation. However, I'm > > > concerned that it basically means merging operator PRs becomes > > sequential. > > > Developers who work on operators have to update their PR every time a > new > > > operator is merged to master, the burden becomes significant if > there're > > 20 > > > ONNX/sparse operators to add and many PRs are submitted/reviewed in > > > parallel. > > > > > > On Wed, Jun 20, 2018 at 10:13 AM, Qing Lan <lanking...@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....@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 <lanking...@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.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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > Yizhi Liu > > DMLC member > > Amazon Web Services > > Vancouver, Canada > > >
