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

Should we ship the static kineto library? #335

Open
h-vetinari opened this issue Jan 28, 2025 · 5 comments
Open

Should we ship the static kineto library? #335

h-vetinari opened this issue Jan 28, 2025 · 5 comments

Comments

@h-vetinari
Copy link
Member

As a follow-up to #76, the question arises if we want to ship the static libkineto.a / kineto.lib. It's part of the private interface of X:

--   Public Dependencies  : caffe2::mkl
--   Private Dependencies : Threads::Threads;pthreadpool;cpuinfo;pytorch_qnnpack;nnpack;XNNPACK;fbgemm;fp16;caffe2::openmp;tensorpipe;nlohmann;gloo;rt;fmt::fmt-header-only;kineto;gcc_s;gcc;dl

and it shows up as a warning when loading Torch through CMake:

Caffe2: Protobuf version 28.3.0
CMake Warning at $PREFIX/share/cmake/Torch/TorchConfig.cmake:22 (message):
  static library kineto_LIBRARY-NOTFOUND not found.
Call Stack (most recent call first):
  $PREFIX/share/cmake/Torch/TorchConfig.cmake:120 (append_torchlib_if_found)
  CMakeLists.txt:3 (find_package)

Both of these aren't killer arguments, but it would probably be safer to put it into $PREFIX/lib / %LIBRARY_LIB%. The reason being that even if libtorch{,_python}.so etc. are shared, the interaction with static libraries is weird. We may be used to the mental model that static libraries get "fully consumed" during the build and aren't required anymore afterwards (which is fine for executables), but this isn't true for libraries, which will generally still need to get any required symbols from the static library at link time (the only real work-around there would be building OBJECT libraries and then yet more processing)

So whether or not this is necessary depends a bit how kineto is used within the torch library, but the technically correct approach would be to ship it IMO. Even if it were only to fix the warning that would already make sense from my POV.

OTOH, kineto also seems to be the reason why a vendored libfmt.a gets built in the first place:

-- Kineto: FMT_SOURCE_DIR = $SRC_DIR/third_party/fmt

and I certainly don't want to ship that (nor is it clear if kineto's fmt matches the one in our pinning which ends up used by pytorch).

Somewhat surprisingly though, libkineto.a never gets installed on unix despite us doing

mv build/lib.*/torch/lib/* ${PREFIX}/lib/

Perhaps the upstream build just never installs these. It's not obvious to me though why that would be, but I haven't dug further than

  if(NOT TARGET kineto)
    add_subdirectory("${KINETO_SOURCE_DIR}")
    set_property(TARGET kineto PROPERTY POSITION_INDEPENDENT_CODE ON)
  endif()

It appears this branch is never taken, because as soon as kineto is added as a subdirectory, it should trigger the installation.

@mgorny
Copy link
Contributor

mgorny commented Jan 28, 2025

Does dynamic linking on Windows require only the .lib file for the immediate .dll, or for its dependencies as well?

@h-vetinari
Copy link
Member Author

As always, I'm less sure on windows. But pretty sure the "cannot absorb a static library in another library (unless with OBJECT and much hacking)" also exists on windows.

@isuruf
Copy link
Member

isuruf commented Jan 29, 2025

Does dynamic linking on Windows require only the .lib file for the immediate .dll, or for its dependencies as well?

Immediate DLL only.

@shermansiu
Copy link

Interestingly enough, the PyTorch team actually shipped their Conda builds without Kineto (USE_KINETO=0). See pytorch/pytorch#51026 for details.

@shermansiu
Copy link

shermansiu commented Feb 14, 2025

In conda-forge/staged-recipes#28931, we noticed that Kineto was one of the requirements for building vllm.

Edit: It turns out, that was not the core issue that caused the build to crash. It was able to be built successfully without explicitly including or excluding Kineto.

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

No branches or pull requests

4 participants