[Pytorch-iOS] remove ONNX & Turn on NO_API for mobile build
#23546
Conversation
There was a problem hiding this comment.
@xta0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
caffe2/CMakeLists.txt
Outdated
| @@ -460,6 +460,11 @@ if (NOT INTERN_BUILD_MOBILE OR NOT BUILD_CAFFE2_MOBILE) | |||
| ${TORCH_ROOT}/test/cpp/jit/test.cpp | |||
| ) | |||
|
|
|||
| if (INTERN_BUILD_MOBILE) | |||
| list(REMOVE_ITEM TORCH_SRCS ${TORCH_SRC_DIR}/csrc/jit/export.cpp) | |||
There was a problem hiding this comment.
REMOVE_ITEM is a bit magical, can we instead just add it only if the opposite condition is true?
There was a problem hiding this comment.
You mean something like this?
if (NOT INTERN_BUILD_MOBILE)
list(APPEND TORCH_SRCS ${TORCH_SRC_DIR}/csrc/jit/export.cpp)
list(APPEND TORCH_SRCS ${TORCH_ROOT}/test/cpp/jit/test.cpp)
endif()
caffe2/CMakeLists.txt
Outdated
| list(APPEND TORCH_SRCS ${TORCH_SRC_DIR}/csrc/jit/fuser/cpu/fused_kernel.cpp) | ||
| endif() | ||
| endif() | ||
|
|
||
| if (NOT WIN32 AND NOT INTERN_BUILD_MOBILE) |
caffe2/CMakeLists.txt
Outdated
| if (NOT INTERN_BUILD_MOBILE) | ||
| list(APPEND TORCH_SRCS ${TORCH_SRC_DIR}/csrc/jit/export.cpp) | ||
| list(APPEND TORCH_SRCS ${TORCH_ROOT}/test/cpp/jit/test.cpp) | ||
| if(NOT WIN32) |
caffe2/CMakeLists.txt
Outdated
| @@ -497,7 +501,9 @@ if (NOT INTERN_BUILD_MOBILE OR NOT BUILD_CAFFE2_MOBILE) | |||
| install(TARGETS caffe2_nvrtc DESTINATION "${TORCH_INSTALL_LIB_DIR}") | |||
| endif() | |||
|
|
|||
| if (NOT NO_API) | |||
| if (INTERN_BUILD_MOBILE) | |||
There was a problem hiding this comment.
do we need both INTERN_BUILD_MOBILE and NO_API here? can we use one and drop the other? e.g.:
option 1: keep NO_API and set it automatically in master CMakeLists.txt for mobile build; use if (NOT NO_API) ... else() ...;
option 2: drop NO_API and use if (INTERN_BUILD_MOBILE) ... else () ...;
"csrc/api/src/jit.cpp" is already listed on line 432, so we should be able to directly set NO_API in master CMakeLists for mobile build.
There was a problem hiding this comment.
And can we make NO_API change a separate PR our rename this PR to better reflect your change.
You can use ghstack to maintain a stack of local diffs / PRs.
There was a problem hiding this comment.
There is one file - output-archive.cpp that depends on export.h is included via this NO_API flag.
And can we make NO_API change a separate PR our rename this PR to better reflect your change.
Yea, let's open another PR for this NO_API change
do we need both INTERN_BUILD_MOBILE and NO_API here? can we use one and drop the other? e.g.:
option 1: keep NO_API and set it automatically in master CMakeLists.txt for mobile build; use if (NOT NO_API) ... else() ...;
option 2: drop NO_API and use if (INTERN_BUILD_MOBILE) ... else () ...;"csrc/api/src/jit.cpp" is already listed on line 432, so we should be able to directly set NO_API in master CMakeLists for mobile build.
Actually I'm thinking if we can get rid of this jit.cpp, cuz there is only one function in this file. I've searched this symbol in the codebase, looks like no other cpp files use it. I'll double check
xta0
changed the title
NO_API for mobile build
There was a problem hiding this comment.
@xta0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@xta0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Is this PR related to the latest Windows nightlies no longer having ONNX? |
|
@xsacha - what exactly are you referring to by "no longer having ONNX"? from code it doesn't seem like it'd affect windows, but might make sense to investigate |
|
Latest nightlies in the past 2 days at least are missing it. |
|
@xsacha Which onnx files are you referring to? |
Summary
The iOS build was broken after this PR 馃憠 23195 was merged, as there are two files still have dependency on ONNX.
test.cppintest/cpp/jitexport.cppintorch/csrc/jitThis PR is to remove ONNX completely from mobile build.
Test plan
build_ios.shfinished successfully.libtorch.acan be compiled and run on iOS devices