Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch from Bazel to CMake #68

Closed
wants to merge 1 commit into from
Closed

Switch from Bazel to CMake #68

wants to merge 1 commit into from

Conversation

VivekPanyam
Copy link
Collaborator

@VivekPanyam VivekPanyam commented May 24, 2019

We're switching from Bazel to CMake for a few reasons:

  • In a future PR, we're going to make the Python Neuropod interface call through to native code (so we only have a single inference implementation). This means we need to ship binaries as part of the wheel or build on the user's computer.
  • Bazel is all about making things hermetic and we want build using the dependencies on the user's computer. This is hard to do in Bazel
  • Using CMake as part of the Python package install process is much easier than using Bazel during that process

Generally, CMake also gives us more granular control over building and linking the backend shared objects. User-defined intermediate shared objects are a little awkward to deal with in Bazel (happy to point to GitHub issues about this or chat more offline about specifics)

(The Bazel build files and configuration will be removed in another PR once this lands)

# Load libtorch. This needs to happen here so both the tests and the
# backends can find the library
# TODO(vip): refactor so this doesn't need to be in the root CMakeLists.txt
find_package(Torch REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

I got this error on my mac:

CMake Error at CMakeLists.txt:19 (find_package):
  By not providing "FindTorch.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "Torch", but
  CMake did not find one.

  Could not find a package configuration file provided by "Torch" with any of
  the following names:

    TorchConfig.cmake
    torch-config.cmake

  Add the installation prefix of "Torch" to CMAKE_PREFIX_PATH or set
  "Torch_DIR" to a directory containing one of the above files.  If "Torch"
  provides a separate development package or SDK, be sure it has been
  installed.
CMake Error at CMakeLists.txt:19 (find_package):
  By not providing "FindTorch.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "Torch", but
  CMake did not find one.

  Could not find a package configuration file provided by "Torch" with any of
  the following names:

    TorchConfig.cmake
    torch-config.cmake

  Add the installation prefix of "Torch" to CMAKE_PREFIX_PATH or set
  "Torch_DIR" to a directory containing one of the above files.  If "Torch"
  provides a separate development package or SDK, be sure it has been
  installed.

What if the user doesn't have PyTorch/other backend or doesn't want it compiled?

# TODO(vip): refactor so this doesn't need to be in the root CMakeLists.txt
find_package(Torch REQUIRED)

# Add the backends
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have includes for jsoncpp like suggested by open-source-parsers/jsoncpp#455?

include_directories("${CMAKE_SOURCE_DIR}/../")

# Make sure all symbols are defined in all the libraries we build
set (CMAKE_SHARED_LINKER_FLAGS "-Wl,--no-undefined")
Copy link
Contributor

Choose a reason for hiding this comment

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

On Mac:

[ 19%] Linking CXX shared library libneuropods.dylib
ld: unknown option: --no-undefined

Copy link
Contributor

Choose a reason for hiding this comment

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

But then, without it:

[ 29%] Linking CXX shared library libneuropod_tensorflow_backend.dylib
Undefined symbols for architecture x86_64:
  "_TF_AllocateTensor", referenced from:

# Build the backend
file(GLOB SOURCES "*.cc")
add_library( neuropod_tensorflow_backend SHARED ${SOURCES})
target_link_libraries(neuropod_tensorflow_backend ${LIBTENSORFLOW} neuropods)
Copy link
Contributor

Choose a reason for hiding this comment

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

${LIBTENSORFLOW} didn't seem to work for some reason, even if I added tensorflow instead of ${LIBTENSORFLOW}, I got:

[ 22%] Linking CXX shared library libneuropod_tensorflow_backend.dylib
ld: library not found for -ltensorflow

@VivekPanyam VivekPanyam force-pushed the cmake branch 2 times, most recently from b08f6e1 to 0e7c1e5 Compare May 30, 2019 20:04
@VivekPanyam
Copy link
Collaborator Author

Having the Neuropod python code call through to native code is more complex than we initially thought. We talked about some solutions offline, but they are all quite complex.

Building with "whatever is on the user's computer" is not generally feasible because most people don't have libtorch or the tensorflow C bindings locally (they have pytorch or tensorflow for python).

If we release prebuilt versions of the backends (with the TF C api + libtorch), it's possible to run into symbol conflicts and subtle issues when pytorch + libtorch are loaded into the same process. Same with TF + libtensorflow_c. This is even trickier if they are built separately or are different versions than the libraries on the user's computer.

Given this information, a lot of the benefits of switching to CMake (mentioned above) are no longer valid

@VivekPanyam VivekPanyam closed this Jun 4, 2019
@VivekPanyam VivekPanyam deleted the cmake branch October 31, 2019 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants