llama-cpp: pull upstream flake changes #289513

happysalada · 2024-02-17T11:49:26Z

Description of changes

I've basically copied the upstream flake.
we had a discussion with @SomeoneSerge about removing the custom install step.
The idea was that using the normal cmake install step uses the upstream file. Whereas using a custom install step, we just loose upstream logic and we just do a worse job at it.

@elohmeier
I've kept the naming of cudaSupport instead of upstream usecuda since it's the norm in nixpkgs, but I don't feel that strongly about it.

Things done

Add a 👍 reaction to pull requests you find important.

pkgs/by-name/ll/llama-cpp/package.nix

SomeoneSerge · 2024-02-17T13:10:37Z

pkgs/by-name/ll/llama-cpp/package.nix

+        # Should likely use `rocmPackages.clr.gpuTargets`.
+        "-DAMDGPU_TARGETS=gfx803;gfx900;gfx906:xnack-;gfx908:xnack-;gfx90a:xnack+;gfx90a:xnack-;gfx940;gfx941;gfx942;gfx1010;gfx1012;gfx1030;gfx1100;gfx1101;gfx1102"


The comment is from the upstream flake, I guess it's ok to just "pull those changes", but consider following up on the suggestion from the comments in a separate commit

the xnack- present on some of these would make it difficult to abstract away
Happy to follow up with a separate PR when this has been resolved exactly and tested.

ghost · 2024-02-18T10:17:23Z

see also #287554 -- think there might be some overlap.

happysalada · 2024-02-18T11:17:57Z

Thank you for letting me know, ill let them know.

mschwaig · 2024-02-18T20:36:39Z

Thanks for asking me about this PR @happysalada, and about moving forward with this instead of mine.
In theory that's fine for me, but there's a few things I noticed about this PR in its current state that I want to mention:

If you are making such significant changes to a package, please give a short written explanation somewhere as part of the PR which explains what the reasons for and benefits of that change are. Saying discussions have been had with person X and Y helps a bit, but by itself it maybe makes the process feel even more opaque to readers of the PR, which I don't think should be acceptable. When I look at this PR I feel kind of left/locked out of the process.

As is, by getting rid of that custom installation step,

~~you're removing the /lib and /include folders. I can only assume that those are obsolete now or that the PR is not done yet, which you could clarify somewhere or mark it as a draft, and~~
besides breaking the interface of the package, not renaming the content of the /bin folder as it was done previously looks like it will create issues if people put the package on their path, since it contains binaries with names like lookup and simple.

The other side of that issue of renaming binaries is that with whats currently on master it's actually really annoying to switch between something like ollama with llama.cpp from the upstream flake and nixpkgs, because of the different binary names. If you think about renaming all of the outputs, it would be great if you could look into addressing that issue as well.
I have been using this change to get around that: 9afa045

mschwaig · 2024-02-18T20:51:00Z

pkgs/by-name/ll/llama-cpp/package.nix

+  # upstream plans on adding targets at the cmakelevel, remove those
+  # additional steps after that
+  postInstall = ''
+    mv $out/bin/main $out/bin/llama


this breaks meta.mainProgram

really annoying to switch between something like ollama with llama.cpp from the upstream flake and nixpkgs, because of the different binary names

To be fair, it was the upstream's choice to use non-composable/generic names (these won't do for nixpkgs, imo). 👍🏻 that the name should be consistent with whatever meta.mainProgram is kept

How about this?

Suggested change

mv $out/bin/main $out/bin/llama

mv $out/bin/main $out/bin/${finalAttrs.finalPackage.meta.mainProgram}

pkgs/by-name/ll/llama-cpp/package.nix

SomeoneSerge · 2024-02-18T22:02:50Z

pkgs/by-name/ll/llama-cpp/package.nix

you're removing the /lib and /include folders

I didn't notice that, where? Libraries and headers are installed by CMake

I built it locally and went by the output of tree to check. Embarrassingly I read that wrong. I will update my comment.

Actually the previous version commit only has /bin, and additionally /lib for libggml_shared.so and libllama.so with .override{ static=false; }.
This PR gets rid of the flag and has some extra stuff in liband adds a /include:

result ├── bin │ ├── baby-llama │ ├── batched │ ├── batched-bench │ ├── beam-search │ ├── benchmark │ ├── convert-llama2c-to-ggml │ ├── convert-lora-to-ggml.py │ ├── convert.py │ ├── embedding │ ├── export-lora │ ├── finetune │ ├── gguf │ ├── imatrix │ ├── infill │ ├── llama │ ├── llama-bench │ ├── llama-server │ ├── llava-cli │ ├── lookahead │ ├── lookup │ ├── parallel │ ├── passkey │ ├── perplexity │ ├── quantize │ ├── quantize-stats │ ├── save-load-state │ ├── simple │ ├── speculative │ ├── test-autorelease │ ├── test-backend-ops │ ├── test-grad0 │ ├── test-grammar-parser │ ├── test-llama-grammar │ ├── test-model-load-cancel │ ├── test-quantize-fns │ ├── test-quantize-perf │ ├── test-rope │ ├── test-sampling │ ├── test-tokenizer-0-falcon │ ├── test-tokenizer-0-llama │ ├── test-tokenizer-1-bpe │ ├── test-tokenizer-1-llama │ ├── tokenize │ └── train-text-from-scratch ├── include │ ├── ggml-alloc.h │ ├── ggml-backend.h │ ├── ggml.h │ └── llama.h └── lib ├── cmake │ └── Llama │ ├── LlamaConfig.cmake │ └── LlamaConfigVersion.cmake ├── libggml_shared.so ├── libllama.so └── libllava_shared.so

Tbh I only checked the upstream flake yet. I'll have a look at both of the PRs during the week

SomeoneSerge · 2024-02-18T22:05:03Z

pkgs/by-name/ll/llama-cpp/package.nix

The other side of that issue of renaming binaries is that with whats currently on master it's actually really annoying to switch between something like ollama with llama.cpp from the upstream flake and nixpkgs, because of the different binary names

That's an interesting thought 👍🏻 . Since we in principle can contribute both to the nixpkgs and to the upstream nix expressions, we could expose all of the relevant metadata in the the passthru (or even in the outputs if needed) to accommodate smooth transition

SomeoneSerge · 2024-02-18T22:07:10Z

pkgs/by-name/ll/llama-cpp/package.nix

put the package on their path, since it contains binaries with names like lookup and simple

To ensure this doesn't bite us in the future, we could propose a PR upstream adding a cmake option that prefixes all of the installed binaries (the default prefix being the empty string)

That's a good idea. 👍

philiptaron · 2024-02-21T18:45:14Z

ggerganov/llama.cpp#5311 has been merged and Vulkan works on macOS now.

philiptaron

Please integrate the changes from ggerganov/llama.cpp#5311, then we can test on x86_64-linux, x86_64-darwin, and aarch64-darwin, ensure it works, then check it in.

https://github.com/NixOS/nixpkgs/pull/289513/files#r1493855899 also needs to be addressed in the nixpkgs context.

SomeoneSerge · 2024-02-23T10:57:26Z

pkgs/by-name/ll/llama-cpp/package.nix

I'm maybe not following, but why integrating the Vulkan PR should be a blocker for merging this? I wouldn't mind if this was a separate PR, depending on whether it's easier to pull this together with or without the Vulkan enhancement.

@mschwaig's concerns need to be resolved indeed. Btw, @happysalada do you still have time for this?

philiptaron · 2024-02-23T16:46:15Z

My goal is that macOS Nix users have the ability to get the accelerated Vulkan build. As @mschwaig notes, with the current revision selected, vulkan needs a restriction that gates it to Linux only. Since this PR doesn't set meta.broken on Darwin in the first place if Vulkan support is specified, it only needs to be moved to a newer rev of llama.cpp in order to implement this feedback, I believe.

Super simple stuff, I think and hope.

happysalada · 2024-02-23T17:14:01Z

Im happy to make the update at the same time, should have time for this saturday morning. Recently day job is crazy during the week.

happysalada · 2024-02-24T14:53:57Z

Alright I've updated to latest 2252, I've rephrased the PR text, and I've added vulcan in the the non broken logic.

If anyone has any other comments on this PR please let me know.

