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 > > > > > > > > > > > > > > > > > > > > > > > > > >
