Re: The operator check for Scala Package
Thanks Jun for your clarification. I think one of the advantages for MXNet is it's language binding. Although the number of customers using Scala is great less than Python, it is still one important language we need to support. About the first question, I would say we all in. At least all breaking changes should let all language binding maintainers notified. Then we make the changes to make sure we have stable support to the customers. The reason for Scala build failure could be many, so we need to diagnose the problem and see if we need to collaborate and solve the problems together. I don't think the other language maintainers and backend must take the responsibilities to diagnose a problems of a language they may not familiar with. 2. It depends differently case by case, we cannot bring a unique time for different failures we have. If you mean the operator check, it could cause you (time to build scalapkg) + (30 seconds to change the code) + (time to run the CI to make sure it worked) + (send a PR to make that change on master branch). This update should be done by the Scala developers for sure. 3. If there is a nightly build failure, maybe we can think of checking that on a weekly basis. Spend 15 mins every week to review the operator changes so we don't miss any important points. Thanks, Qing On 6/20/18, 9:57 PM, "Jun Wu" wrote: We should reach an agreement on the responsibility/ownership before moving forward. 1. Who will take the ownership of fixing Scala build failure if an operator is changed/added in C++? 2. What is the expected turn around time of fixing the scala build failure. 3. What if we are short of resources of fixing the scala build failure? How do we deal with the failing nightly CI every night? With all being said, there is at least one thing that must be clear, we certainly don't want to put the extra burden on the backend developers. On Wed, Jun 20, 2018 at 9:39 PM Qing Lan wrote: > Hi All, > > Thank you all for your feedback. This changes do influence a lot on the > PRs related to operator changes. I will take what Marco proposed to place > that in the nightly build rather than every CI we runs. > > Thanks, > Qing > > On 6/20/18, 8:40 PM, "Hao Jin" wrote: > > I don't think we should keep this hash check thing. As Qing stated > before > on this thread, if there's any change in documentation the hash value > is > also changed and then flagged as a problem, how will that break any > user's > code? I would go for something like Marco's proposal: moving this to an > asynchronous check. > At this very moment, I've got 4 PRs that are all related to "operator > changes", adopting this kind of method is adding extra work for me and > every other contributor whose work involves changes on operator code, > and I > don't think it's a reasonable kind of extra work like tracking PRs on > JIRA. > Hao > > On Wed, Jun 20, 2018 at 8:10 PM, Naveen Swamy > wrote: > > > 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 < > > marco.g.ab...@googlemail.com.invalid> 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
Re: The operator check for Scala Package
We should reach an agreement on the responsibility/ownership before moving forward. 1. Who will take the ownership of fixing Scala build failure if an operator is changed/added in C++? 2. What is the expected turn around time of fixing the scala build failure. 3. What if we are short of resources of fixing the scala build failure? How do we deal with the failing nightly CI every night? With all being said, there is at least one thing that must be clear, we certainly don't want to put the extra burden on the backend developers. On Wed, Jun 20, 2018 at 9:39 PM Qing Lan wrote: > Hi All, > > Thank you all for your feedback. This changes do influence a lot on the > PRs related to operator changes. I will take what Marco proposed to place > that in the nightly build rather than every CI we runs. > > Thanks, > Qing > > On 6/20/18, 8:40 PM, "Hao Jin" wrote: > > I don't think we should keep this hash check thing. As Qing stated > before > on this thread, if there's any change in documentation the hash value > is > also changed and then flagged as a problem, how will that break any > user's > code? I would go for something like Marco's proposal: moving this to an > asynchronous check. > At this very moment, I've got 4 PRs that are all related to "operator > changes", adopting this kind of method is adding extra work for me and > every other contributor whose work involves changes on operator code, > and I > don't think it's a reasonable kind of extra work like tracking PRs on > JIRA. > Hao > > On Wed, Jun 20, 2018 at 8:10 PM, Naveen Swamy > wrote: > > > 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 < > > marco.g.ab...@googlemail.com.invalid> 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 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. > > > >
Re: The operator check for Scala Package
Hi All, Thank you all for your feedback. This changes do influence a lot on the PRs related to operator changes. I will take what Marco proposed to place that in the nightly build rather than every CI we runs. Thanks, Qing On 6/20/18, 8:40 PM, "Hao Jin" wrote: I don't think we should keep this hash check thing. As Qing stated before on this thread, if there's any change in documentation the hash value is also changed and then flagged as a problem, how will that break any user's code? I would go for something like Marco's proposal: moving this to an asynchronous check. At this very moment, I've got 4 PRs that are all related to "operator changes", adopting this kind of method is adding extra work for me and every other contributor whose work involves changes on operator code, and I don't think it's a reasonable kind of extra work like tracking PRs on JIRA. Hao On Wed, Jun 20, 2018 at 8:10 PM, Naveen Swamy wrote: > 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 < > marco.g.ab...@googlemail.com.invalid> 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 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 > 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 > > > >
Re: The operator check for Scala Package
I don't think we should keep this hash check thing. As Qing stated before on this thread, if there's any change in documentation the hash value is also changed and then flagged as a problem, how will that break any user's code? I would go for something like Marco's proposal: moving this to an asynchronous check. At this very moment, I've got 4 PRs that are all related to "operator changes", adopting this kind of method is adding extra work for me and every other contributor whose work involves changes on operator code, and I don't think it's a reasonable kind of extra work like tracking PRs on JIRA. Hao On Wed, Jun 20, 2018 at 8:10 PM, Naveen Swamy wrote: > 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 < > marco.g.ab...@googlemail.com.invalid> 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 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 > 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 > > > 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 > > > >
Re: The operator check for Scala Package
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 < marco.g.ab...@googlemail.com.invalid> 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 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 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 > > > 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 > > 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" > > 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 > > > > 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 > >
Re: [LAZY VOTE] Test coverage of PRs
we should have a betting pool on what the current coverage is. On Wed, Jun 20, 2018 at 7:54 PM Naveen Swamy wrote: > +1 to collect data on coverage. > > On Wed, Jun 20, 2018 at 7:38 PM, Marco de Abreu < > marco.g.ab...@googlemail.com.invalid> wrote: > > > Hello, > > > > for now, this is only a test for myself and a way to gather data in order > > to give us the possibility to make a data-driven decision. So far, we > have > > not decided on the implications and which restrictions we will set as > > result for the produced metrics and I'd like to postpone that until we > know > > the confidence of the system. This impact is one of the aspects I'd like > to > > assess in the next weeks. > > > > The initial goal is to give the reviewer an additional tool to assess the > > coverage of one's PR. One example could be the optimization of some > backend > > logic. The reviewer would then see if the changed codeis actually being > hit > > by any tests or if they are being missed entirely. I agree that just the > > percentage could be highly misleading and we should review that very > > clearly. > > > > For now, I'd only like to gather the data in the background. I would > follow > > up with a big thread on dev@ about my observations, my recommendations > and > > the possible options we have to use the data. We can then decide as > > community how exactly we would like to proceed and how much importance we > > give to the generated reports. > > > > Best regards, > > Marco > > > > > > > > On Wed, Jun 20, 2018 at 7:23 PM Tianqi Chen > > wrote: > > > > > While I think test coverage is a nice information to have. I would > object > > > to using this as a metric to decide whether a PR should be merged. > > > Code-cov act as a mere coverage of APIs, which is a useful aspect, it > can > > > be misleading in many cases, especially when such change involves > > > cross-language APIs and automatically generated wrapper. > > > Sometimes the code-cov shows a negative impact on coverage while CI > > passes, > > > and the giving information was quite misleading if you just look at the > > PR > > > tabs > > > > > > I would still trust the reviewer's decision on the pull request > merging. > > > > > > Tianqi > > > > > > On Wed, Jun 20, 2018 at 7:14 PM, Marco de Abreu < > > > marco.g.ab...@googlemail.com.invalid> wrote: > > > > > > > Hello, > > > > > > > > I'd like to introduce test coverage metrics of PRs using > > > > https://codecov.io/. > > > > This tool will aggregate coverage reports across multiple runs, > > platforms > > > > and technologies and gives contributors as well as reviewers a new > tool > > > > that allows to improve the quality of a pull request. > > > > > > > > Since we need to gather some data first, I'd like to request merging > > > > https://github.com/apache/incubator-mxnet/pull/11344. This will > enable > > > > publishing the coverage data to the service and have no impact on > your > > > PRs > > > > - it will just allow me to assess the quality of the service in the > > > > background while I come up with a full integration design. My initial > > > plan > > > > is to start with coverage across Python and C++ and then, with the > help > > > of > > > > our community, extend the report across all our supported languages. > > > > > > > > Does anybody object to having us gather this data in the background? > > > > > > > > Best regards, > > > > Marco > > > > > > > > > >
Re: The operator check for Scala Package
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 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 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 > > 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 > 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" > 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 > > 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
Re: [LAZY VOTE] Test coverage of PRs
+1 to collect data on coverage. On Wed, Jun 20, 2018 at 7:38 PM, Marco de Abreu < marco.g.ab...@googlemail.com.invalid> wrote: > Hello, > > for now, this is only a test for myself and a way to gather data in order > to give us the possibility to make a data-driven decision. So far, we have > not decided on the implications and which restrictions we will set as > result for the produced metrics and I'd like to postpone that until we know > the confidence of the system. This impact is one of the aspects I'd like to > assess in the next weeks. > > The initial goal is to give the reviewer an additional tool to assess the > coverage of one's PR. One example could be the optimization of some backend > logic. The reviewer would then see if the changed codeis actually being hit > by any tests or if they are being missed entirely. I agree that just the > percentage could be highly misleading and we should review that very > clearly. > > For now, I'd only like to gather the data in the background. I would follow > up with a big thread on dev@ about my observations, my recommendations and > the possible options we have to use the data. We can then decide as > community how exactly we would like to proceed and how much importance we > give to the generated reports. > > Best regards, > Marco > > > > On Wed, Jun 20, 2018 at 7:23 PM Tianqi Chen > wrote: > > > While I think test coverage is a nice information to have. I would object > > to using this as a metric to decide whether a PR should be merged. > > Code-cov act as a mere coverage of APIs, which is a useful aspect, it can > > be misleading in many cases, especially when such change involves > > cross-language APIs and automatically generated wrapper. > > Sometimes the code-cov shows a negative impact on coverage while CI > passes, > > and the giving information was quite misleading if you just look at the > PR > > tabs > > > > I would still trust the reviewer's decision on the pull request merging. > > > > Tianqi > > > > On Wed, Jun 20, 2018 at 7:14 PM, Marco de Abreu < > > marco.g.ab...@googlemail.com.invalid> wrote: > > > > > Hello, > > > > > > I'd like to introduce test coverage metrics of PRs using > > > https://codecov.io/. > > > This tool will aggregate coverage reports across multiple runs, > platforms > > > and technologies and gives contributors as well as reviewers a new tool > > > that allows to improve the quality of a pull request. > > > > > > Since we need to gather some data first, I'd like to request merging > > > https://github.com/apache/incubator-mxnet/pull/11344. This will enable > > > publishing the coverage data to the service and have no impact on your > > PRs > > > - it will just allow me to assess the quality of the service in the > > > background while I come up with a full integration design. My initial > > plan > > > is to start with coverage across Python and C++ and then, with the help > > of > > > our community, extend the report across all our supported languages. > > > > > > Does anybody object to having us gather this data in the background? > > > > > > Best regards, > > > Marco > > > > > >
Re: The operator check for Scala Package
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 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 > 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 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" 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 > 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 > > > > >
Re: The operator check for Scala Package
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 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 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" 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 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" > .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
Re: [LAZY VOTE] Test coverage of PRs
While I think test coverage is a nice information to have. I would object to using this as a metric to decide whether a PR should be merged. Code-cov act as a mere coverage of APIs, which is a useful aspect, it can be misleading in many cases, especially when such change involves cross-language APIs and automatically generated wrapper. Sometimes the code-cov shows a negative impact on coverage while CI passes, and the giving information was quite misleading if you just look at the PR tabs I would still trust the reviewer's decision on the pull request merging. Tianqi On Wed, Jun 20, 2018 at 7:14 PM, Marco de Abreu < marco.g.ab...@googlemail.com.invalid> wrote: > Hello, > > I'd like to introduce test coverage metrics of PRs using > https://codecov.io/. > This tool will aggregate coverage reports across multiple runs, platforms > and technologies and gives contributors as well as reviewers a new tool > that allows to improve the quality of a pull request. > > Since we need to gather some data first, I'd like to request merging > https://github.com/apache/incubator-mxnet/pull/11344. This will enable > publishing the coverage data to the service and have no impact on your PRs > - it will just allow me to assess the quality of the service in the > background while I come up with a full integration design. My initial plan > is to start with coverage across Python and C++ and then, with the help of > our community, extend the report across all our supported languages. > > Does anybody object to having us gather this data in the background? > > Best regards, > Marco >
[LAZY VOTE] Test coverage of PRs
Hello, I'd like to introduce test coverage metrics of PRs using https://codecov.io/. This tool will aggregate coverage reports across multiple runs, platforms and technologies and gives contributors as well as reviewers a new tool that allows to improve the quality of a pull request. Since we need to gather some data first, I'd like to request merging https://github.com/apache/incubator-mxnet/pull/11344. This will enable publishing the coverage data to the service and have no impact on your PRs - it will just allow me to assess the quality of the service in the background while I come up with a full integration design. My initial plan is to start with coverage across Python and C++ and then, with the help of our community, extend the report across all our supported languages. Does anybody object to having us gather this data in the background? Best regards, Marco
Re: users@mxnet
+1 On Wed, Jun 20, 2018 at 8:43 AM Steffen Rochel wrote: > I had a discussion yesterday with Jun Wu (wujun@gmail.com) to get a > better understanding about the concerns raised, that users might get > confused and maintenance efforts. > I agree with Jim that building and fostering the community is important. > First of all, I suggest we should be open minded and not make claims that > we have a good understanding of user preferences. We might have insights > about preferences of current users (which I also would question as we > sampled only a small set), but we certainly don't have insight about the > preferences of new users we are trying to attract. > In such situation it might be better to run an experiment, offer choices > and collect real feedback - lets be customer focussed. > My suggestion is to establish a user@ list and support the list with a > volunteer subset of contributors and committers to minimize the maintenance > impact on the whole community. > After a reasonable time like 6 months we can evaluate the adoption of user@ > and effort to support and can make an informed, data driven decision how to > proceed. > > I recommend to create the user@ list and call for volunteers to support > the > list. > > Regards, > Steffen > > On Tue, Jun 19, 2018 at 8:10 AM Hagay Lupesko wrote: > > > Jim, > > > > Earlier on the thread you suggested to clarify and expand on the usage > of a > > user@ mailing list and how it is useful for a project. > > > > It may be helpful for the community to learn a bit more about it. Could > you > > expand and/or share relevant links and examples? > > > > Thank you, > > Hagay > > > > On Tue, Jun 19, 2018, 07:31 Jim Jagielski wrote: > > > > > Just so we are clear: building and fostering a community takes effort. > > > Either it is something important to the project, or it's not. > > > > > > My assumption is that It Is. > > > > > > > On Jun 18, 2018, at 8:59 PM, YiZhi Liu wrote: > > > > > > > > I am personally not a big fan of mailing list but agree with Thomas > > > > that we may get extra users, which worth a try. > > > > On the other hand, I also have concern that we do not have a > community > > > > big enough to support multiple forums. If people asked questions but > > > > got no response, that can be worse than not having the mailing list > at > > > > all. > > > > On Mon, Jun 18, 2018 at 5:46 PM Thomas DELTEIL > > > > wrote: > > > >> > > > >> I was actually the one stating that we didn't need a user mailing > list > > > >> during the Seattle meetup, given all the reasons already exposed > > above. > > > >> > > > >> However given what proponents of a mailing list said, I personally > > > wouldn't > > > >> mind adding a new channel as a user mailing list, and monitoring it. > > > There > > > >> seems to be a subset of users, used to apache projects, that > wouldn't > > > use > > > >> the forum but would use a mailing list. Though I think it is not as > > > >> feature-rich as the forum and there is a risk of dilution of > > > information. > > > >> It is more about reaching those extra users. If we see a dilution of > > > >> traffic on the forum towards the mailing list (~currently 100 > > > posts/week) > > > >> then maybe we can reconsider our assumptions? > > > >> > > > >> All the best, > > > >> > > > >> Thomas Delteil > > > >> > > > >> On Mon, Jun 18, 2018, 17:30 Pedro Larroy < > > pedro.larroy.li...@gmail.com> > > > >> wrote: > > > >> > > > >>> I agree with Tianqi, Eric and others. We shouldn't dilute the > > community > > > >>> with another forum. Disqus is already working and has healthy > > > >>> participation, you can get an email digest if you so desire. > > > Subscribing to > > > >>> a mailing list to get a question answered is quite a heavyweight > > > investment > > > >>> for many people and users who might not have the resources nor > mental > > > >>> bandwidth to receive more email volume in their inboxes. > > > >>> > > > >>> On Mon, Jun 18, 2018 at 10:19 AM Tianqi Chen < > > tqc...@cs.washington.edu > > > > > > > >>> wrote: > > > >>> > > > The problem of having multiple separate channels of communication > is > > > that > > > users get confused, and the cost of maintenance goes up(people > have > > to > > > watch both). As the current community was at discuss forum and > many > > > users > > > prefer it, having a mail-list is only a burden we will bring > > > > > > Tianqi > > > > > > On Mon, Jun 18, 2018 at 9:48 AM, Jim Jagielski > > > wrote: > > > > > > > IMO, that is the wrong way to look at it. > > > > > > > > A users@ mailing list is a great, easy, low-cost and > low-overhead > > > way > > > >>> of > > > > *increasing* the user community and providing an extra level of > > > >>> support. > > > > Unless there is "strong evidence" that this is NOT the case, I > > would > > > > recommend we create the list. > > > > > > > >> On Jun 16, 2018, at 12:28 AM, Tianqi Chen <
Re: [VOTE] Release MXNet version 1.2.1.RC0 (Patch Release)
+1 On Mon, Jun 18, 2018, 6:52 PM Anirudh wrote: > Hi, > > This is the vote to release Apache MXNet (incubating) version 1.2.1. Voting > will start now and close Thursday June 21st 7:00 PM PDT. > > Link to release candidate 1.2.1.rc0: > > https://github.com/apache/incubator-mxnet/releases/tag/1.2.1.rc0 > > View this page for installation instructions: > > https://mxnet.incubator.apache.org/install/index.html > > (Note: The README.md points to the 1.2.1 tag and does not work at the > moment). > > Please remember to test first before voting accordingly. > > +1 = approve > +0 = no opinion > -1 = disapprove (provide reason) > > Anirudh >
Re: The operator check for Scala Package
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 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" 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 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" .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
Re: The operator check for Scala Package
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" 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 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 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 > > > > > > > > >
Re: users@mxnet
I had a discussion yesterday with Jun Wu (wujun@gmail.com) to get a better understanding about the concerns raised, that users might get confused and maintenance efforts. I agree with Jim that building and fostering the community is important. First of all, I suggest we should be open minded and not make claims that we have a good understanding of user preferences. We might have insights about preferences of current users (which I also would question as we sampled only a small set), but we certainly don't have insight about the preferences of new users we are trying to attract. In such situation it might be better to run an experiment, offer choices and collect real feedback - lets be customer focussed. My suggestion is to establish a user@ list and support the list with a volunteer subset of contributors and committers to minimize the maintenance impact on the whole community. After a reasonable time like 6 months we can evaluate the adoption of user@ and effort to support and can make an informed, data driven decision how to proceed. I recommend to create the user@ list and call for volunteers to support the list. Regards, Steffen On Tue, Jun 19, 2018 at 8:10 AM Hagay Lupesko wrote: > Jim, > > Earlier on the thread you suggested to clarify and expand on the usage of a > user@ mailing list and how it is useful for a project. > > It may be helpful for the community to learn a bit more about it. Could you > expand and/or share relevant links and examples? > > Thank you, > Hagay > > On Tue, Jun 19, 2018, 07:31 Jim Jagielski wrote: > > > Just so we are clear: building and fostering a community takes effort. > > Either it is something important to the project, or it's not. > > > > My assumption is that It Is. > > > > > On Jun 18, 2018, at 8:59 PM, YiZhi Liu wrote: > > > > > > I am personally not a big fan of mailing list but agree with Thomas > > > that we may get extra users, which worth a try. > > > On the other hand, I also have concern that we do not have a community > > > big enough to support multiple forums. If people asked questions but > > > got no response, that can be worse than not having the mailing list at > > > all. > > > On Mon, Jun 18, 2018 at 5:46 PM Thomas DELTEIL > > > wrote: > > >> > > >> I was actually the one stating that we didn't need a user mailing list > > >> during the Seattle meetup, given all the reasons already exposed > above. > > >> > > >> However given what proponents of a mailing list said, I personally > > wouldn't > > >> mind adding a new channel as a user mailing list, and monitoring it. > > There > > >> seems to be a subset of users, used to apache projects, that wouldn't > > use > > >> the forum but would use a mailing list. Though I think it is not as > > >> feature-rich as the forum and there is a risk of dilution of > > information. > > >> It is more about reaching those extra users. If we see a dilution of > > >> traffic on the forum towards the mailing list (~currently 100 > > posts/week) > > >> then maybe we can reconsider our assumptions? > > >> > > >> All the best, > > >> > > >> Thomas Delteil > > >> > > >> On Mon, Jun 18, 2018, 17:30 Pedro Larroy < > pedro.larroy.li...@gmail.com> > > >> wrote: > > >> > > >>> I agree with Tianqi, Eric and others. We shouldn't dilute the > community > > >>> with another forum. Disqus is already working and has healthy > > >>> participation, you can get an email digest if you so desire. > > Subscribing to > > >>> a mailing list to get a question answered is quite a heavyweight > > investment > > >>> for many people and users who might not have the resources nor mental > > >>> bandwidth to receive more email volume in their inboxes. > > >>> > > >>> On Mon, Jun 18, 2018 at 10:19 AM Tianqi Chen < > tqc...@cs.washington.edu > > > > > >>> wrote: > > >>> > > The problem of having multiple separate channels of communication is > > that > > users get confused, and the cost of maintenance goes up(people have > to > > watch both). As the current community was at discuss forum and many > > users > > prefer it, having a mail-list is only a burden we will bring > > > > Tianqi > > > > On Mon, Jun 18, 2018 at 9:48 AM, Jim Jagielski > > wrote: > > > > > IMO, that is the wrong way to look at it. > > > > > > A users@ mailing list is a great, easy, low-cost and low-overhead > > way > > >>> of > > > *increasing* the user community and providing an extra level of > > >>> support. > > > Unless there is "strong evidence" that this is NOT the case, I > would > > > recommend we create the list. > > > > > >> On Jun 16, 2018, at 12:28 AM, Tianqi Chen < > tqc...@cs.washington.edu > > > > > > wrote: > > >> > > >> So unless there is a strong evidence that our community users > > prefers > > the > > >> mail-list, I would recommend we keep the current way > > >> > > >> Tianqi > > >> > > >> On Fri, Jun 15, 2018 at 9:25 PM, Sergio
