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.

Sign up for GitHub

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

Jump to bottom

Make->CMake #10663

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make->CMake #10663

wants to merge 1 commit into from
+8 −6

Conversation

jboero
Copy link
Contributor

@jboero jboero commented Dec 4, 2024

Makefile is being deprecated. Stick with static builds using cmake and skip rpaths checks.

#10514

Make sure to read the contributing guidelines before submitting a PR

@jboero
Make->CMake
5293e17 
Makefile is being deprecated. Stick with static builds using cmake and skip rpaths checks.

ggerganov#10514
@jboero jboero mentioned this pull request Dec 4, 2024
Merged
@github-actions github-actions bot added the devops improvements to build systems and github actions label Dec 4, 2024
slaren
slaren reviewed Dec 4, 2024
View reviewed changes
.devops/llama-cpp.srpm.spec
Comment on lines +45 to +46
cd build/bin
ls
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is. Since the build uses a snapshot of master it's really helpful to list the bin contents in build logs. People keep changing build outputs etc and that helps me troubleshoot automation. Valid?

slaren
slaren reviewed Dec 4, 2024
View reviewed changes
.devops/llama-cpp.srpm.spec
@@ -34,13 +37,14 @@ Models are not included in this package and must be downloaded separately.
%setup -n llama.cpp-master

%build
make -j
cmake -B build -DBUILD_SHARED_LIBS=0
cmake --build build --config Release -j$(nproc)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you only want to bundle llama-server, add -t llama-server to build only that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes a good point. I started to package the libs but it got ugly and I was debating spinning off a separate RPM for libs. Open to suggestions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know how this RPM is intended to be used. If it is meant to be something that distributions can use to bundle a llama.cpp package, it is definitely better to use shared libs and bundle multiple version of the CPU backend as I already pointed before.
Honestly I am not sure why this file is here in the first place. It looks like something that should be maintained by the distro maintainer. Can you explain what is the purpose of this file?

@jboero
Copy link
Contributor Author

jboero commented Dec 4, 2024 via email

@slaren
Copy link
Collaborator

slaren commented Dec 4, 2024

Then it is your decision what you want to include in your distro package. But I still don't see why this needs to be in the llama.cpp repository.

@jboero
Copy link
Contributor Author

jboero commented Dec 5, 2024 via email •
edited
Loading

@slaren
Copy link
Collaborator

slaren commented Dec 5, 2024 •
edited
Loading

We don't distribute RPM packages, and I don't think it should be our responsibility to do so or to decide what goes into them. We do distribute docker images. From your own link, I can see that llama.cpp is the only of these packages that has the .spec in the source repository, in other cases you maintain a .spec file in your own repository. Why should llama.cpp be different?

@jboero
Copy link
Contributor Author

jboero commented Dec 5, 2024 via email

@slaren
Copy link
Collaborator

slaren commented Dec 11, 2024

Would you be interested in creating a full set of packages for llama.cpp and ggml that follow the Fedora packaging guidelines? This would imply creating different packages for libggml, libllama, the llama.cpp examples, and possibly additional packages for the different backends, as well as developer packages for the libraries. This may require changes to llama.cpp, but I would be willing to work with you on solving any issues that may appear. This could be used to build official packages for RPM-based systems, and could also serve as an example of how to package llama.cpp for other distro maintainers. In the current state, I don't think it should remain in this repository.

@jboero
Copy link
Contributor Author

jboero commented Dec 11, 2024 via email

@slaren
Copy link
Collaborator

slaren commented Dec 11, 2024

That would be great. It's ok if the CUDA or other backends cannot be included in every distribution, but other backends like Vulkan should be ok and would be useful to many people. Over time it would be good to have packages for all the backends, but that can be done progressively. Let me know if you need any help to get this started. Some tips:

  • Building with GGML_BACKEND_DL enabled will allow installing additional backends as shared libraries that are loaded at runtime. This would allow having a single libggml and libllama package, that optionally can be extended by installing additional backend packages.
  • Building with GGML_CPU_ALL_VARIANTS will cause multiple versions of the CPU backend optimized for different processors to be built. This is the best way to support a wide variety of processors with just one package, and should be part of the libggml package. However, there may be some issues with the library search path, since currently ggml only looks for loadable backends in the current directory and the directory of the executable, but it may be necessary to search in the system library directories as well.

@jboero
Copy link
Contributor Author

jboero commented Dec 13, 2024

OK I agree. Starting work on this now.
It's a shame the GGML backend is built statically with env variables instead of having different library names. This way each lib RPM will need a conflictswith declaration as people can't install multiple versions of a lib with the same name. Then dnf/yum with an --alowerasing will swap out the backend lib. It would be great if there were a libggml-cuda.so, libggml-rocm.so, etc so they could be selected at runtime. Anyway that's a much bigger refactor. Will just split out packages for libs on cpu, cuda, and rocm. Last I tried on a Fedora MacBook the metal backend wasn't quite supported yet.

Does that sound good? Then in theory llama-server can be a universal package as long as the package versions match. I've been using YYYYMMDD for package version as there doesn't seem to be standard versioning in releases.

@jboero
Copy link
Contributor Author

jboero commented Dec 14, 2024

Actually can anybody advise me the best way to inline or build with the CMAKE_SKIP_RPATH flag? All I can manage is hacky scripted edit of CMakeLists.txt. This was one reason I never switched the SRPMs to CMake (my distaste for CMake is no secret). Building the RPMs you can't do cmake install without root to access the system paths /usr/bin etc so setting rpath directly becomes problematic. Looking at patchelf etc which also feels like a hacky afterthought.

@max-krasnyansky
Copy link
Collaborator

max-krasnyansky commented Dec 14, 2024 •
edited
Loading

Actually can anybody advise me the best way to inline or build with the CMAKE_SKIP_RPATH flag? All I can manage is hacky scripted edit of CMakeLists.txt. This was one reason I never switched the SRPMs to CMake (my distaste for CMake is no secret). Building the RPMs you can't do cmake install without root to access the system paths /usr/bin etc so setting rpath directly becomes problematic. Looking at patchelf etc which also feels like a hacky afterthought.

Sorry if I missed other context.

cmake -D CMAKE_INSTALL_RPATH="$ORIGIN;<other-paths>" -B build ...
cmake --build buld
cmake --install build --prefix /tmp/whatever/...

btw We could also add native support for CPack if there is interest.
It's been a while since I used the RPM packager, the DEB package works great.

@jboero
Copy link
Contributor Author

jboero commented Dec 14, 2024 •
edited
Loading

cmake -D CMAKE_INSTALL_RPATH="$ORIGIN;<other-paths>" -B build ...
cmake --build build
cmake --install build --prefix /tmp/whatever/...

Thanks for that - I knew it had to be something simple but CMake man pages and tab autocompletion weren't showing me any love. I should be able to insert the build env vars for that to be universal. I will need to skip the install as rpmbuild sets paths and does the install step itself to manage rpmdb.

Happy to coordinate with CPack. At HashiCorp we settled on FPM/Effing Package Manager too. I kept things simple with rpm specs which can be easily uploaded to Fedora build automation and integration tested across the supported Fedora/Enterprise Linux platforms.

@max-krasnyansky
Copy link
Collaborator

cmake -D CMAKE_INSTALL_RPATH="$ORIGIN;<other-paths>" -B build ...
cmake --build build
cmake --install build --prefix /tmp/whatever/...

Thanks for that - I knew it had to be something simple but CMake man pages and tab autocompletion weren't showing me any love. I should be able to insert the build env vars for that to be universal. I will need to skip the install as rpmbuild sets paths and does the install step itself to manage rpmdb.

You could write custom %build and %install sections in the .spec, no?

Happy to coordinate with CPack. At HashiCorp we settled on FPM/Effing Package Manager too. I kept things simple with rpm specs which can be easily uploaded to Fedora build automation and integration tested across the supported Fedora/Enterprise Linux platforms.

Ah, the FPM is great! Used it in the past as well.
I wish they did it in Python3.

@jboero
Copy link
Contributor Author

jboero commented Dec 14, 2024 •
edited
Loading

You could write custom %build and %install sections in the .spec, no?

You could but then cmake becomes a prerequisite for simply installing a pre-built binary which isn't ideal. I'm pretty sure packaging guidelines recommend against dev tool prereqs for pre-built binaries to keep things simple. The build containers in COPR are pretty flexible - should be user and $HOME neutral. I'm traveling today but will test out your CMake suggestion against COPR build environments in a day or two.

@slaren
Copy link
Collaborator

slaren commented Dec 14, 2024

It's a shame the GGML backend is built statically with env variables instead of having different library names. This way each lib RPM will need a conflictswith declaration as people can't install multiple versions of a lib with the same name.

That's not the case. That's what I have been trying to tell you, if you use GGML_BACKEND_DL to build a backend, it is just a dynamic library that can be loaded at runtime. Then you only need a package for the base ggml library, and additional packages for each backend that are installed together with the ggml library package.

@jboero
Copy link
Contributor Author

jboero commented Dec 14, 2024 via email

@Man2Dev
Copy link
Contributor

Man2Dev commented Dec 14, 2024 •
edited
Loading

Would you be interested in creating a full set of packages for llama.cpp and ggml that follow the Fedora packaging guidelines? This would imply creating different packages for libggml, libllama, the llama.cpp examples, and possibly additional packages for the different backends, as well as developer packages for the libraries. This may require changes to llama.cpp, but I would be willing to work with you on solving any issues that may appear. This could be used to build official packages for RPM-based systems, and could also serve as an example of how to package llama.cpp for other distro maintainers. In the current state, I don't think it should remain in this repository.

Hello, I'm Mohammadreza, one of the Fedora maintainers for the llama.cpp and whisper.cpp packages.

