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

llama : reorganize source code + improve CMake #8006

Merged
merged 13 commits into from
Jun 26, 2024
Merged

Conversation

ggerganov
Copy link
Owner

@ggerganov ggerganov commented Jun 19, 2024

ref #7573, #6913

Adopt a new structure of the source code that makes ggml source code and build scripts easier to reuse.

Note that the build options that are relevant to the ggml library are now prefixed with GGML_. For example, LLAMA_CUDA and WHISPER_METAL are now GGML_CUDA and GGML_METAL. However, WHISPER_COREML and WHISPER_OPENVINO are still named the same because the CoreML and OpenVINO functionality in whisper.cpp is not part of the ggml library.

The Makefiles in llama.cpp and whisper.cpp have been updated to be more similar with each other.

Header files (such as ggml.h, llama.h and whisper.h) are now placed in include subfolders, while the source files are placed in src.

PRs in other projects that will be updated and merged together with this one:

TODOs:

  • update ggml repo
  • update whisper.cpp repo
  • update Makefiles
    • llama.cpp
    • whisper.cpp
  • update sync scripts
    • llama.cpp
    • ggml
    • whisper.cpp
    • add ggml/cmake
  • fix examples
  • fix CI
    • HIPBLAS
    • kompute
    • nix (maybe will work after merge to master?)
    • windows
  • update README with new GGML_ options
    • llama.cpp
    • whisper.cpp

Changes:

  • deprecate LLAMA_XXX in favor of GGML_XXX

Resolved PRs:

TODO in follow-up PRs:

  • move relevant tests from tests to ggml/tests
  • avoid using CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and build and link shared libraries properly
  • link shared libs to examples by default
  • simplify sync scripts to work on the ggml folder level

@github-actions github-actions bot added build Compilation issues python python script changes labels Jun 19, 2024
@ggerganov ggerganov force-pushed the gg/reorganize-project branch from 729c7cc to cde2512 Compare June 19, 2024 14:53
@mofosyne mofosyne added the Review Complexity : Medium Generally require more time to grok but manageable by beginner to medium expertise level label Jun 19, 2024
@ggerganov ggerganov force-pushed the gg/reorganize-project branch 10 times, most recently from ace2b97 to 8216c4c Compare June 21, 2024 09:33
@github-actions github-actions bot added the script Script related label Jun 21, 2024
@ggerganov ggerganov force-pushed the gg/reorganize-project branch from 8216c4c to 5b1490a Compare June 21, 2024 09:34
@github-actions github-actions bot added the devops improvements to build systems and github actions label Jun 21, 2024
@ggerganov ggerganov force-pushed the gg/reorganize-project branch from c9a3dc8 to 73d5f90 Compare June 21, 2024 10:13
@github-actions github-actions bot added documentation Improvements or additions to documentation nix Issues specific to consuming flake.nix, or generally concerned with ❄ Nix-based llama.cpp deployment examples SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language labels Jun 21, 2024
@ggerganov
Copy link
Owner Author

The llama.cpp reorganization should be now mostly ready and can be tested. The ggml code, tests and cmake scripts will be shared across all 3 repos (as if it is a git submodule).

Some CI workflows are still failing - any help with resolving these is appreciated. I'll now focus on updating whisper.cpp to reuse ggml in a similar way and then look to merge this sometime next week. Extra attention to the new build options (e.g. LLAMA_CUDA is now GGML_CUDA)

@ggerganov ggerganov force-pushed the gg/reorganize-project branch from e550e3e to 7c1c378 Compare June 21, 2024 12:24
@slaren

This comment was marked as outdated.

@slaren
Copy link
Collaborator

slaren commented Jun 21, 2024

It works with -DBUILD_SHARED_LIBS=OFF, so I am probably misunderstanding the error.

@slaren
Copy link
Collaborator

slaren commented Jun 21, 2024

I noticed now that CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS is set in ggml when building on windows with shared libs. I think that the problem actually is that CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS is not set early enough, because configuring with -DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=ON also works, without requiring the changes that I listed before.

This seems to work. My assumption is that CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS needs to be set per project, unless passed through the command line, then it is applied to every subproject as well, but I am not sure if that's correct.

