-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
Could you please add a test for this case? |
@marcoabreu, OK, where are the instructions for adding a unit test to the cpp package? I see that in https://github.com/apache/incubator-mxnet/blob/master/cpp-package/tests/ci_test.sh |
Oh you're right, we don't have a structure to test cpp package properly. My bad... But yes, everything in cpp-package should be compiled. Feel free to play around with it and give me heads up if you need assistance. https://github.com/apache/incubator-mxnet/blob/master/cpp-package/tests/ci_test.sh is the right place to start. |
@larroy could you review? |
@marcoabreu, I don't understand how the Python test would fail when I never made an edit to the Python package? Could you explain? |
@marcoabreu, I never touched the Python and Gluon package, why do I get this:
|
Since we are using randomized data in our unit tests, they tend to be flaky by nature. I have retriggered the build and create an issue for this test #9820 |
@marcoabreu, test was made, checks passes, now what? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small change in the code, otherwise LGTM. Would like to have this by a person familiar with the interface reviewed
|
||
MXNotifyShutdown(); | ||
return ret; | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double return statement
MXNETCPP_REGISTER_OPTIMIZER(adagrad, AdaGradOptimizer); | ||
MXNETCPP_REGISTER_OPTIMIZER(adadelta, AdaDeltaOptimizer); | ||
MXNETCPP_REGISTER_OPTIMIZER(signum, SignumOptimizer); | ||
if (cmap().empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are checking all optimizers at once, but wouldn't it be better to check every optimizer separately? AFAICT Registry::__REGISTER__
is used, but Registry::__REGISTER_OR_GET__
would make more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimizers are all registered at once in the original CPP-Package code and in the Python code using decorators. In the original code #5251, they were all registered at once in the file, so when the program was initialized, all optimizers would already be registered (which is the case in Python), not in the first call to the function Find, but that caused multiple definition errors, so in #5545, the were all registered at once in the first call to Find, which caused the inability to have more than one optimizer bc they would be re-registered with each call, so this change makes sure they are registered once. Why check every optimizer separately when they should all be registered at the same time and only once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's strange for a look-up function to register optimizers. Would it make more sense to have a macro to handle the multiple-definition problem for built-in optimizers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not exactly sure what you mean, but it sounds like you want to revert to the original code, but that would break #5545. @sifmelcara why in #5545 did you
"3. In optimizer.hpp, I delayed the registration of sgd optimizer to the first call of OptimizerRegistry::Find."?
@ marcoabreu, made the change, now what? |
Let's await the review
Am 20.02.2018 5:07 nachm. schrieb "chsin" <notifications@github.com>:
… @ marcoabreu, made the change, now what?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9809 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ARxB66L3bw5Oq-7sWYG0LMN6FAU8lb5Aks5tWu2ugaJpZM4SIhtS>
.
|
@marcoabreu, I don't know who you're waiting for, so I'll ask the CPP-Package creator: @lx75249, Xin Li, you wrote the original CPP package, are you OK with this change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved in terms of CI
I'm not familiar with the CPP package so I can't really review this part. |
Hello! Just got a ping from @chsin for this PR. I'm the author of the ODR fix. However, as @szha mentioned, I wonder if there is a better way to register the optimizers? I guess we can use global inline variable but that would require C++17 |
Hmm, in fact I think adding back |
But we'll still have the weirdness of initializing with Find that @szha is unhappy about.. |
Unfortunately we don't have static constructors in c++, and that's why the initialization becomes so weird. Emulating a static constructor will make the code more confusing, but registering optimizers in Find is as acceptable as creating an instance in singleton getters. |
This is at least better than the current solution. So merged |
* opt register only once * lint pass * opt register only once lint pass * add test_optimizer.cpp * fix warning * typo
* opt register only once * lint pass * opt register only once lint pass * add test_optimizer.cpp * fix warning * typo
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
make lint
)Comments
I narrowed my example code down to this snip of code:
To produce this error:
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
https://github.com/apache/incubator-mxnet/blob/master/cpp-package/include/mxnet-cpp/optimizer.h#L133
https://github.com/apache/incubator-mxnet/blob/master/cpp-package/include/mxnet-cpp/optimizer.hpp#L141
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, you observe thatdef register(klass)
isn't called bydef create_optimizer(name, **kwargs)
. In fact, when the CPP-Package was first added in Integrate cpp package (#5251), the registering of optimizers was not placed into the creation:The embedding of register in Find happened with the merge of [cpp-package] Fix multiple definition issue. Since there is a justification for the strange structure, I just put in a small fix that would register the optimizer just once.