Skip to content

Conversation

SanftMonster
Copy link
Collaborator

Add llama.cpp as a submodule here for some reason:

  1. It's easy for users to find which commit they should head to if they want to compile native library themselves.
  2. It makes it more convenient for development based on llama.cpp in LLamaSharp.

@zsogitbe
Copy link
Contributor

This will be great! We can then try to make 1 VS solution which compiles the C++ and C# code together with the chosen configuration and with 1 click.

@SanftMonster
Copy link
Collaborator Author

SanftMonster commented Mar 15, 2024

@SignalRT The ci of windows has failed after 3 times of retry. Is that the same issue with #599, I guess? If that's confirmed a known issue, I'll merge this PR without verification.

@martindevans
Copy link
Member

Can this new VS Solution use the make build scripts included in llama.cpp? I don't really know much about cpp in VS so maybe that integrates easily.

Asking because at the moment our build is relatively simple because we just clone the llama.cpp repo and call cmake - no maintenance work is required to keep our build up to date. Don't even need cmake or any other build stuff setup and installed locally, because it's all setup to run in GitHub actions. I'd rather not change either of these properties if possible.

@zsogitbe
Copy link
Contributor

I think that both solutions could exist next to each other in the future, if you choose to do so. Many people like to compile the C++ code themselves, for example for setting some special properties (e.g., AVX2 + CUDA 12), or adding some C++ code, or for security reasons. But, there are also many who like the simplicity to just download the library and use the code without the need for to recompile the C++ code.
For the moment this is just for convenience that we are 100% sure about the C++ version used (makes a big difference which version you use and it is not immediately obvious which version it is - for example, in the current readme I think that a different C++ code is referenced, then what it actually is in the last master - as you know I had some troubles with this) and for being able to experiment with the two together (C++ and C#). No drastic changes expected in your build system for the moment.

@SignalRT
Copy link
Collaborator

@SignalRT The ci of windows has failed after 3 times of retry. Is that the same issue with #599, I guess? If that's confirmed a known issue, I'll merge this PR without verification.

The issue #599 , if understand is related to that I missed llava_shared DLL on the cuda build, but this should not break anything related with the test because no cuda test are running in CI.

The test that fail seems to not be related with llava: The active test run was aborted. Reason: Test host process crashed : llama_model_loader: loaded meta data with 19 key-value pairs and 291 tensors from Models/llama-2-7b-chat.Q3_K_S.gguf (version GGUF V2)

@martindevans
Copy link
Member

The same error was happening in #603, not sure why.

@SignalRT
Copy link
Collaborator

The same error was happening in #603, not sure why.

I running the test on the master to try to understand the reason

@SanftMonster
Copy link
Collaborator Author

If cuda is excluded, is it possible that the newly added model weight leads to a lack of disk space or memory?

@zsogitbe
Copy link
Contributor

It is simply because of the loaded llava_shared.dll is wrong. If you replace it with the correct dll, then it will run OK.
I have tested it with replacing it and the test passes.

@SanftMonster
Copy link
Collaborator Author

Merge this PR without check now since it has no reason for ci to fail because of the submodule.

@SanftMonster SanftMonster merged commit 6ddd45b into SciSharp:master Mar 17, 2024
@SignalRT
Copy link
Collaborator

It is simply because of the loaded llava_shared.dll is wrong. If you replace it with the correct dll, then it will run OK. I have tested it with replacing it and the test passes.

I tried in my fork and CI test pass. Some times at the first try sometimes at second try. As an example:
https://github.com/SignalRT/LLamaSharp/actions/runs/8306390273

I'm aware that we didn´t introduce llava_shared in the CUDA directories and this is something that I will try to add on my next PR, but as far as I'm aware there is no CUDA test on CI. If you are aware of some wrong llava version I would like to know specifically the dll that you are talking about because the DLL theoretically comes from update_binaries pipeline.

@SanftMonster
Copy link
Collaborator Author

@zsogitbe I've created a new branch experimental_cpp for you. On this branch our review won't be stringent. Good luck for you!

@zsogitbe
Copy link
Contributor

Thank you Rinne!

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

Successfully merging this pull request may close these issues.

4 participants