-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
MXInitialise Readies mxnet for use. While this is possible with some other commands this function makes more sense and does not require unneeded variables. MXNDArrayLoadListFromMemory Duplicates MXNDArrayLoad but from a location in memory. This was needed for my work as we bundle models into another file.
Merge branch 'master' of git://github.com/apache/incubator-mxnet
The test that failed is flaky #9856 |
MXInitialise Readies mxnet for use. While this is possible with some other commands this function makes more sense and does not require unneeded variables. MXNDArrayLoadListFromMemory Duplicates MXNDArrayLoad but from a location in memory. This was needed for my work as we bundle models into another file.
MXInitialise Readies mxnet for use. While this is possible with some other commands this function makes more sense and does not require unneeded variables. MXNDArrayLoadListFromMemory Duplicates MXNDArrayLoad but from a location in memory. This was needed for my work as we bundle models into another file.
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.
What's the deal with all the uncommented code?
CMakeLists.txt
Outdated
# ---[ NNPack | ||
if(USE_NNPACK) | ||
if (USE_MKLDNN) | ||
message(WARNING "With MKLDNN enabled NNPack operators will not run.") |
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? What happens in that case?
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.
both MKLDNN and NNPack define CPU versions of some operators, while there won't be an issue with compilation, when these operators are invoked they will need to use only the one backend.
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.
What happens if a user invokes one of these operators?
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.
The way I'm coding it, it will use the mkldnn backend. Once I finish that maybe some testing would show that NNPack is faster in which case it can be changed to the default.
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 a fan of such implicit actions. It should be up to the user to decide which Backend to use. To me, setting both options seems like a conflict and should fail.
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'll change it to fail in that case.
I wasn't going to push but I had conflicts with the upstream/master that needed to be resolved. The problem is that none of the old implementation of the operators would work, but I would like to keep them as a reference while I code new versions. |
set(CONFU_DEPENDENCIES_BINARY_DIR "${CMAKE_BINARY_DIR}/3rdparty/NNPACK/deps" | ||
CACHE PATH "Confu-style dependencies source directory") | ||
|
||
add_subdirectory("${NNPACK_SOURCE_DIR}") |
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'd suggest to do set the following variables:
set(NNPACK_BUILD_TESTS OFF CACHE BOOL "")
set(NNPACK_BUILD_BENCHMARKS OFF CACHE BOOL "")
set(NNPACK_LIBRARY_TYPE "static" CACHE STRING "")
set(PTHREADPOOL_LIBRARY_TYPE "static" CACHE STRING "")
set(CPUINFO_LIBRARY_TYPE "static" CACHE STRING "")
NNPACK_BUILD_TESTS
and NNPACK_BUILD_BENCHMARKS
disabled building NNPACK's own tests and benchmarks (which are enabled by default), and setting NNPACK_LIBRARY_TYPE
, PTHREADPOOL_LIBRARY_TYPE
and CPUINFO_LIBRARY_TYPE
as "static"
would build these projects as static libraries even if BUILD_SHARED_LIBS
is ON
.
After add_subdirectory
, you'd likely want to add
set_property(TARGET nnpack PROPERTY POSITION_INDEPENDENT_CODE ON)
set_property(TARGET pthreadpool PROPERTY POSITION_INDEPENDENT_CODE ON)
set_property(TARGET cpuinfo PROPERTY POSITION_INDEPENDENT_CODE ON)
to build nnpack, pthreadpool, and cpuinfo with -fPIC
even through they are static libraries.
Hi, the community has passed to vote about associating the code changes with JIRA (https://lists.apache.org/thread.html/ab22cf0e35f1bce2c3bf3bec2bc5b85a9583a3fe7fd56ba1bbade55f@%3Cdev.mxnet.apache.org%3E) We have updated the guidelines for contributors in https://cwiki.apache.org/confluence/display/MXNET/Development+Process, please ensure that you have created a JIRA at https://issues.apache.org/jira/projects/MXNET/issues/ to describe your work in this pull request and include the JIRA title in your PR as [MXNET-xxxx] your title where MXNET-xxxx is the JIRA id Thanks! |
Hi @dabraude. Thanks for the patch and for proposing the support. Given that this PR hasn't been updated for quite a while and the many changes in our CI, it would be great if you could create a new PR for this change. Thank you. |
Description
This is the first part of #9719
Checklist
Essentials
make lint
)Changes
Comments
@Maratyszcza may want to check that everything is correct with regards to imports
Next up is refactoring the operators