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