Tianqi and Kellen, do you mind if we move ahead and merge https://github.com/apache/incubator-mxnet/pull/11344 now?
-Marco On Thu, Jun 21, 2018 at 4:50 PM Yasser Zamani <[email protected]> wrote: > +1 but just as an MXNet lover newbie :) > > We at Apache Struts, already use "coveralls" and I personally like it a > lot even with some limitations with it. It doesn't allow us to merge a > PR with a not tested code. It reports us where are those not covered new > codes. > > Regards. > > On 6/21/2018 7:13 PM, Marco de Abreu wrote: > > To avoid any confusion, I have disable the PR comments at > > > https://github.com/apache/incubator-mxnet/pull/11344/files#diff-1f7b4b85ed8525c5239f741431a72872R25 > . > > Thus, the data will only be recorded in the background is then > retrievable > > at https://codecov.io/gh/apache/incubator-mxnet/pulls. > > > > -Marco > > > > On Thu, Jun 21, 2018 at 6:43 AM Marco de Abreu < > [email protected]> > > wrote: > > > >> Hi Kellen, > >> > >> Thanks for your feedback. > >> > >> The same argument about a hosted service could be applied to GitHub - we > >> could just use the git repository hosted under Apache Infra then and > >> completely stick to JIRA. This service gives us the advantage of being > free > >> of charge, a mature project and having built-in support for all > languages > >> we run in this project. If you got an open source software which offers > the > >> same features and is as easy to manage, I'm happy to take suggestions. > >> > >> As stated previously, we're going to start with Python and C++ coverage, > >> evaluate it over the following days/weeks and then discuss how we should > >> move forward. I'm happy to take contributions from the community to > extend > >> the coverage across more languages. For now, I'd just like to gather > some > >> data. > >> > >> Best regards, > >> Marco > >> > >> > >> kellen sunderland <[email protected]> schrieb am Do., 21. > Juni > >> 2018, 11:15: > >> > >>> -0.1 (non-binding). > >>> > >>> Looks good, and I like the idea of adding coverage, however I have a > few > >>> minor issues with this approach: > >>> > >>> First it seems to use a hosted service, which can disappear / be > acquired > >>> / > >>> change terms at any time. In general I would try and avoid using > services > >>> like this for Apache projects. I don't feel as strongly about this as > >>> some > >>> in the community do, but there are so many open source alternatives to > >>> generate coverage reports that I'm not sure why we have to go the Saas > >>> route in this case. > >>> > >>> Secondly the core library is written in C++. Most of the significant > bugs > >>> I've seen in MXNet so far have been in the C++ code. I'd suggest we > focus > >>> on coverage reports for C++ before, or at least in addition to > Python. I > >>> feel less strongly about the other languages, but coverage in other > areas > >>> (i.e. Scala) might also be nice eventually. > >>> > >>> -Kellen > >>> > >>> On Thu, Jun 21, 2018 at 4:59 AM Chris Olivier <[email protected]> > >>> wrote: > >>> > >>>> we should have a betting pool on what the current coverage is. > >>>> > >>>> On Wed, Jun 20, 2018 at 7:54 PM Naveen Swamy <[email protected]> > >>> wrote: > >>>> > >>>>> +1 to collect data on coverage. > >>>>> > >>>>> On Wed, Jun 20, 2018 at 7:38 PM, Marco de Abreu < > >>>>> [email protected]> 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 < > >>> [email protected]> > >>>>>> 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 < > >>>>>>> [email protected]> 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 > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > >
