Caffe without the patch, cpp-package fixed also with Caffe plugin #5573

cjolivier01 · 2017-03-26T02:53:31Z

No description provided.

…dded.

cjolivier01 · 2017-03-26T02:55:41Z

cpp-package/example/lenet.cpp

@@ -35,19 +35,19 @@ class Lenet {
        Convolution("conv1", data, conv1_w, conv1_b, Shape(5, 5), 20);
    Symbol tanh1 = Activation("tanh1", conv1, ActivationActType::tanh);
    Symbol pool1 = Pooling("pool1", tanh1, Shape(2, 2), PoolingPoolType::max,
-      false, PoolingPoolingConvention::valid, Shape(2, 2));
+      false, false, PoolingPoolingConvention::valid, Shape(2, 2));


What should the default here be? I assume it's false like the default in the declaration.
parameter is: bool cudnn_off = false

cjolivier01 · 2017-03-26T02:56:03Z

cpp-package/example/resnet.cpp

@@ -43,7 +46,7 @@ Symbol getConv(const std::string & name, Symbol data,
                                  kernel, num_filter, stride, Shape(1, 1),
                                  pad, 1, 512);

-  Symbol bn = BatchNorm(name + "_bn", conv, 2e-5, bn_momentum, false);
+  Symbol bn = BatchNorm(name + "_bn", conv, BN_GAMMA, BN_BETA, 2e-5, bn_momentum, false);


What is the desired default gamma and beta? These weren't in the older versions.

