mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "McCollum, Cliff" <mccol...@amazon.co.uk>
Subject Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)
Date Tue, 16 Jan 2018 16:52:46 GMT
While you can debate the "broken windows" theory (https://en.wikipedia.org/wiki/Broken_windows_theory)
I think it has relevance to code warnings: the more warnings you tolerate, the more likely
you are to end up with other undesirable things in your code. My preference has always been
to treat warnings as errors in any code that leaves a developer's own machine. If a team/project
doesn't do this, it is relying on the good intentions of people to prevent code warnings from
multiplying to the point that they become effectively useless.

I have mixed feelings about breaking a build in these cases (you can't deny that it will work
to reduce warnings) but I would fully support any decision that chose zero-warnings as a goal
and that refused to accept new code commits if they increased the warning count.

CM

--
Cliff McCollum <mccollum@amazon.co.uk<mailto:mccollum@amazon.co.uk>>
Software Development Manager, Core ML, Amazon Cambridge




On 16 Jan 2018, at 16:30, Pedro Larroy <pedro.larroy.lists@gmail.com<mailto:pedro.larroy.lists@gmail.com>>
wrote:

I understand. What prevents you from disabling the -Werror flag on
your build configuration?  I don't see where's the big issue. We have
already tens of flags to configure the build anyway.

I have been fixing every warning that I came across so far in the main
platforms that I have access. I consider it a practice of leaving
things cleaner than you find them, and would appreciate if everyone
else would be a good citizen and do the same. Having to explicitly
disable the flag for special cases like the one you mentionwould serve
this purpose.

Anway, I would be satisfied with at least having warnings as errors on CI.

Regarding my development experience, I just try to make bona fide
recommendations given that I worked (survived?) in C++ codebases with
hundreds of developers that run successfully in the order of billions
of devices. I'm sure you have your own bag of tricks. Please excuse me
if my comments came across as questioning your development experience
at any time. I have high regards for your development experience in
any case.









On Tue, Jan 16, 2018 at 4:55 PM, Chris Olivier <cjolivier01@gmail.com<mailto:cjolivier01@gmail.com>>
wrote:
Pedro,

i don’t know if you’ve ever done much development or not, but during
development, it’s quite common to comment out arbitrary lines of code,
create a variable only for debug inspection, or other things that will
generate warnings, but are actually intentional.  causing a compile error i
this case would not be acceptable, in my opinion.

as for the any compiler issue, if someone is using a newer gcc or clang,
and while it only has 2 new warning, they appear in 200 places, are you
saying it’s the responsibility of this poor community developer to fix all
of those warnings? or they can open up a JIRA to your team and you will fix
them?


On Tue, Jan 16, 2018 at 7:48 AM Marco de Abreu <marco.g.abreu@googlemail.com<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto:marco.g.abreu@googlemail.com>> wrote:

+1

On Mon, Jan 15, 2018 at 6:27 PM, Pedro Larroy <
pedro.larroy.lists@gmail.com<mailto: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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message