chsin opened a new pull request #9809: register optimizers only once URL: https://github.com/apache/incubator-mxnet/pull/9809 ## Description ## Regards issue #9800. The bug was that CPP-Package won't allow more than one optimizer to be created. See comments for more details. ## Checklist ## ### Essentials ### - [x] Passed code style checking (`make lint`) - [x] Changes are complete (i.e. I finished coding on this PR) - [ ] All changes have test coverage: - Unit tests are added for small changes to verify correctness (e.g. adding a new operator) - Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore) - Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL) - [x] Code is well-documented: - [x] To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change ## Comments ## I narrowed it down to this snip of code: ```c++ #include "mxnet-cpp/MxNetCpp.h" int main(int argc, char** argv) { Optimizer* opt = OptimizerRegistry::Find("sgd"); opt = OptimizerRegistry::Find("adam"); MXNotifyShutdown(); return 0; } ``` To produce this error: ```c++ terminate called after throwing an instance of 'dmlc::Error' what(): [18:02:48] /scratch/cahsin/repo/incubator-mxnet/cpp-package/include/mxnet-cpp/optimizer.hpp:141: Check failed: cmap().count(name) == 0 (1 vs. 0) sgd already registered Stack trace returned 7 entries: [bt] (0) ./mlp_cpu(_ZN4dmlc10StackTraceEv+0x42) [0x411b41] [bt] (1) ./mlp_cpu(_ZN4dmlc15LogMessageFatalD1Ev+0x1b) [0x411db1] [bt] (2) ./mlp_cpu() [0x41538f] [bt] (3) ./mlp_cpu() [0x414de8] [bt] (4) ./mlp_cpu() [0x411231] [bt] (5) /lib64/libc.so.6(__libc_start_main+0xfd) [0x3449c1ed1d] [bt] (6) ./mlp_cpu() [0x410ad9] Aborted (core dumped) ``` I am using adam there as an example, but any two calls to OptimizerRegistry::Find will result in the error. The problem is here: https://github.com/apache/incubator-mxnet/blob/master/cpp-package/include/mxnet-cpp/optimizer.hpp#L127 ```c++ inline Optimizer* OptimizerRegistry::Find(const std::string& name) { MXNETCPP_REGISTER_OPTIMIZER(sgd, SGDOptimizer); MXNETCPP_REGISTER_OPTIMIZER(ccsgd, SGDOptimizer); // For backward compatibility MXNETCPP_REGISTER_OPTIMIZER(rmsprop, RMSPropOptimizer); MXNETCPP_REGISTER_OPTIMIZER(adam, AdamOptimizer); MXNETCPP_REGISTER_OPTIMIZER(adagrad, AdaGradOptimizer); MXNETCPP_REGISTER_OPTIMIZER(adadelta, AdaDeltaOptimizer); MXNETCPP_REGISTER_OPTIMIZER(signum, SignumOptimizer); auto it = cmap().find(name); if (it == cmap().end()) return nullptr; return it->second(); } ``` https://github.com/apache/incubator-mxnet/blob/master/cpp-package/include/mxnet-cpp/optimizer.h#L133 ```c++ #define MXNETCPP_REGISTER_OPTIMIZER(Name, OptimizerType)\ OptimizerRegistry::__REGISTER__(#Name, [](){return new OptimizerType();}) ``` https://github.com/apache/incubator-mxnet/blob/master/cpp-package/include/mxnet-cpp/optimizer.hpp#L141 ```c++ inline int OptimizerRegistry::__REGISTER__(const std::string& name, OptimizerCreator creator) { CHECK_EQ(cmap().count(name), 0) << name << " already registered"; cmap().emplace(name, std::move(creator)); return 0; } ``` This means users can only call ```OptimizerRegistry::Find``` once because every time they want to create an optimizer object, ```OptimizerRegistry::Find``` re-registers everything. This is abnormal. If you look into what the Python API does [optimizer.py](https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/optimizer.py#L113), you observe that ```def register(klass)``` isn't called by ```def create_optimizer(name, **kwargs)```. In fact, when the CPP-Package was first added in [Integrate cpp package (#5251)](https://github.com/apache/incubator-mxnet/blob/21b2da07f6ff10669b7649860b913e17e6184708/cpp-package/include/mxnet-cpp/optimizer.hpp), the registering of optimizers was not placed into the creation: ```c++ std::map<std::string, OptimizerCreator> OptimizerRegistry::cmap_; MXNETCPP_REGISTER_OPTIMIZER(sgd, SGDOptimizer); MXNETCPP_REGISTER_OPTIMIZER(ccsgd, SGDOptimizer); // For backward compatibility Optimizer* OptimizerRegistry::Find(const std::string& name) { auto it = cmap_.find(name); if (it == cmap_.end()) return nullptr; return it->second(); } ``` The embedding of register in Find happened with the merge of [[cpp-package] Fix multiple definition issue](https://github.com/apache/incubator-mxnet/pull/5545/files#diff-715b8c623cdceb730bd1f01c9b243d62R67). Since there is a justification for the strange structure, I just put in a small fix that would register the optimizer just once.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