inline Symbol BatchNorm(const std::string& symbol_name,
Symbol data,
Symbol gamma,
Symbol beta,
mx_float eps = 0.001,
mx_float momentum = 0.9,
bool fix_gamma = true,
bool use_global_stats = false,
bool output_mean_var = false) {

Gamma and beta are learnable parameters that can be initialized automatically by initializer, but op.h genereator can't distinguish parameters from hyperparameters currently, as mxnet doesn't provide such information. In older versions, gamma and beta were not declared explicitly. We can use Operator("BatchNorm") here to avoid specifying gamma and beta before figuring out how to detect parameters.

cjolivier01 · 2017-03-26T03:00:38Z

src/executor/graph_executor.cc

@@ -905,8 +905,6 @@ GraphExecutor::CachedSegOpr GraphExecutor::CreateCachedSegOpr(size_t topo_start,
    // you need to copy it out. (potential memory leak risk)
    char *p_opr_name = new char[opr_names.size() + 1];
    memcpy(p_opr_name, opr_names.c_str(), opr_names.size() + 1);
-#else


This isn't necessary and generates a warning

cjolivier01 · 2017-03-26T03:04:52Z

Fixing lint...

cjolivier01 · 2017-03-26T04:34:28Z

travis appears to be broken (nohup)

cjolivier01 · 2017-03-26T04:35:36Z

cpp-package/include/mxnet-cpp/kvstore.hpp

@@ -14,8 +14,8 @@
 #include "mxnet-cpp/kvstore.h"
 #include "mxnet-cpp/optimizer.h"

-#ifndef KVSTORE_HPP
-#define KVSTORE_HPP
+#ifndef CPP_PACKAGE_INCLUDE_MXNET_CPP_KVSTORE_HPP_


These are lint fixes which captured problems in the original cpp-package headers

you can make lint ignore CPP_PACKAGE_INCLUDE, it's done for r-package for example

Ok. I went ahead and fixed it so that we could have some clean, homogenous code.

cjolivier01 · 2017-03-26T04:37:23Z

cpp-package/example/charRNN.cpp

@@ -199,7 +199,7 @@ class BucketSentenceIter : public DataIter {
 public:
  BucketSentenceIter(string filename, int minibatch, Context context) : batch(minibatch),
  current(-1), device(context) {
-    auto& content = readContent(filename);
+    auto content = readContent(filename);


This was incorrectly taking a reference of a temporary

cjolivier01 · 2017-03-26T04:37:50Z

cpp-package/example/charRNN.cpp

@@ -343,7 +343,8 @@ class BucketSentenceIter : public DataIter {
        chars.push_back(c);
      }
    }
-    return {map, chars};
+    // Note: Can't use {} because this would hit the explicit constructor
+    return tuple<unordered_map<wchar_t, mx_float>, vector<wchar_t>>(map, chars);


My gcc-5.4 won't allow this

piiswrong · 2017-03-26T06:42:32Z

Makefile

@@ -133,7 +138,7 @@ endif
 .PHONY: clean all test lint doc clean_all rcpplint rcppexport roxygen\
 	cython2 cython3 cython cyclean

-all: lib/libmxnet.a lib/libmxnet.so $(BIN)
+all: lib/libmxnet.a lib/libmxnet.so $(BIN) cpp-package-example-all


shouldn't build cpp-package by default

It only generates a header file for cpp-package, and is necessary since updates that break cpp-package will be reflected in the test..

are cpp-package tests run during CI builds? I think they they probably aren't because they didn't build at all. In the meantime, how will we keep people from "breaking" these compiliables again?

How about we just build lenet by default? At any rate, the build for all of them is quick -- a few seconds.

Or you mean a config.mk flag and w can turn it on for the CI build? That might be ok.

If someone changes operator api, cpp-package test will fail since op.h changes after building cpp-package.

piiswrong · 2017-03-26T06:47:57Z

@lx75249

piiswrong · 2017-03-26T07:28:41Z

I think we just need to put add arguments for beta and gamma Xin Li <notifications@github.com>于2017年3月26日周日上午12:15写道：

…

***@***.**** commented on this pull request. ------------------------------ In cpp-package/example/resnet.cpp <#5573 (comment)>: > @@ -43,7 +46,7 @@ Symbol getConv(const std::string & name, Symbol data, kernel, num_filter, stride, Shape(1, 1), pad, 1, 512); - Symbol bn = BatchNorm(name + "_bn", conv, 2e-5, bn_momentum, false); + Symbol bn = BatchNorm(name + "_bn", conv, BN_GAMMA, BN_BETA, 2e-5, bn_momentum, false); Gamma and beta are learnable parameters that can be initialized automatically by initializer, but op.h genereator can't distinguish parameters from hyperparameters currently, as mxnet doesn't provide such information. In older versions, gamma and beta were not declared explicitly. We can use Operator("BatchNorm") here to avoid specifying gamma and beta before figuring out how to detect parameters. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#5573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAiudJkFhwdavJJa16Kgiu1cHte1UYhIks5rphCrgaJpZM4MpS_g> .

conopt · 2017-03-26T07:49:58Z

cpp-package/include/mxnet-cpp/.gitignore

@@ -0,0 +1,2 @@
+# Rebuildable file(s)
+op.h


This will force everyone to build the whole library..

I am a little confused. The expectation is that someone will check out the mxnet source tree and include directly from mxnet/cpp-package/include, but dynamically load the libmxnet.so that came with their pip install rather than one that they built locally? That seems unlikely and dangerous, correct? I expect development on mxnet to accelerate in the near future and a widely-distributed libmxnet.so to not be in sync for long (as well as inconsistencies with caffe plugin, etc.).
The cmake build "distributes" mxnet headers during a build if INSTALL_INCLUDE_DIR is defined as a path, although not yet for cpp-package. If we're going to make a distribution like apt-get install mxnet-dev, I would expect that we can deploy the built op.h

wdyt?

yes you're right...

piiswrong · 2017-03-29T04:38:29Z

@lx75249 @cjolivier01 Is this good for merging?

cjolivier01 · 2017-03-29T19:45:11Z

Fixing build things. Also, need to do Operator("BatchNorm")

… build machine)

cjolivier01 · 2017-03-30T16:40:07Z

other checkin of ft.cc contrib operator broke the build on Windows, libcufft library not linked. Added to cmake.

…ugin (#5573)" This reverts commit a5abd79.

* Revert "Caffe without the patch, cpp-package fixed also with Caffe plugin (#5573)" This reverts commit a5abd79. * Revert "Revert "contrib fft,ifft,countsketch (#5563)" (#5634)" This reverts commit 7f688f7.

…ache#5573) * PEP8 indentation fix * Remove executable flag * Remove executable flags * Fixing cpp-package including problems with caffe converter * Fix warnings * Remove need for caffe patch for caffe plugin * Ignore cmake paths, remove rebuildable op.h for cpp-package * cpp-package examples fixed. Makefile and CMake of op.h and examples added. * cpp-package examples fixed. Makefile and CMake of op.h and examples added. * Fix source file * Better example.mk * Turn off caffe by default * lint fixes * Trying to figure out how to fix submodules * nohub problem on travis so force retry * Edited test to just run make instead of obsolete make example * Fix cpp-package op.h bootstrapping with CMake * Build tweaking cpp-package * Lint fixes * op.h generator * FIx cpp-package for latest merge from master * Fix lint * static link * link whole static lib * Trigger another build attempt * Trigger another build attempt * Add caffe plugin support (disabled until dependencies can be added to build machine) * Add cufft library * win32 cufft library * don't link unit tests as static * Add include for rebuildable protobuf header caffe.pb.h

* Revert "Caffe without the patch, cpp-package fixed also with Caffe plugin (apache#5573)" This reverts commit a5abd79. * Revert "Revert "contrib fft,ifft,countsketch (apache#5563)" (apache#5634)" This reverts commit 7f688f7.

Olivier and others added 12 commits March 23, 2017 12:53

PEP8 indentation fix

d5f2439

Remove executable flag

d8cdb85

Remove executable flags

1a1cea5

Fixing cpp-package including problems with caffe converter

0261889

Fix warnings

1a69803

Remove need for caffe patch for caffe plugin

7e5e539

Ignore cmake paths, remove rebuildable op.h for cpp-package

208e912

cpp-package examples fixed. Makefile and CMake of op.h and examples a…

d4aa582

…dded.

cpp-package examples fixed. Makefile and CMake of op.h and examples a…

84f1d4c

…dded.

Fix source file

8216db5

Better example.mk

1e2e157

Turn off caffe by default

36ccab2

cjolivier01 commented Mar 26, 2017

View reviewed changes

cjolivier01 added 5 commits March 25, 2017 21:11

lint fixes

6f65a27

Trying to figure out how to fix submodules

84aeb15

Merge branch 'master' into caffefix

b57245e

nohub problem on travis so force retry

a408279

Edited test to just run make instead of obsolete make example

38d9770

cjolivier01 closed this Mar 26, 2017

cjolivier01 reopened this Mar 26, 2017

cjolivier01 commented Mar 26, 2017

View reviewed changes

piiswrong reviewed Mar 26, 2017

View reviewed changes

conopt reviewed Mar 26, 2017

View reviewed changes

cjolivier01 added 2 commits March 26, 2017 20:41

Fix cpp-package op.h bootstrapping with CMake

8361297

Merge branch 'master' into caffefix

9d519f9

Olivier added 2 commits March 29, 2017 12:40

Build tweaking cpp-package

dde8f33

Merge in from master

214b1eb

Olivier and others added 11 commits March 29, 2017 12:53

Lint fixes

b72af94

op.h generator

33a6231

FIx cpp-package for latest merge from master

8b9a929

Fix lint

04e3baf

static link

5918bd1

link whole static lib

6b0876e

Merge branch 'master' into caffefix

001175c

Trigger another build attempt

ec4f1ab

Trigger another build attempt

b89397f

Add caffe plugin support (disabled until dependencies can be added to…

a7f6766

… build machine)

Add cufft library

fd8152d

Olivier and others added 4 commits March 30, 2017 11:10

win32 cufft library

5bbee1e

don't link unit tests as static

2b42e03

Add include for rebuildable protobuf header caffe.pb.h

e4987aa

Merge branch 'master' into caffefix

c21e859

piiswrong merged commit a5abd79 into apache:master Mar 31, 2017

piiswrong added a commit that referenced this pull request Mar 31, 2017

Revert "Caffe without the patch, cpp-package fixed also with Caffe pl…

76a80de

…ugin (#5573)" This reverts commit a5abd79.

szha mentioned this pull request Jun 26, 2017

can't find op.h when I try to use example in cpp-package #6794

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Caffe without the patch, cpp-package fixed also with Caffe plugin #5573

Caffe without the patch, cpp-package fixed also with Caffe plugin #5573

cjolivier01 commented Mar 26, 2017

cjolivier01 Mar 26, 2017 •

edited

Loading

cjolivier01 Mar 26, 2017 •

edited

Loading

conopt Mar 26, 2017

cjolivier01 Mar 26, 2017

cjolivier01 commented Mar 26, 2017

cjolivier01 commented Mar 26, 2017

cjolivier01 Mar 26, 2017

piiswrong Mar 26, 2017 •

edited

Loading

cjolivier01 Mar 27, 2017

cjolivier01 Mar 26, 2017

cjolivier01 Mar 26, 2017

piiswrong Mar 26, 2017

conopt Mar 26, 2017

cjolivier01 Mar 27, 2017

cjolivier01 Mar 27, 2017

cjolivier01 Mar 27, 2017

conopt Mar 27, 2017

piiswrong commented Mar 26, 2017

piiswrong commented Mar 26, 2017 via email

conopt Mar 26, 2017

cjolivier01 Mar 27, 2017 •

edited

Loading

conopt Mar 27, 2017

piiswrong commented Mar 29, 2017

cjolivier01 commented Mar 29, 2017

cjolivier01 commented Mar 30, 2017

Caffe without the patch, cpp-package fixed also with Caffe plugin #5573

Caffe without the patch, cpp-package fixed also with Caffe plugin #5573

Conversation

cjolivier01 commented Mar 26, 2017

cjolivier01 Mar 26, 2017 • edited Loading

Choose a reason for hiding this comment

cjolivier01 Mar 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjolivier01 commented Mar 26, 2017

cjolivier01 commented Mar 26, 2017

Choose a reason for hiding this comment

piiswrong Mar 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piiswrong commented Mar 26, 2017

piiswrong commented Mar 26, 2017 via email

Choose a reason for hiding this comment

cjolivier01 Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piiswrong commented Mar 29, 2017

cjolivier01 commented Mar 29, 2017

cjolivier01 commented Mar 30, 2017

cjolivier01 Mar 26, 2017 •

edited

Loading

cjolivier01 Mar 26, 2017 •

edited

Loading

piiswrong Mar 26, 2017 •

edited

Loading

cjolivier01 Mar 27, 2017 •

edited

Loading