-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
Hey @barry-jin , Thanks for submitting the PR
CI supported jobs: [unix-gpu, edge, miscellaneous, centos-cpu, clang, website, sanity, windows-gpu, windows-cpu, centos-gpu, unix-cpu] Note: |
@mxnet-bot run ci [sanity, unix-gpu] |
Jenkins CI successfully triggered : [sanity, unix-gpu] |
@mxnet-bot run ci [sanity, unix-gpu] |
Jenkins CI successfully triggered : [sanity, unix-gpu] |
@mxnet-bot run ci [unix-gpu] |
Jenkins CI successfully triggered : [unix-gpu] |
@mxnet-bot run ci [centos-cpu] |
Jenkins CI successfully triggered : [centos-cpu] |
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.
Thank you @barry-jin! You're adding back a subset of the operators removed in #18531 ("These operators are often used with mx.module APIs. Removing them for mxnet 2.0. You can find equivalent loss functions in mx.gluon.loss namespace.") Why not add back all the Operators that are commonly used therein? This may also be worth an update in the MXNet 2 RFC #16167 as you propose to support the Symbol APIs in the CPP package. For example, this change in CPP package also means that there is no need to drop the other language bindings that were also removed in #18531
cpp-package/tests/ci_test.sh
Outdated
# skippping temporarily, tracked by https://github.com/apache/incubator-mxnet/issues/20011 | ||
#cp ../../build/cpp-package/example/test_regress_label . | ||
#./test_regress_label | ||
|
||
# sh unittests/unit_test_mlp_csv.sh |
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.
Have you tried running?
@@ -0,0 +1,2 @@ | |||
# Rebuildable file(s) | |||
op.h |
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.
Why is op.h placed inside the build dir? Usually we only support out of source build in cmake. This .gitignore may no longer be needed as Makefile in-source build is no longer supported.
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.
op.h is generated by OpWrapperGenerator.py from user side, which should not be tracked by git. Should I just put this .gitignore in cpp-package directory or merged with incubator-mxnet/.gitignore
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.
op.h is generated by OpWrapperGenerator.py from user side
OpWrapperGenerator.py is invoked by cmake inside the build
folder ("out of source build"). Usually out-of-source build should not modify the code / files outside of the build
folder.
cpp-package/README.md
Outdated
## Building C++ Package | ||
|
||
The cpp-package directory contains the implementation of C++ API. As mentioned above, users are required to build this directory or package before using it. | ||
**The cpp-package is built while building the MXNet shared library, *libmxnet.so*.** |
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.
There is no reason for that. cpp-package depends only on libmxnet.so C APIs. It would be better to keep a separate CMakeLists.txt for the cpp-package with the only requirement to find libmxnet.so. This implies removing the USE_CPP_PACKAGE
in the main CMakeLists.txt
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.
Thanks for pointing it out. Let me update the documentation.
@mxnet-bot run ci [centos-gpu, unix-gpu] |
Jenkins CI successfully triggered : [centos-gpu, unix-gpu] |
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.
Thank you for adding the missing operators. I think the PR is good to merge now and the other points can be addressed later
Description
Migrate cpp-package to MXNet2.0. Still work in progress.
Checklist
Essentials
Changes
Comments