mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kellen sunderland <kellen.sunderl...@gmail.com>
Subject Re: [LAZY VOTE] Test coverage of PRs
Date Thu, 21 Jun 2018 09:14:53 GMT
-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