mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marco de Abreu <marco.g.ab...@googlemail.com.INVALID>
Subject Re: [LAZY VOTE] Test coverage of PRs
Date Thu, 21 Jun 2018 14:43:54 GMT
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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message