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 Fri, 22 Jun 2018 05:49:09 GMT
I've got no objections.  Appreciate you adding coverage support.

On Thu, Jun 21, 2018, 10:06 PM Marco de Abreu
<marco.g.abreu@googlemail.com.invalid> wrote:

> 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 <yasserzamani@apache.org>
> 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 <
> > 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