However, all of this seems like a hack, we go through the effort to use dllexport in ggml.h and llama.h with all the symbols that should be exported, but then we just throw all of this away and use CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS to export every symbol.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 6b9b5413..96718d75 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -6,6 +6,7 @@ include(CheckIncludeFileCXX)
 set(CMAKE_WARN_UNUSED_CLI YES)

 set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
+set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)

 if (NOT XCODE AND NOT MSVC AND NOT CMAKE_BUILD_TYPE)
     set(CMAKE_BUILD_TYPE Release CACHE STRING "Build type" FORCE)
diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt
index c6fccc02..02415f2d 100644
--- a/common/CMakeLists.txt
+++ b/common/CMakeLists.txt
@@ -1,5 +1,7 @@
 # common

+set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)
+
 find_package(Threads REQUIRED)

 # Build info header
diff --git a/ggml/CMakeLists.txt b/ggml/CMakeLists.txt
index bdbda425..21ef7e4a 100644
--- a/ggml/CMakeLists.txt
+++ b/ggml/CMakeLists.txt
@@ -3,6 +3,7 @@ project("ggml" C CXX)
 include(CheckIncludeFileCXX)

 set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
+set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)

 if (NOT XCODE AND NOT MSVC AND NOT CMAKE_BUILD_TYPE)
     set(CMAKE_BUILD_TYPE Release CACHE STRING "Build type" FORCE)
diff --git a/ggml/src/CMakeLists.txt b/ggml/src/CMakeLists.txt
index 84bc8e19..b7c79321 100644
--- a/ggml/src/CMakeLists.txt
+++ b/ggml/src/CMakeLists.txt
@@ -825,10 +825,6 @@ endif()

 if (WIN32)
     add_compile_definitions(_CRT_SECURE_NO_WARNINGS)
-
-    if (BUILD_SHARED_LIBS)
-        set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)
-    endif()
 endif()

 if (GGML_LTO)

CMakeLists.txt Outdated
" to set correct LLAMA_BLAS_VENDOR")
# override ggml options
set(GGML_CCACHE ${LLAMA_CCACHE})
set(GGML_BUILD_SHARED_LIBS ${LLAMA_BUILD_SHARED_LIBS})
Copy link
Collaborator

@slaren slaren Jun 21, 2024

Choose a reason for hiding this comment

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

GGML_BUILD_SHARED_LIBS and LLAMA_BUILD_SHARED_LIBS do not exist, BUILD_SHARED_LIBS is used directly.

@mofosyne
Copy link
Collaborator

if you are reorganizing source codes, I would like to suggest moving some compiled tools from the example folder into a dedicated folder. It's not quite a script, but in my opinion its not examples as well... considering it's more for maintainers internal usage

@ggerganov ggerganov merged commit f3f6542 into master Jun 26, 2024
1 check passed
@ggerganov ggerganov deleted the gg/reorganize-project branch June 26, 2024 15:33
@OuadiElfarouki
Copy link
Contributor

@slaren The LLAMA_CUDA_FORCE_CUBLAS cmake option got mistakenly removed but is still used. I believe it's intended to be mutually exclusive with GGML_CUDA_FORCE_MMQ so some changes might be needed (cmake, mmq.cu, ggml-cuda.cu ..)

@ggerganov
Copy link
Owner Author

ggerganov commented Jun 26, 2024

There is now GGML_CUDA_FORCE_CUBLAS

Edit: nvm #8140

mudler added a commit to mudler/LocalAI that referenced this pull request Jun 27, 2024
mudler added a commit to mudler/LocalAI that referenced this pull request Jun 27, 2024
mudler added a commit to mudler/LocalAI that referenced this pull request Jun 27, 2024
mudler added a commit to mudler/LocalAI that referenced this pull request Jun 27, 2024
loonerin added a commit to loonerin/llama.cpp that referenced this pull request Jun 27, 2024
PR ggerganov#8006 changes defaults to build shared libs. However, CI for release
builds expects static builds.
loonerin added a commit to loonerin/llama.cpp that referenced this pull request Jun 27, 2024
PR ggerganov#8006 changes defaults to build shared libs. However, CI for releases
expects static builds.
slaren pushed a commit that referenced this pull request Jun 27, 2024
* CI: fix release build (Ubuntu)

PR #8006 changes defaults to build shared libs. However, CI for releases
expects static builds.

