Fix TORCH_LIBRARIES variables when do static build #49458

gemfield · 2020-12-16T12:42:43Z

With this fix, TORCH_LIBRARIES variable can provide all nessesary static libraries build from pytorch repo.
User program (if do static build) now can just link with ${TORCH_LIBRARIES} + MKL + cuda runtime.

facebook-github-bot · 2020-12-16T13:13:08Z

💊 CI failures summary and remediations

As of commit 1b2e102 (more details on the Dr. CI page):

3/3 failures introduced in this PR

🕵️ 3 new failures recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

pytorch_linux_xenial_py3_6_gcc5_4_build (1/3)

Step: "(Optional) Merge target branch" (full log | diagnosis details | 🔁 rerun)

Automatic merge failed; fix conflicts and then commit the result.

CONFLICT (add/add): Merge conflict in .circleci/scripts/binary_linux_test.sh
Auto-merging .circleci/scripts/binary_linux_test.sh
CONFLICT (add/add): Merge conflict in .circleci/generate_config_yml.py
Auto-merging .circleci/generate_config_yml.py
CONFLICT (add/add): Merge conflict in .circleci/docker/common/install_conda.sh
Auto-merging .circleci/docker/common/install_conda.sh
CONFLICT (add/add): Merge conflict in .circleci/config.yml
Auto-merging .circleci/config.yml
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/dimensions.py
Auto-merging .circleci/cimodel/data/dimensions.py
Automatic merge failed; fix conflicts and then commit the result.


Exited with code exit status 1

pytorch_linux_xenial_cuda9_2_cudnn7_py3_gcc5_4_build (2/3)

Step: "(Optional) Merge target branch" (full log | diagnosis details | 🔁 rerun)

Automatic merge failed; fix conflicts and then commit the result.

CONFLICT (add/add): Merge conflict in .circleci/scripts/binary_linux_test.sh
Auto-merging .circleci/scripts/binary_linux_test.sh
CONFLICT (add/add): Merge conflict in .circleci/generate_config_yml.py
Auto-merging .circleci/generate_config_yml.py
CONFLICT (add/add): Merge conflict in .circleci/docker/common/install_conda.sh
Auto-merging .circleci/docker/common/install_conda.sh
CONFLICT (add/add): Merge conflict in .circleci/config.yml
Auto-merging .circleci/config.yml
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/dimensions.py
Auto-merging .circleci/cimodel/data/dimensions.py
Automatic merge failed; fix conflicts and then commit the result.


Exited with code exit status 1

pytorch_xla_linux_bionic_py3_6_clang9_build (3/3)

Step: "(Optional) Merge target branch" (full log | diagnosis details | 🔁 rerun)

Automatic merge failed; fix conflicts and then commit the result.

CONFLICT (add/add): Merge conflict in .circleci/scripts/binary_linux_test.sh
Auto-merging .circleci/scripts/binary_linux_test.sh
CONFLICT (add/add): Merge conflict in .circleci/generate_config_yml.py
Auto-merging .circleci/generate_config_yml.py
CONFLICT (add/add): Merge conflict in .circleci/docker/common/install_conda.sh
Auto-merging .circleci/docker/common/install_conda.sh
CONFLICT (add/add): Merge conflict in .circleci/config.yml
Auto-merging .circleci/config.yml
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/dimensions.py
Auto-merging .circleci/cimodel/data/dimensions.py
Automatic merge failed; fix conflicts and then commit the result.


Exited with code exit status 1

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

gemfield · 2020-12-17T02:17:42Z

@malfet could you have a code review for this PR?

malfet

Overall LGTM, but please address review feedback, namely refactor repeated patterns into append_wholearchive_lib_if_found

malfet · 2020-12-17T02:24:30Z

cmake/TorchConfig.cmake.in

+  find_library(C10_LIBRARY c10 PATHS "${TORCH_INSTALL_PREFIX}/lib")
+  list(APPEND TORCH_LIBRARIES ${C10_LIBRARY})


Why not use append_torchlib macro here?

I didn't plan to touch the code which belongs to shared library build. Anyway, I can fix this if it looks good.

malfet · 2020-12-17T02:25:07Z

cmake/TorchConfig.cmake.in

+    if(${_arg}_LIBRARY)
+      list(APPEND TORCH_LIBRARIES ${${_arg}_LIBRARY})
+    else()
+      message(WARNING "static library ${${_arg}_LIBRARY} not found.")


Can you explain why this one should not be an error?
Also, why this macro is specific to a static libraries discovery?

warning instead of error mainly because of two reasons:
1, I cannot find the corresponding CMake option for every individual static library, so if the static library is not found, means the implied cmake option is off (rather than an error);
2, some static libraries are not mandatory for end user, warning instead of error provides flexibility in this situation.

Hmm, can you give an example of optional library dependencies?
Also "static library" message is misleading, as append_torchlib_if_found is called from both shared and static library path.

eigen_blas is optional, as we may use MKL as lapack backend, and I cannot find a public cmake option to control this,e.g.:

if(@USE_EIGENBLAS@)

If eigen_blas option can be found, and also caffe2_protos, protobuf-lite, protobuf, protoc, onnx, onnx_proto, foxi_loader, fmt, sleef, asmjit are all mandatory library for static build, we can change the warning to error.

malfet · 2020-12-17T02:26:33Z

cmake/TorchConfig.cmake.in

+  endforeach()
+endmacro()
+
+function(add_whole_archive_flag lib output_var)


Can you please explain why add_whole_archive_flag is a function, but append_torchlib_if_found is macro?

list append seems not support parent scope, while "set list" seems not intuitive, so use macro instead. And, to be consistent, I will change the function(add_whole_archive_flag) to macro if it looks good.

malfet · 2020-12-17T02:28:24Z

cmake/TorchConfig.cmake.in

+  else()
+    set(library_with_flag "")
+  endif()


Can you please explain, why we shouldn't raise an error if torch_LIBRARY could not be found?

Some static libraries are optional, e.g. for torch_cuda and c10_cuda. After pytorch cuda build, end user can still link libtorch with cuda libraries missing (just use the cpu device) .

malfet · 2020-12-17T02:29:48Z

cmake/TorchConfig.cmake.in

+    add_whole_archive_flag(${torch_cpu_LIBRARY} library_with_flag)
+  else()
+    set(library_with_flag "")
+  endif()
+  list(APPEND TORCH_LIBRARIES ${library_with_flag})


Can you please explain why else() statement is needed?

Suggested change

add_whole_archive_flag(${torch_cpu_LIBRARY} library_with_flag)

else()

set(library_with_flag "")

endif()

list(APPEND TORCH_LIBRARIES ${library_with_flag})

add_whole_archive_flag(${torch_cpu_LIBRARY} library_with_flag)

list(APPEND TORCH_LIBRARIES ${library_with_flag})

endif()

You are right.

malfet · 2020-12-17T02:31:12Z

cmake/TorchConfig.cmake.in

+  if(torch_cuda_LIBRARY)
+    add_whole_archive_flag(${torch_cuda_LIBRARY} library_with_flag)
+  else()
+    set(library_with_flag "")
+  endif()
+  list(APPEND TORCH_LIBRARIES ${library_with_flag})


This looks like a repeated pattern, can it be refactored to macro? Something like add_wholearchive_library_if_found?

yes, this will be done.

…f_found macro

gemfield · 2020-12-18T01:55:11Z

@malfet All have been fixed.

facebook-github-bot

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-01-13T15:58:26Z

@malfet merged this pull request in deba3bd.

Fix TORCH_LIBRARIES variables when do static build

74897ca

facebook-github-bot added the cla signed label Dec 16, 2020

pytorchbot added the open source label Dec 16, 2020

gemfield mentioned this pull request Dec 17, 2020

static library order is important, fix the wrong library order #45424

Closed

malfet approved these changes Dec 17, 2020

View reviewed changes

refactor add_whole_archive_flag function to append_wholearchive_lib_i…

e925ea3

…f_found macro

facebook-github-bot reviewed Jan 13, 2021

View reviewed changes

Fix warning message for append_torchlib_if_found

1b2e102

facebook-github-bot closed this in deba3bd Jan 13, 2021

facebook-github-bot added the Merged label Jan 13, 2021

droberts195 mentioned this pull request Sep 23, 2021

Native bundle increase in 8.0.0 elastic/ml-cpp#2038

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TORCH_LIBRARIES variables when do static build #49458

Fix TORCH_LIBRARIES variables when do static build #49458

gemfield commented Dec 16, 2020

facebook-github-bot commented Dec 16, 2020 •

edited

Loading

gemfield commented Dec 17, 2020

malfet left a comment

malfet Dec 17, 2020

gemfield Dec 17, 2020

malfet Dec 17, 2020

gemfield Dec 17, 2020

malfet Jan 13, 2021

gemfield Jan 13, 2021

malfet Dec 17, 2020

gemfield Dec 17, 2020 •

edited

Loading

malfet Dec 17, 2020

gemfield Dec 17, 2020 •

edited

Loading

malfet Dec 17, 2020

gemfield Dec 17, 2020

malfet Dec 17, 2020

gemfield Dec 17, 2020

gemfield commented Dec 18, 2020

facebook-github-bot left a comment

facebook-github-bot commented Jan 13, 2021

		find_library(C10_LIBRARY c10 PATHS "${TORCH_INSTALL_PREFIX}/lib")
		list(APPEND TORCH_LIBRARIES ${C10_LIBRARY})

Fix TORCH_LIBRARIES variables when do static build #49458

Fix TORCH_LIBRARIES variables when do static build #49458

Conversation

gemfield commented Dec 16, 2020

facebook-github-bot commented Dec 16, 2020 • edited Loading

💊 CI failures summary and remediations

🕵️ 3 new failures recognized by patterns

pytorch_linux_xenial_py3_6_gcc5_4_build (1/3)

pytorch_linux_xenial_cuda9_2_cudnn7_py3_gcc5_4_build (2/3)

pytorch_xla_linux_bionic_py3_6_clang9_build (3/3)

gemfield commented Dec 17, 2020

malfet 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

gemfield Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gemfield Dec 17, 2020 • edited Loading

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

gemfield commented Dec 18, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 13, 2021

facebook-github-bot commented Dec 16, 2020 •

edited

Loading

gemfield Dec 17, 2020 •

edited

Loading

gemfield Dec 17, 2020 •

edited

Loading