A while back, I started working on a new spec file to update llama.cpp to its latest version in fedora. However, due to significant changes in the project, this required a complete rewrite to address the issues effectively. During this process, I managed to segment and resolve several packaging challenges in the llama.cpp package.

I saw your offer to collaborate on creating a full set of packages for llama.cpp and GGML following Fedora's packaging guidelines. Would it be possible to extend this offer to me as well? I’d love to work with you to improve the integration of llama.cpp into the Fedora mainline repository. While we don’t support the CUDA backend Hello, I'm Mohammadreza, one of the Fedora maintainers for the llama.cpp and whisper.cpp packages.

A while back, I started working on a new spec file to update llama.cpp to its latest version. However, due to significant changes in the project, this required a complete rewrite to address the issues effectively. During this process, I managed to segment and resolve several packaging challenges in the llama.cpp package.

I saw your offer to collaborate on creating a full set of packages for llama.cpp and GGML following Fedora's packaging guidelines. Would it be possible to extend this offer to me as well? I’d love to work with you to improve the integration of llama.cpp into the Fedora mainline repository. While we don’t support the CUDA backend in Fedora mainline, we do have full ROCm support, partial Vulkan support, and other smaller acceleration frameworks.

@slaren
Copy link
Collaborator

slaren commented Dec 15, 2024

I saw your offer to collaborate on creating a full set of packages for llama.cpp and GGML following Fedora's packaging guidelines. Would it be possible to extend this offer to me as well? I’d love to work with you to improve the integration of llama.cpp into the Fedora mainline repository. While we don’t support the CUDA backend in Fedora mainline, we do have full ROCm support, partial Vulkan support, and other smaller acceleration frameworks.

Yes, of course. I wasn't aware of these Fedora packages in the mainline repository, I would be happy to work with you to improve them. I think it would be good for developers and users if we could have a set of packages for ggml (and the backends) and llama.cpp that other applications can use. This will require changes to ggml and llama.cpp to achieve this, but I think it is worth the effort.

@max-krasnyansky suggestion of using CPack looks very interesting to me. If this works well, it could be very useful to generate all kinds of packages of llama.cpp. I don't have any experience working with CPack, so I would appreciate any insight about it, especially if there are any known issues that may prevent using it to create packages for Fedora or other distributions.

@jboero
Copy link
Contributor Author

jboero commented Dec 15, 2024 •
edited
Loading

The mainlined llama-cpp package is news to me too. It only seems to have libggml.so and libllama.so with build ID and no symlinks for lib*.so.1.

There also seems to be an outstanding PR for the old refactor from LLAMA_* to GGML_*. Maybe the package should be renamed libllama as part of the refactor. There does seem to be a llama-cpp-devel package for headers. @Man2Dev can you or trix please add me to members on that package? I can convert my old COPR spec here to llama-cpp-server or ggml-server to submit to mainline if we get this right. I'll also have to add support for GGML_BACKEND_DL as described above.

libggml-common
   llama-server
   libggml-backend-*
libggml-devel (headers)

@Man2Dev
Copy link
Contributor

Man2Dev commented Dec 15, 2024

The mainlined llama-cpp package is news to me too. It only seems to have libggml.so and libllama.so with build ID and no symlinks for lib*.so.1.

There also seems to be an outstanding PR for the old refactor from LLAMA_* to GGML_*. Maybe the package should be renamed libllama as part of the refactor. There does seem to be a llama-cpp-devel package for headers. @Man2Dev can you or trix please add me to members on that package? I can convert my old COPR spec here to llama-cpp-server or ggml-server to submit to mainline if we get this right. I'll also have to add support for GGML_BACKEND_DL as described above.

libggml-common
   llama-server
   libggml-backend-*
libggml-devel (headers)

GGML_BACKEND_DL and some of the segmentation of the acceleration frameworks have already been added. I haven't open PR for them since it's not done and I'm waiting for ROCm 6.3 to land. https://src.fedoraproject.org/fork/man2dev/rpms/llama-cpp

@Man2Dev
Copy link
Contributor

Man2Dev commented Dec 15, 2024

The mainlined llama-cpp package is news to me too. It only seems to have libggml.so and libllama.so with build ID and no symlinks for lib*.so.1.

There also seems to be an outstanding PR for the old refactor from LLAMA_* to GGML_*. Maybe the package should be renamed libllama as part of the refactor. There does seem to be a llama-cpp-devel package for headers. @Man2Dev can you or trix please add me to members on that package? I can convert my old COPR spec here to llama-cpp-server or ggml-server to submit to mainline if we get this right. I'll also have to add support for GGML_BACKEND_DL as described above.

libggml-common
   llama-server
   libggml-backend-*
libggml-devel (headers)

Its up to package owner to add new members. If you are interestedin working on the llama-cpp package or any other AI related packages please join the ai/ml matrix room there you can talk to rest of the members

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slaren slaren slaren left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
devops improvements to build systems and github actions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jboero @slaren @max-krasnyansky @Man2Dev