-
Notifications
You must be signed in to change notification settings - Fork 14.1k
build: for GGML_BACKEND_DL, ggml need not depend on backend #17709
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
base: master
Are you sure you want to change the base?
Conversation
| # write the shared library to the output directory | ||
| set_target_properties(${backend} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}) | ||
| target_compile_definitions(${backend} PRIVATE GGML_BACKEND_DL) | ||
| add_dependencies(ggml ${backend}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is a build order dependency and I think it should be kept. add_dependencies(A B) tells CMake that target A depends on target B meaning that whenever we build target A, CMake must first make sure that target B is built.
This makes sure that if we make a change to a backend and then try to build the ggml target, CMake will first rebuild the backend before rebuilding ggml and this keeps things consistent.
If we don't have this it would be possible that after everything is built and we have libggml-base.so, libggml.so, and libggml-vulkan.so in the build directory, and if we then make a change to the ggml-vulkan backend and try building the ggml target, cmake would check ggml and determine that it is up to date and the ggml-vulkan backend would not get rebuilt. So our changes would not be compiled. But with this build order dependency CMake will first rebuild the ggml-vulkan backend before rebuilding ggml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you're saying is that this dependency is not functionally required, but that people may be using --target ggml as a shorthand to build all ggml libraries, and for some reason not just using --target all. I'm not sure why people would do that, but if you want to preserve that kind of behavior we could add a new target for it:
diff --git a/ggml/src/CMakeLists.txt b/ggml/src/CMakeLists.txt
index 7f2665f75..fe5cf5e17 100644
--- a/ggml/src/CMakeLists.txt
+++ b/ggml/src/CMakeLists.txt
@@ -247,7 +247,11 @@ if (CMAKE_SYSTEM_NAME MATCHES "Linux")
target_link_libraries(ggml PRIVATE dl)
endif()
+add_custom_target(ggml_all)
+add_dependencies(ggml_all ggml)
+
function(ggml_add_backend_library backend)
+ add_dependencies(ggml_all ${backend})
if (GGML_BACKEND_DL)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you're saying is that this dependency is not functionally required
I actually think that this is required to allow the following:
Suppose I use ggml by including it as part of my cmake project. And I've configured GGML_BACKEND_DL=ON to generate dynamic libraries for the backends I want to use that can be loaded at runtime (as opposed to linked at build time).
With the dependency to the backend there, when I build ggml the backends will also be built. Without this dependency only libggml.so and libggml-base.so will be built which might cause issues for project using ggml in this way. Without this those project would have to explicitly build each backend or use --target all.
I'm not sure why people would do that
I think that users/projects that embed ggml might do this in a similar way as described above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The backends are built because their cmake files get included and those files call ggml_add_backend_library->add_library. It's the add_library call that tells cmake to build it, not the dependency. add_dependencies additionally enforces a build order, and that's what I want to remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add_dependencies additionally enforces a build order, and that's what I want to remove.
Yes, but I think this is something we might want to keep. My concern is the following scenario.
As an example, we first build with GGML_BACKEND_DL=ON with add_dependencies:
$ cmake --fresh -S . -B build-backends -DCMAKE_BUILD_TYPE=Debug\
-DGGML_BACKEND_DL=ON \
-DGGML_CPU_ALL_VARIANTS=ON \
-DGGML_CUDA=ON \
-DCMAKE_CUDA_ARCHITECTURES="89"
$ cmake --build build-backends -j8 --target ggml
...
[ 52%] Building CUDA object ggml/src/ggml-cuda/CMakeFiles/ggml-cuda.dir/ggml-cuda.cu.o
[ 54%] Linking CUDA shared module ../../../bin/libggml-cuda.so
[ 97%] Built target ggml-cuda
[100%] Built target ggmlNow if we remove add_dependencies and update the CUDA backend:
$ echo "something" >> ggml/src/ggml-cuda/ggml-cuda.cuAnd then build with the same command as above the following will be built:
-- Build files have been written to: /home/danbev/work/ai/llama.cpp/build-backends
[ 50%] Built target ggml-base
[100%] Built target ggmlSo the CUDA backend is not built and would not compile if it was. This is the issue I think might have consequences for some users/projects if we remove this.
But if others don't see this as an issue I'm alright with this change. I just mainly wanted to make sure we have considered this scenario.
Removing this dependency allows more of the build to run in parallel with building the backend. I don't think this dependency is necessary since the backend is dynamically linked.
Build time on my system with this change stacked on #17708.