-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Adding linenoise.cpp to llama-run #11252
Conversation
Linked issue: |
fdaa9ba
to
bd3e9ae
Compare
14dc5f3
to
29d40de
Compare
This does not work on Windows? I guess inside of a container it would work. |
I couldn't find any library like linenoise that had Windows support. Plenty have macOS/Linux support. But yeah if using a container or WSL2, etc. we'd be fine. I didn't kill Windows native support here, but you don't get the cool features like being able to cycle through prompt history with up and down arrows. |
29d40de
to
47487ae
Compare
examples/run/linenoise.h
Outdated
* * Redistributions in binary form must reproduce the above copyright | ||
* notice, this list of conditions and the following disclaimer in the | ||
* documentation and/or other materials provided with the distribution. |
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.
Note that this will require adding a notice in the releases.
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.
Where can I copy that in? I think the license is compatible once we include these copyrights
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.
Not sure how this is usually handled, I guess we could add a file with all 3rd party copyright notices and copy it to the release packages.
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.
We have the same issue with CURL if we start distributing it for Windows FWIW, from curl license: "Permission to use, copy, modify, and distribute this software for any purpose with or without fee is hereby granted, provided that the above copyright notice and this permission notice appear in all copies."
@ggerganov you were speaking about shipping curl on Windows recently.
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 also don't know what is the common way to handle this. Probably look for how other projects do it.
Btw, each llama.cpp
release already includes a copy of the full source code in this repo so I think all licenses are technically already included in the release packages? Again, we can make this more explicit if necessary - feel free to improve.
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'm ok with that technique, there's no standard technique. Everybody does it a little different.
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 am not a lawyer, but I am not convinced that's enough. We already copy our own LICENSE
file to the release packages as llama.cpp.txt
, so it shouldn't be a problem to also add the 3rd party licenses there.
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.
@slaren can you show me the script where that copy occurs? We can just check in:
https://github.com/ericcurtin/linenoise.cpp/blob/main/LICENSE
and copy that file to linenoise.cpp.txt maybe ?
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.
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.
Thanks my searching tool skips hidden directories, sometimes I forget this. I hope it's good now, it should do the copy of the LICENSE now I think.
27812c8
to
6360829
Compare
Most of the time windows will be using a container so this should not be an issue. |
4127d6a
to
1aa42d3
Compare
What unblocks this from getting merged @slaren and @ggerganov ? |
1aa42d3
to
e3f66a8
Compare
e3f66a8
to
e2d118a
Compare
Yay green build and licensing file copied @slaren @ggerganov |
@@ -796,6 +796,7 @@ jobs: | |||
if: ${{ ( github.event_name == 'push' && github.ref == 'refs/heads/master' ) || github.event.inputs.create_release == 'true' }} | |||
run: | | |||
Copy-Item LICENSE .\build\bin\Release\llama.cpp.txt | |||
Copy-Item .\examples\run\linenoise.cpp\LICENSE .\build\bin\Release\linenoise.cpp.txt |
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.
This will only affect the Windows releases, but the macOS and linux releases should also be updated.
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.
How about now?
e2d118a
to
05c338b
Compare
I just tested on my Linux box with address sanitizer enabled, and the address sanitizer does not produce an error during model loading. However, it produces an error at the end of the first response:
Looks similar to the issue reported by @slaren earlier on Windows. |
I've managed to reproduce it, what's really weird is it seems to jump around the place, I'm guessing the bug is before chat_loop because of the bug where asan exposes it during the loading of the model file (that's what I've seen too). (Unless we are actually chasing 2 bugs). |
Something else I see in valgrind:
|
I wonder if I call ggml_backend_load_all(); before initialising context will it fix this |
Nevermind I think ggml_backend_load_all() is ok |
I can confirm when we revert: this issue goes away, which cleanly reverts |
This is a simple reproducer anyway for asan, valgrind, etc. :
|
The sanitizer error from @ggerganov is caused by adding pointers |
Wow it would have been a long time before I considered small string optimization causing issues. Thanks so much. I'll try and change:
to:
I guess sometimes C++ isn't safer than C 😓 |
Replacing the |
Still doesn't explain the llama_model_load_from_file bug which seems more readily reproducible, it happens before all this. |
Haven't been able to reproduce the other one. |
The error with model load could be a false positive described in https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow , where one part of the app is built without ASAN enabled (could be @ggerganov Did you try deleting the |
33d1eec
to
164c4ee
Compare
Nonetheless I pushed the list change 😄 |
This is a fork of linenoise that is C++17 compatible. I intend on adding it to llama-run so we can do things like traverse prompt history via the up and down arrows: https://github.com/ericcurtin/linenoise.cpp Signed-off-by: Eric Curtin <ecurtin@redhat.com>
164c4ee
to
0ea354c
Compare
Now that you mention it, the llama.cpp files are built without sanitizer flags. This needs to be fixed as well. |
You could be right, things do run fine without asan |
I'll push a fix on |
For the |
#11279 resolves the sanitizer error on mac. |
Is my understanding correct that the issue with the GGUF code turned out to be a false positive that has now been fixed? |
Yes and I think all the issues discussed in this PR are resolved now. |
This is a fork of linenoise that is C++17 compatible. I intend on adding it to llama-run so we can do things like traverse prompt history via the up and down arrows:
https://github.com/ericcurtin/linenoise.cpp