Separately I have a question for @SomeoneSerge .
We had a discussion about ollama, that is doing a custom install step hardcoding a path for llama-cpp. You rightfully suggested that they should use pkg-config to locate the .so files. In order for ollama to be able to use package config, llama-cpp has to define a .pc file, correct ?
(you'll have to excuse my lack of knowledge in the c++/c build tooling system).

philiptaron

Requesting changes for the meta.mainProgram thing and the meta.broken and meta.badPlatforms keys; a bunch of super small other things are also noted.

pkgs/by-name/ll/llama-cpp/package.nix

philiptaron · 2024-02-26T22:30:37Z

pkgs/by-name/ll/llama-cpp/package.nix

+    cuda_cccl.dev # <nv/target>
+
+    # A temporary hack for reducing the closure size, remove once cudaPackages
+    # have stopped using lndir: https://github.com/NixOS/nixpkgs/issues/271792


It's on the Roadmap 🔮!

philiptaron · 2024-02-26T22:32:15Z

pkgs/by-name/ll/llama-cpp/package.nix

 in
 effectiveStdenv.mkDerivation (finalAttrs: {
  pname = "llama-cpp";
-  version = "2249";
+  version = "2252";


Wow, we're already at 2275 just over the weekend 👀 . No need to adjust, but dang, this is why flakes enable Nix support in fast-moving repositories!

philiptaron · 2024-02-26T22:33:30Z

pkgs/by-name/ll/llama-cpp/package.nix

  postPatch = ''
    substituteInPlace ./ggml-metal.m \
      --replace '[bundle pathForResource:@"ggml-metal" ofType:@"metal"];' "@\"$out/bin/ggml-metal.metal\";"
  '';


Unrelated to your change: this is such an odd line and I wonder why the Nix build needs it. I'm no Metal-head, though.

philiptaron · 2024-02-26T22:34:24Z

pkgs/by-name/ll/llama-cpp/package.nix

+  buildInputs = optionals effectiveStdenv.isDarwin darwinBuildInputs
+    ++ optionals cudaSupport cudaBuildInputs
+    ++ optionals mpiSupport mpi
+    ++ optionals openclSupport [ clblast ]
+    ++ optionals rocmSupport rocmBuildInputs
+    ++ optionals vulkanSupport vulkanBuildInputs;


pkgs/by-name/ll/llama-cpp/package.nix

philiptaron · 2024-02-26T22:41:43Z

pkgs/by-name/ll/llama-cpp/package.nix

+  # upstream plans on adding targets at the cmakelevel, remove those
+  # additional steps after that
+  postInstall = ''
+    mv $out/bin/main $out/bin/llama


How about this?

Suggested change

mv $out/bin/main $out/bin/llama

mv $out/bin/main $out/bin/${finalAttrs.finalPackage.meta.mainProgram}

pkgs/by-name/ll/llama-cpp/package.nix

philiptaron · 2024-02-26T22:45:49Z

pkgs/by-name/ll/llama-cpp/package.nix

@@ -146,7 +156,7 @@ effectiveStdenv.mkDerivation (finalAttrs: {
    license = licenses.mit;


The description also wants to be updated from the upstream: Inference of LLaMA model in pure C/C++

I like the pnameSuffix and descriptionSuffix things that upstream does as well, which I note you chose not to port over.

I was a little lazy, and wasn't 100% sure it was going to be accepted. I wanted to make the smallest change possible. How about we do that in a follow up PR ?

Yeah, I have no strong feelings about it being in this PR.

happysalada · 2024-02-29T00:45:25Z

Alright I went through the comments and fixed what was easy/simple to fix and brought the update to 2294.
Let me know if anything else.

philiptaron

Result of nixpkgs-review pr 289513 run on x86_64-linux 1

1 package built:

llama-cpp

philiptaron · 2024-02-29T00:46:47Z

pkgs/by-name/ll/llama-cpp/package.nix

@@ -146,7 +156,7 @@ effectiveStdenv.mkDerivation (finalAttrs: {
    license = licenses.mit;


Yeah, I have no strong feelings about it being in this PR.

philiptaron

Thanks @GrahamcOfBorg, I'm still saying yes.

happysalada · 2024-02-29T11:27:51Z

@SomeoneSerge @philiptaron if any of you has time / availability to add a .pc file to llama-cpp ill be happy to try to propose using pkg-config to ollama . currently ollama is broken on nixpkgs because they want the lib files at specific places. Changing to pkg-config would fix that.

philiptaron · 2024-02-29T16:42:23Z

@SomeoneSerge @philiptaron if any of you has time / availability to add a .pc file to llama-cpp ill be happy to try to propose using pkg-config to ollama . currently ollama is broken on nixpkgs because they want the lib files at specific places. Changing to pkg-config would fix that.

Agree, but I am a complete novice when it comes to pkg-config; the task is better suited for those who know how to use that means of ascent. I've got commit rights over in ggerganov/llama.cpp, so if you (or anyone here!) uncorks a patch, I'll see what I can do to get it merged.

happysalada requested review from elohmeier and SomeoneSerge February 17, 2024 11:49

happysalada force-pushed the update_llama_cpp branch from 18cefab to b71cce1 Compare February 17, 2024 11:52

SomeoneSerge reviewed Feb 17, 2024

View reviewed changes

pkgs/by-name/ll/llama-cpp/package.nix Outdated Show resolved Hide resolved

SomeoneSerge reviewed Feb 17, 2024

View reviewed changes

pkgs/by-name/ll/llama-cpp/package.nix Outdated Show resolved Hide resolved

SomeoneSerge reviewed Feb 17, 2024

View reviewed changes

pkgs/by-name/ll/llama-cpp/package.nix Outdated Show resolved Hide resolved

SomeoneSerge reviewed Feb 17, 2024

View reviewed changes

happysalada force-pushed the update_llama_cpp branch from b71cce1 to b48e8c3 Compare February 17, 2024 14:11

ofborg bot requested a review from dit7ya February 17, 2024 14:43

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Feb 17, 2024

happysalada force-pushed the update_llama_cpp branch from b48e8c3 to 0fb62fd Compare February 18, 2024 09:46

happysalada requested a review from SomeoneSerge February 18, 2024 09:46

happysalada mentioned this pull request Feb 18, 2024

llama-cpp: add support for Vulkan backend #287554

Closed

13 tasks

mschwaig suggested changes Feb 18, 2024

View reviewed changes

SomeoneSerge reviewed Feb 18, 2024

View reviewed changes

philiptaron requested changes Feb 22, 2024

View reviewed changes

SomeoneSerge reviewed Feb 23, 2024

View reviewed changes

happysalada force-pushed the update_llama_cpp branch from 0fb62fd to 86e127d Compare February 24, 2024 14:50

ofborg bot added 10.rebuild-darwin: 1 10.rebuild-linux: 1 labels Feb 24, 2024

philiptaron requested changes Feb 26, 2024

View reviewed changes

llama-cpp: 2249 -> 2294; bring upstream flake

0317300

happysalada force-pushed the update_llama_cpp branch from 86e127d to 0317300 Compare February 29, 2024 00:44

philiptaron approved these changes Feb 29, 2024

View reviewed changes

ofborg bot requested a review from philiptaron February 29, 2024 01:09

philiptaron approved these changes Feb 29, 2024

View reviewed changes

happysalada merged commit cd7f814 into NixOS:master Feb 29, 2024
22 of 23 checks passed

		# Should likely use `rocmPackages.clr.gpuTargets`.
		"-DAMDGPU_TARGETS=gfx803;gfx900;gfx906:xnack-;gfx908:xnack-;gfx90a:xnack+;gfx90a:xnack-;gfx940;gfx941;gfx942;gfx1010;gfx1012;gfx1030;gfx1100;gfx1101;gfx1102"

	mv $out/bin/main $out/bin/llama
	mv $out/bin/main $out/bin/${finalAttrs.finalPackage.meta.mainProgram}

		@@ -146,7 +156,7 @@ effectiveStdenv.mkDerivation (finalAttrs: {
		license = licenses.mit;

llama-cpp: pull upstream flake changes #289513

llama-cpp: pull upstream flake changes #289513

Conversation

happysalada commented Feb 17, 2024 • edited Loading

Description of changes

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 18, 2024 • edited by ghost Loading

happysalada commented Feb 18, 2024

mschwaig commented Feb 18, 2024 • edited Loading

Choose a reason for hiding this comment

SomeoneSerge Feb 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mschwaig Feb 18, 2024 • 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

philiptaron commented Feb 21, 2024

philiptaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philiptaron commented Feb 23, 2024

happysalada commented Feb 23, 2024

happysalada commented Feb 24, 2024

philiptaron left a comment • 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

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

happysalada commented Feb 29, 2024

philiptaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philiptaron left a comment

Choose a reason for hiding this comment

happysalada commented Feb 29, 2024

philiptaron commented Feb 29, 2024

happysalada commented Feb 17, 2024 •

edited

Loading

ghost commented Feb 18, 2024 •

edited by ghost

Loading

mschwaig commented Feb 18, 2024 •

edited

Loading

SomeoneSerge Feb 18, 2024 •

edited

Loading

mschwaig Feb 18, 2024 •

edited

Loading

philiptaron left a comment •

edited

Loading