mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yasser Zamani <yasserzam...@apache.org>
Subject Re: [LAZY VOTE] Test coverage of PRs
Date Thu, 21 Jun 2018 14:50:30 GMT
+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 <marco.g.abreu@googlemail.com>
> 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 <kellen.sunderland@gmail.com> 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 <cjolivier01@gmail.com>
>>> wrote:
>>>
>>>> we should have a betting pool on what the current coverage is.
>>>>
>>>> On Wed, Jun 20, 2018 at 7:54 PM Naveen Swamy <mnnaveen@gmail.com>
>>> wrote:
>>>>
>>>>> +1 to collect data on coverage.
>>>>>
>>>>> On Wed, Jun 20, 2018 at 7:38 PM, Marco de Abreu <
>>>>> marco.g.abreu@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 <
>>> tqchen@cs.washington.edu>
>>>>>> 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.abreu@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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 
Mime
View raw message