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 20:05:55 GMT
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