Agreed, for new APIs we can turn into a nightly job and report on dev@. The goal here is not to burden anyone but to catch changes on the backend before it propagates and break user's code and co-ordinate changes across language bindings which IMO is essential, so I would like to keep for changes on the existing API.
On Wed, Jun 20, 2018 at 7:58 PM, Marco de Abreu < [email protected]> wrote: > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> > > > 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 <[email protected]> > > 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" <[email protected]> > > 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 <[email protected] > > > > > 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" < > > > [email protected] > > > > > .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 < > > > > > [email protected]> > > > > > > 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 < > > > > > > > [email protected]> 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 < > > > > > [email protected]> > > > > > > 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 > > > > > >
