I don't think it's reasonable to fail on purpose in Scala when changing documentation or adding operators in C++. At least, it should follow the same behavior as we have for Python binding.
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 > > > > > > > > > > > > > > > > > > > > > > > > > >
