Unify libtorch and libcaffe2 #17783

kostmo · 2019-03-07T23:47:15Z

This PR is an intermediate step toward the ultimate goal of eliminating "caffe2" in favor of "torch". This PR moves all of the files that had constituted "libtorch.so" into the "libcaffe2.so" library, and wraps "libcaffe2.so" with a shell library named "libtorch.so". This means that, for now, caffe2/CMakeLists.txt becomes a lot bigger, and torch/CMakeLists.txt becomes smaller.

The torch Python bindings (torch_python.so) still remain in torch/CMakeLists.txt.

The follow-up to this PR will rename references to caffe2 to torch, and flatten the shell into one library.

dzhulgakov

Great progress, CI passes! :)

One question is about libtorch.so - as far as I understand there's not such file any more. We probably should keep libtorch.so name because external things can depend on it. I'd imagine there are fewer dependencies on libcaffe2.so externally and we can make a symlink maybe.

I think it's getting to a point to get more people's input. @soumith @orionr @yf225 @bddppq - please take a look. Also, we have a release soon. Should we strive to land it before that? (pro: stress testing during binary builds, cons: potentially more churn)

dzhulgakov · 2019-04-11T16:24:17Z

caffe2/CMakeLists.txt

+# formerly-libtorch
+# ==========================================================
+
+set(TORCH_SRC_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../torch")


we should probably just lift this main folder up to avoid .. but we can do it in a separate PR

dzhulgakov · 2019-04-11T16:25:41Z

caffe2/CMakeLists.txt

+  set(TORCH_SRCS
+    ${GENERATED_CXX_TORCH}
+    ${GENERATED_H_TORCH}
+    ${TORCH_SRC_DIR}/csrc/autograd/anomaly_mode.cpp


it'd be nice to dedup this list with build_variables.py in the future (not this PR) - I wonder whether we can just read the same file in in both build systems

dzhulgakov · 2019-04-11T16:29:38Z

caffe2/CMakeLists.txt

+  target_compile_definitions(caffe2 PUBLIC _THP_CORE)
+
+
+  # until they can be unified, keep these lists synced with setup.py


probably it's worth to look into it in the future (not sure what exact history of this duplication is) - from setup.py it seems we're building only stub.cpp, so it might be outdated comment

dzhulgakov · 2019-04-11T16:31:03Z

caffe2/CMakeLists.txt

 if (NOT WIN32)
-  target_compile_options(caffe2 PRIVATE "-fvisibility=hidden")
+    # FIXME kostmo
+#  target_compile_options(caffe2 PRIVATE "-fvisibility=hidden")


after we fixed THPPointer - can we bring it on or are there still a lot of failures?

There are still failures, e.g.:

ImportError: /scratch/kostmo/anaconda3/lib/python3.7/site-packages/torch/lib/libtorch_python.so: undefined symbol: _ZTIN5torch3jit4NodeE

dzhulgakov · 2019-04-11T16:31:24Z

caffe2/CMakeLists.txt

-if (NOT MSVC)
-  target_compile_options(caffe2 INTERFACE "$<$<COMPILE_LANGUAGE:CXX>:-std=c++11>")
-endif()
+#if (NOT MSVC)


why is this off?

Seems redundant with this line:

set_property(TARGET caffe2 PROPERTY CXX_STANDARD 11)

kostmo · 2019-04-12T22:40:39Z

Ran into this Windows build failure after rebasing (to resolve a merge conflict):

21:55:55 dataloader.cpp.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: virtual __cdecl torch::data::samplers::Sampler<struct TestIndex>::~Sampler<struct TestIndex>(void)" (__imp_??1?$Sampler@UTestIndex@@@samplers@data@torch@@UEAA@XZ) referenced in function "int `class std::unique_ptr<class torch::data::StatelessDataLoader<struct TestIndexDataset,struct TestIndexSampler>,struct std::default_delete<class torch::data::StatelessDataLoader<struct TestIndexDataset,struct TestIndexSampler> > > __cdecl torch::data::make_data_loader<struct TestIndexDataset,struct TestIndexSampler>(struct TestIndexDataset,struct TestIndexSampler,struct data::dtor$7::DataLoaderOptions)'::`1'::dtor$7" (?dtor$7@?0???$make_data_loader@UTestIndexDataset@@UTestIndexSampler@@@data@torch@@YA?AV?$unique_ptr@V?$StatelessDataLoader@UTestIndexDataset@@UTestIndexSampler@@@data@torch@@U?$default_delete@V?$StatelessDataLoader@UTestIndexDataset@@UTestIndexSampler@@@data@torch@@@std@@@std@@UTestIndexDataset@@UTestIndexSampler@@UDataLoaderOptions@01@@Z@4HA)
21:55:55 dataloader.cpp.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) const torch::data::samplers::Sampler<struct TestIndex>::`vftable'" (__imp_??_7?$Sampler@UTestIndex@@@samplers@data@torch@@6B@) referenced in function "class std::unique_ptr<class torch::data::StatelessDataLoader<struct TestIndexDataset,struct TestIndexSampler>,struct std::default_delete<class torch::data::StatelessDataLoader<struct TestIndexDataset,struct TestIndexSampler> > > __cdecl torch::data::make_data_loader<struct TestIndexDataset,struct TestIndexSampler>(struct TestIndexDataset,struct TestIndexSampler,struct torch::data::DataLoaderOptions)" (??$make_data_loader@UTestIndexDataset@@UTestIndexSampler@@@data@torch@@YA?AV?$unique_ptr@V?$StatelessDataLoader@UTestIndexDataset@@UTestIndexSampler@@@data@torch@@U?$default_delete@V?$StatelessDataLoader@UTestIndexDataset@@UTestIndexSampler@@@data@torch@@@std@@@std@@UTestIndexDataset@@UTestIndexSampler@@UDataLoaderOptions@01@@Z)
21:55:55 bin\test_api.exe : fatal error LNK1120: 2 unresolved externals

dzhulgakov · 2019-04-16T16:35:29Z

It's getting in a good shape - all of the following with visibility=hidden enabled:

The windows build passes (!!!) and only tests fails because of test_extension - we have hard-coded libtorch name here: https://github.com/pytorch/pytorch/blob/master/torch/utils/cpp_extension.py#L432
ASAN build is broken because of header-only classes with visibility=hidden, I'm fixing it
ROCm has some symbol missing - I'm looking into it too.

However, before we can land this we need to decide naming problem:

libtorch.so is a public name, so we should either name the entire thing libtorch (and break libcaffe2 backward compat which is more ok) or figure out some way to create a proxy library (or symlink?)
gpu pieces are now linked in the "add-on" library caffe2_gpu.so. If we decide not to make a proxy libtorch which depends on both in (1), we'd need to fold the gpu bits directly in the main library.

@soumith @gchanan - do you have insights on above?

Also asking for review from more folks: @orionr @bddppq @yf225 @pjh5

bddppq · 2019-04-16T17:15:03Z

torch/CMakeLists.txt

    list(APPEND TORCH_PYTHON_SRCS
      ${TORCH_SRC_DIR}/csrc/cuda/Module.cpp
      ${TORCH_SRC_DIR}/csrc/cuda/Storage.cpp
      ${TORCH_SRC_DIR}/csrc/cuda/Stream.cpp
      ${TORCH_SRC_DIR}/csrc/cuda/Event.cpp
      ${TORCH_SRC_DIR}/csrc/cuda/utils.cpp
-      ${TORCH_SRC_DIR}/csrc/cuda/comm.cpp


Is this is the reason causing the undefined symbol in rocm build? (although I don't understand why cuda build doesn't see the same issue).

Ok I saw it has been moved to caffe2/CMakeLists.txt. But from the generated build.ninja file, comm.cpp is not included in the rocm build.

bddppq · 2019-04-16T17:23:58Z

caffe2/CMakeLists.txt

+  endif ()
+
+  if (USE_CUDA)
+    list(APPEND Caffe2_GPU_SRCS


These need to be added to Caffe2_HIP_SRCS if USE_ROCM

dzhulgakov · 2019-04-18T07:35:45Z

So apparently windows CI (on jenkins) doesn't start if the merge doesn't go smoothly. Brace for rebase I guess.

pjh5 · 2019-04-22T19:58:42Z

caffe2/CMakeLists.txt

+# Only necessary on Windows?
+# Contains "caffe2" and "caffe2_gpu".
+file(WRITE ${CMAKE_BINARY_DIR}/empty.cpp "")
+add_library(torch STATIC ${CMAKE_BINARY_DIR}/empty.cpp)


What does this solve on windows? It looks like torch is just a wrapper around caffe2 and caffe2_gpu; what do we need this target for?

peterjc123 · 2019-05-08T13:50:41Z

caffe2/CMakeLists.txt

-file(WRITE ${DUMMY_EMPTY_FILE} "")
+
+if (MSVC)
+  set(DUMMY_FILE_CONTENT "__declspec(dllexport) int ignore_this_library_placeholder(){return 0;}")


needs escaping before ;, that is

set(DUMMY_FILE_CONTENT "__declspec(dllexport) int ignore_this_library_placeholder(){return 0\\;}")

By exporting this function, torch.lib will be generated, but it will not reference any other libs because it is not calling any funcs here, so it will only contain the placeholder func in the symbol table, which is quite useless.
I don't know why the empty stub is needed here. Maybe it was used to keep the backward compatibility? But it was not true, we are already appending caffe2.lib and c10.lib when we do find_package(Torch REQUIRED) here when BUILD_SHARED_LIB is ON. Or maybe we just to pretend torch.dll still exists?
Anyway, if you still want to keep the dependency link between torch.lib and caffe2.lib / caffe_gpu.lib, then you have to call at least one func from these libs in empty.cpp and switch off the optimizing flags. Here is my PR for your reference: https://github.com/pytorch/pytorch/pull/16071/files.

I don't know why the empty stub is needed here. Maybe it was used to keep the backward compatibility?

Relaying an answer from Dima:

primarily to keep the shared library file name to be torch, not caffe2 to make sure that people builds and extensions don’t break
Alternatively we could just generate only libtorch instead of libcaffe2 but it’d make the PR bigger and also would still have the same issue for C2 users ( but the latter part might be fine)

peterjc123 · 2019-05-08T14:36:43Z

As for the build timeout issue for caffe2 win builds, I think that you can try the following things:

Remove the argument /maxcpucount in tools\build_pytorch_libs.py, to confirm whether it was or was not caused by a deadlock in msbuild
Try different generators (in/ not in msbuild)
Updating VS (in / not in the MSVC toolchain)

peterjc123 · 2019-05-09T11:11:48Z

@pytorchbot retest this please

orionr

Woohoo!

facebook-github-bot

@kostmo is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-05-10T19:36:10Z

@kostmo merged this pull request in 4ba28de.

Summary: #17783 has made ninja and makefile builds to print out build commands unconditionally, this has made the build log very verbose, e.g. ROCm CI build log becomes >13mb. Large build log make searching for real error hard. #20508 has reverted the ninja change, and this one reverts the makefile change. Pull Request resolved: #21053 Differential Revision: D15533412 Pulled By: bddppq fbshipit-source-id: ad89b617d06acc670d75d4cf25111a4081e9c95e

Summary: This renames the CMake `caffe2` target to `torch`, as well as renaming `caffe2_gpu` to `torch_gpu` (and likewise for other gpu target variants). Many intermediate variables that don't manifest as artifacts of the build remain for now with the "caffe2" name; a complete purge of `caffe2` from CMake variable names is beyond the scope of this PR. The shell `libtorch` library that had been introduced as a stopgap in #17783 is again flattened in this PR. Pull Request resolved: #20774 Differential Revision: D15769965 Pulled By: kostmo fbshipit-source-id: b86e8c410099f90be0468e30176207d3ad40c821

Summary: This renames the CMake `caffe2` target to `torch`, as well as renaming `caffe2_gpu` to `torch_gpu` (and likewise for other gpu target variants). Many intermediate variables that don't manifest as artifacts of the build remain for now with the "caffe2" name; a complete purge of `caffe2` from CMake variable names is beyond the scope of this PR. The shell `libtorch` library that had been introduced as a stopgap in pytorch/pytorch#17783 is again flattened in this PR. Pull Request resolved: pytorch/pytorch#20774 Differential Revision: D15769965 Pulled By: kostmo fbshipit-source-id: b86e8c410099f90be0468e30176207d3ad40c821

kostmo requested review from ebetica, goldsborough and yf225 as code owners March 8, 2019 01:15

dzhulgakov reviewed Apr 11, 2019

View reviewed changes

dzhulgakov requested review from soumith and bddppq April 16, 2019 16:25

bddppq reviewed Apr 16, 2019

View reviewed changes

caffe2/CMakeLists.txt

endif ()

if (USE_CUDA)

list(APPEND Caffe2_GPU_SRCS

Copy link

Contributor

bddppq Apr 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need to be added to Caffe2_HIP_SRCS if USE_ROCM

pjh5 reviewed Apr 22, 2019

View reviewed changes

dzhulgakov mentioned this pull request Apr 25, 2019

C++ changes toward libtorch and libcaffe2 unification #19554

Closed

kostmo requested review from apaszke, ezyang and fmassa as code owners May 1, 2019 22:45

kostmo changed the title ~~Unify libtorch and libcaffe2~~ [wip] Unify libtorch and libcaffe2 May 1, 2019

peterjc123 reviewed May 8, 2019

View reviewed changes

Unify libtorch and libcaffe2

33b5253

kostmo changed the title ~~[wip] Unify libtorch and libcaffe2~~ Unify libtorch and libcaffe2 May 9, 2019

orionr approved these changes May 9, 2019

View reviewed changes

facebook-github-bot reviewed May 9, 2019

View reviewed changes

soumith approved these changes May 9, 2019

View reviewed changes

facebook-github-bot closed this in 4ba28de May 10, 2019

facebook-github-bot added the merged label May 10, 2019

bddppq mentioned this pull request May 29, 2019

Make build with makefiles less noisy #21053

Closed

peterjc123 mentioned this pull request May 31, 2019

Fix the caffe2_gpu linkage with torch on Windows #16071

Closed

kostmo mentioned this pull request May 31, 2019

Torch rename #20774

Closed

mattip mentioned this pull request Apr 12, 2020

[MSVC Static] DeviceGuardImpl for CPU is not available #18757

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify libtorch and libcaffe2 #17783

Unify libtorch and libcaffe2 #17783

kostmo commented Mar 7, 2019 •

edited

dzhulgakov left a comment

dzhulgakov Apr 11, 2019

dzhulgakov Apr 11, 2019

dzhulgakov Apr 11, 2019

dzhulgakov Apr 11, 2019

kostmo Apr 11, 2019

dzhulgakov Apr 11, 2019

kostmo Apr 11, 2019

kostmo commented Apr 12, 2019

dzhulgakov commented Apr 16, 2019

bddppq Apr 16, 2019

bddppq Apr 16, 2019

bddppq Apr 16, 2019

dzhulgakov commented Apr 18, 2019

pjh5 Apr 22, 2019

peterjc123 May 8, 2019

peterjc123 May 8, 2019 •

edited

kostmo May 9, 2019

peterjc123 commented May 8, 2019

peterjc123 commented May 9, 2019

orionr left a comment

facebook-github-bot left a comment

facebook-github-bot commented May 10, 2019

		target_compile_definitions(caffe2 PUBLIC _THP_CORE)


		# until they can be unified, keep these lists synced with setup.py

Unify libtorch and libcaffe2 #17783

Unify libtorch and libcaffe2 #17783

Conversation

kostmo commented Mar 7, 2019 • edited

dzhulgakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostmo commented Apr 12, 2019

dzhulgakov commented Apr 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzhulgakov commented Apr 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterjc123 May 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterjc123 commented May 8, 2019

peterjc123 commented May 9, 2019

orionr left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented May 10, 2019

kostmo commented Mar 7, 2019 •

edited

peterjc123 May 8, 2019 •

edited