* CI: fix release build (Mac)

---------

Co-authored-by: loonerin <[email protected]>
mudler added a commit to mudler/LocalAI that referenced this pull request Jun 27, 2024
* arrow_up: Update ggerganov/llama.cpp

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: Ettore Di Giacinto <[email protected]>

* deps(llama.cpp): update build variables to follow upstream

Update build recipes with ggerganov/llama.cpp#8006

Signed-off-by: Ettore Di Giacinto <[email protected]>

* Disable shared libs by default in llama.cpp

Signed-off-by: Ettore Di Giacinto <[email protected]>

* Disable shared libs in llama.cpp Makefile

Signed-off-by: Ettore Di Giacinto <[email protected]>

* Disable metal embedding for now, until it is tested

Signed-off-by: Ettore Di Giacinto <[email protected]>

* fix(mac): explicitly enable metal

Signed-off-by: Ettore Di Giacinto <[email protected]>

* debug

Signed-off-by: Ettore Di Giacinto <[email protected]>

* fix typo

Signed-off-by: Ettore Di Giacinto <[email protected]>

---------

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: Ettore Di Giacinto <[email protected]>
Co-authored-by: mudler <[email protected]>
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Jun 30, 2024
* scripts : update sync [no ci]

* files : relocate [no ci]

* ci : disable kompute build [no ci]

* cmake : fixes [no ci]

* server : fix mingw build

ggml-ci

* cmake : minor [no ci]

* cmake : link math library [no ci]

* cmake : build normal ggml library (not object library) [no ci]

* cmake : fix kompute build

ggml-ci

* make,cmake : fix LLAMA_CUDA + replace GGML_CDEF_PRIVATE

ggml-ci

* move public backend headers to the public include directory (ggerganov#8122)

* move public backend headers to the public include directory

* nix test

* spm : fix metal header

---------

Co-authored-by: Georgi Gerganov <[email protected]>

* scripts : fix sync paths [no ci]

* scripts : sync ggml-blas.h [no ci]

---------

Co-authored-by: slaren <[email protected]>
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Jun 30, 2024
* CI: fix release build (Ubuntu)

PR ggerganov#8006 changes defaults to build shared libs. However, CI for releases
expects static builds.

* CI: fix release build (Mac)

---------

Co-authored-by: loonerin <[email protected]>
MagnusS0 pushed a commit to MagnusS0/llama.cpp-normistral-tokenizer that referenced this pull request Jul 1, 2024
* scripts : update sync [no ci]

* files : relocate [no ci]

* ci : disable kompute build [no ci]

* cmake : fixes [no ci]

* server : fix mingw build

ggml-ci

* cmake : minor [no ci]

* cmake : link math library [no ci]

* cmake : build normal ggml library (not object library) [no ci]

* cmake : fix kompute build

ggml-ci

* make,cmake : fix LLAMA_CUDA + replace GGML_CDEF_PRIVATE

ggml-ci

* move public backend headers to the public include directory (ggerganov#8122)

* move public backend headers to the public include directory

* nix test

* spm : fix metal header

---------

Co-authored-by: Georgi Gerganov <[email protected]>

* scripts : fix sync paths [no ci]

* scripts : sync ggml-blas.h [no ci]

---------

Co-authored-by: slaren <[email protected]>
MagnusS0 pushed a commit to MagnusS0/llama.cpp-normistral-tokenizer that referenced this pull request Jul 1, 2024
* CI: fix release build (Ubuntu)

PR ggerganov#8006 changes defaults to build shared libs. However, CI for releases
expects static builds.

* CI: fix release build (Mac)

---------

Co-authored-by: loonerin <[email protected]>
EZForever added a commit to EZForever/llama.cpp-static that referenced this pull request Jul 18, 2024
brittlewis12 added a commit to brittlewis12/llama-cpp-rs that referenced this pull request Jul 25, 2024
brittlewis12 added a commit to brittlewis12/llama-cpp-rs that referenced this pull request Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Compilation issues devops improvements to build systems and github actions documentation Improvements or additions to documentation examples nix Issues specific to consuming flake.nix, or generally concerned with ❄ Nix-based llama.cpp deployment python python script changes Review Complexity : Medium Generally require more time to grok but manageable by beginner to medium expertise level script Script related server SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language testing Everything test related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants