Re: build from source instructions

[Steffen Rochel]

Aaron - we should keep instructions how to build from source. Updating and
re-organizing makes sense to me.
Steffen

On Mon, Aug 27, 2018 at 4:54 PM Aaron Markham 
wrote:

> Hello,
> I was looking into the C++ instructions and came across this seemingly
> pretty old page:
> https://mxnet.incubator.apache.org/install/build_from_source
>
> I think it has several inaccuracies as different/updated installation info
> has been added to different pages.
>
> Should it be deleted?
>
> Or should a specific build from source page be maintained (moving/copying
> info from the other more recently updated pages)?
>
> I'm really thinking that it would be easier to maintain if each OS had its
> own page, Python/pip info had its own page, then bindings had their own
> pages.
>
> Other suggestions?
>
> Cheers,
> Aaron
>

build from source instructions

[Aaron Markham]

Hello,
I was looking into the C++ instructions and came across this seemingly
pretty old page:
https://mxnet.incubator.apache.org/install/build_from_source

I think it has several inaccuracies as different/updated installation info
has been added to different pages.

Should it be deleted?

Or should a specific build from source page be maintained (moving/copying
info from the other more recently updated pages)?

I'm really thinking that it would be easier to maintain if each OS had its
own page, Python/pip info had its own page, then bindings had their own
pages.

Other suggestions?

Cheers,
Aaron

Re: Removing misleading metric log

[Naveen Swamy]

Can you not just print the batch range[10-20] along with epoch, there might
be user scripts that depend on these values(we won't know who is using) , I
don't think we should drop this log.

On Mon, Aug 27, 2018 at 9:38 AM Vandana Kannan  wrote:

> Hi All,
>
> The log self.logger.info('Epoch[%d] Train-%s=%f', epoch, name, val) in
> python/mxnet/module/base_module.py gives the user the impression that the
> metric printed is for the entire epoch, which is misleading (Ref
> https://github.com/apache/incubator-mxnet/pull/10437). This log was
> maintained so that scripts that look for this text, are not broken. But
> this is continuing to cause confusion for users.
>
> https://github.com/apache/incubator-mxnet/pull/12182 removes this
> particular log. The scripts that parse this output within incubator-mxnet
> have been fixed, but there is a risk that external scripts may break due to
> this change.
>
> Are there any known scripts that look for this log? Any other thoughts
> about this PR?
>
> Thanks,
> Vandana
>
>

Removing misleading metric log

[Vandana Kannan]

Hi All,

The log self.logger.info('Epoch[%d] Train-%s=%f', epoch, name, val) in 
python/mxnet/module/base_module.py gives the user the impression that the 
metric printed is for the entire epoch, which is misleading (Ref 
https://github.com/apache/incubator-mxnet/pull/10437). This log was maintained 
so that scripts that look for this text, are not broken. But this is continuing 
to cause confusion for users.

https://github.com/apache/incubator-mxnet/pull/12182 removes this particular 
log. The scripts that parse this output within incubator-mxnet have been fixed, 
but there is a risk that external scripts may break due to this change.

Are there any known scripts that look for this log? Any other thoughts about 
this PR?

Thanks,
Vandana

Re: clang-tidy and static code analysis

[kellen sunderland]

Great suggestion Philip, and thanks for the reference.  I've run a few
sanitizers locally but didn't see any output that concerned me. Absolutely
on my todo list to get them integrated into CI (unless someone beats me to
it).

On Sun, Aug 26, 2018 at 1:54 AM Marco de Abreu
 wrote:

> Great idea, thanks a lot Kellen. I had a look at the results and I really
> like that they are clearly actionable and reveal issues that somebody
> wouldn't have caught on the first sight.
>
> My personal favorite is the branch anomaly detection which finds execution
> paths that would result in a function returning garbage data if a certain
> path is being taken. I'm looking forward to more results!
>
> Best regards,
> Marco
>
> Philip Cho  schrieb am Sa., 25. Aug. 2018,
> 21:48:
>
> > +1 as well.
> >
> > As a related idea, let's also consider adding sanitizer tests, which
> > detects leaks and memory errors at runtime. Overhead is a lot lower than
> > other methods such as valgrind.
> > For an example, see the sanitizer tests in XGBoost:
> > https://github.com/dmlc/xgboost/pull/3557
> > https://github.com/dmlc/xgboost/pull/3525
> >
> > Hyunsu Cho.
> >
> > On Sat, Aug 25, 2018, 11:47 AM Hagay Lupesko  wrote:
> >
> > > I really like this proposal.
> > > It will help improve the quality of MXNet native code, and maintain a
> > > uniform high bar.
> > > An extra 5 mins of build time seems reasonable.
> > >
> > > +1
> > >
> > >
> > > On Sat, Aug 25, 2018, 07:02 kellen sunderland <
> > kellen.sunderl...@gmail.com
> > > >
> > > wrote:
> > >
> > > > Hello all,
> > > >
> > > > Inspired by Vanadana, cclaus and the project members who setup the
> very
> > > > solid linting tools already in place for MXNet, I'm propose we enable
> > > > clang-tidy-6.0 in our CI (PR here:
> > > > https://github.com/apache/incubator-mxnet/pull/12282).  clang-tidy
> is
> > > > getting to be quite a high-quality, free, easy-to-use static analysis
> > > tool
> > > > for modern C++.  In my opinion it's a very useful extension of
> clang's
> > > > already great code warning system.  Adding it to MXNet might help us
> > > catch
> > > > a few errors (memory leaks, use-after-frees, etc.) and it could help
> us
> > > > keep our coding standards uniform between contributors.  It should
> also
> > > > help automate some of the work that is currently required of the PR
> > > > reviewers.
> > > >
> > > > I'd suggest we initially enabled clang-tidy as a mostly informational
> > CI
> > > > build step that will give many warnings, as in this sample run:
> > > >
> > > >
> > >
> >
> http://jenkins.mxnet-ci.amazon-ml.com/blue/rest/organizations/jenkins/pipelines/incubator-mxnet/branches/PR-12282/runs/20/nodes/102/log/?start=0
> > > > (warnings at bottom of the build).  We'll be able to optionally fail
> > the
> > > > build if certain rules are violated.  There's a complete list of
> rules
> > > > here:
> > > >
> > > >
> > >
> >
> https://releases.llvm.org/6.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/list.html
> > > > .
> > > > If anyone has a controversial rule they'd like to see enforced, feel
> > free
> > > > to nominate rule in this thread.  Non-controversial (use your best
> > > > judgement) rules can be enabled with a workflow similar to what we
> > > already
> > > > do with pylint.  Choose a rule, make changes to the codebase such
> that
> > > that
> > > > rule will pass and add the rule to the .clang-tidy configuration file
> > in
> > > > the root of the repo.  The current formatting of the file should make
> > it
> > > > obvious which rules are enabled as warnings/failures.
> > > >
> > > > I'd estimate once the PR is merged the build times for this task will
> > be
> > > > pretty nominal (4-5 minutes).  Since the task is run in parallel, it
> > > should
> > > > have no impact on the PR total verification times.  I also think that
> > > > introducing a tool like this right after a release has been cut will
> be
> > > > convenient for developers.  It's a good time to introduce large fixup
> > > > changes, and it will give us lot of time to find any potential side
> > > effects
> > > > of modernization refactors.
> > > >
> > > > What does everyone think?  Does it make sense to introduce clang-tidy
> > and
> > > > gradually address or enforce rules as with cpplint / pylint / flake8?
> > > >
> > > > -Kellen
> > > >
> > >
> >
>