mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Barber, Christopher" <Christopher.Bar...@analog.com>
Subject Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)
Date Tue, 16 Jan 2018 16:21:33 GMT
I can see an argument for treating warnings as errors during manual builds (if the developer
so wishes), but for automated builds I would rather get more information. I also wouldn’t
kill a build just because a test fails either because there may be useful information from
other tests. Why should a bug in one new operator prevent testing everything else? Note that
there is no reason you cannot set up a system to inform developers as soon as a failure is
seen in the build without actually stopping the build.

The question here is whether a warning failure is likely to lead to actual bad behavior in
the rest of the build. On projects on which I have worked, most warnings do not result in
such errors.

On 1/16/18, 10:48 AM, "Marco de Abreu" <marco.g.abreu@googlemail.com> wrote:

    So you're proposing to have a stage AFTER test execution which would report
    warnings as errors? While this is a good idea, I'm afraid that a fail-fast
    would also have its benefits - especially considering that compilation only
    takes a few minutes and consumes few resources while test execution takes
    up most of the time and is very costly.
    
    -Marco
    
    On Tue, Jan 16, 2018 at 4:11 PM, Barber, Christopher <
    Christopher.Barber@analog.com> wrote:
    
    > Personally, I don’t like treating warnings as errors because it prevents
    > compilation from completing and causes you to lose any ability to test the
    > code and get any other information. Killing the build because of a failed
    > warning for something that might not matter means that you may not find out
    > about other important test failures until much later. Better to add a test
    > that grovels the build logs for warning messages and treat it as a test
    > failure.
    >
    > I also prefer to only enable exactly those warnings that truly matter.
    >
    > On 1/16/18, 8:23 AM, "Marco de Abreu" <marco.g.abreu@googlemail.com>
    > wrote:
    >
    >     I'd vote for having warnings as errors only for CI but not in general
    >     builds which are getting executed by users on their local machine.
    > Just in
    >     case CI misses a warning due to a different version, this could block a
    >     developer from compiling MXNet locally even though it might just be a
    >     warning which is not critical enough (otherwise it would be an error)
    > to
    >     justify blocking the compilation. In my opinion, it would be good if
    > we can
    >     filter most warnings during PR-stage and risk that some are getting
    > into
    >     the master branch due to a different compiler version. A reduction of
    > (for
    >     example) 95% without risking to break the master branch on different
    >     compilers is way better in my opinion than having a 100% coverage which
    >     could block compilation - especially because we would only notice if a
    > user
    >     tells us afterwards.
    >
    >     -Marco
    >
    >     On Tue, Jan 16, 2018 at 1:32 PM, Pedro Larroy <
    > pedro.larroy.lists@gmail.com>
    >     wrote:
    >
    >     > Hi Chris
    >     >
    >     > I get the rationale of the point you raise, but In my opinion, and
    >     > considering the complexity of C++ and the potential for difficult and
    >     > expensive to track bugs, I think this should be enabled by default
    > for
    >     > both release and debug. A developer is free to disable warnings in
    > his
    >     > own private branch, but I don't see what would be the benefit of
    > this.
    >     >
    >     > Regarding your second point, I think this is a minor issue which is
    >     > outweighed by the benefits. In the case you propose, the author of a
    >     > PR can easily fix a bunch of warnings when CI fails as usual. For
    >     > example in case he gets one or two warnings that his version of the
    >     > compiler didn't catch, or if she has an additional warning of some
    >     > type with a different version of GCC / Clang.
    >     >
    >     > This has the objective to prevent warning inflation. In practice, a
    >     > different version of GCC might produce just a couple of new warning
    >     > types that will be easily fixable once we upgrade the compiler in CI.
    >     > We also get the benefit of preventing warnings on the gcc versions
    >     > that the author is using, in the case he has a different one. Another
    >     > option is to enable warnings as errors only on CI. I would prefer to
    >     > have it enabled by default, for correctness. As first time users are
    >     > not likely to compile MXNet by themselves, and also considering the
    >     > significant complexity of compiling MXNet from scratch for newcomers.
    >     >
    >     > In general, the compilers that we have running on CI should be our
    >     > reference compilers. And for practical purposes, having no warnings
    > in
    >     > those versions of Clang and GCC would be a positive step towards more
    >     > code quality, clean compilation and a more mantainable code base.
    >     > Once we have CI stable we can build a matrix of supported compilers
    > in
    >     > the docs, as for example there are versions of GCC which are not
    >     > supported by the nvidia tools.
    >     >
    >     > Pedro.
    >     >
    >     >
    >     >
    >     >
    >     > On Mon, Jan 15, 2018 at 7:27 PM, Chris Olivier <
    > cjolivier01@gmail.com>
    >     > wrote:
    >     > > If enabled, it should only cause errors in Release builds, since
    > having
    >     > > warnings in WIP code is not unusual.
    >     > >
    >     > > In addition, different developers use different gcc/clang
    > versions. Some
    >     > > gcc versions, for instance, generate warnings where others do
    > not.  It
    >     > > would not be fair to render unbuildable a developer who is using a
    > newer
    >     > > (or older) gcc version is different from CI.  Can this argument be
    > tied
    >     > to
    >     > > a particular compiler/platform/version?
    >     > >
    >     > > On Mon, Jan 15, 2018 at 9:43 AM, Marco de Abreu <
    >     > > marco.g.abreu@googlemail.com> wrote:
    >     > >
    >     > >> +1
    >     > >>
    >     > >> On Mon, Jan 15, 2018 at 6:27 PM, Pedro Larroy <
    >     > >> pedro.larroy.lists@gmail.com>
    >     > >> wrote:
    >     > >>
    >     > >> > Hi
    >     > >> >
    >     > >> > I would like to propose to compile in CI with warnings as
    > errors for
    >     > >> > increased code quality. This has a dual purpose:
    >     > >> >
    >     > >> > 1. Enforce a clean compilation output. Warnings often indicate
    >     > >> > deficiencies in the code and hide new warnings which can be
an
    >     > >> > indicator of problems.
    >     > >> >
    >     > >> > 2. Warnings can surface bugs as has happened before.
    >     > >> >
    >     > >> > While this might be impractical in all architectures, I would
    > propose
    >     > >> > having the Linux and Clang build run without warnings in CI.
    >     > >> >
    >     > >> > I think we are very close to this as I personally have been
    > fixing
    >     > >> > warnings in Linux and OSX / Clang.
    >     > >> >
    >     > >> > References:
    >     > >> >
    >     > >> > https://github.com/apache/incubator-mxnet/pull/9398
    >     > >> >
    >     > >> > http://jenkins.mxnet-ci.amazon-ml.com/blue/
    > organizations/jenkins/
    >     > >> > incubator-mxnet/detail/PR-9398/1/pipeline
    >     > >> >
    >     > >> > Pedro.
    >     > >> >
    >     > >>
    >     >
    >
    >
    >
    

Mime
View raw message