Conversation
…pply-patches script perfectly lines up with the current whisper.cpp folder in the main branch
…file into brake/whisper_submodule
aittalam
left a comment
There was a problem hiding this comment.
Thanks for the PR @njbrake! Aside from a couple minor comments, there's a more important one IMHO to be addressed, i.e. the fact that (differently from whisper and sd) the process this time does not tell apart patched files from newly introduced ones (see my larger comment in renames.sh).
I think getting a better understanding of what is a change from llama.cpp's code vs what has been newly introduced would be beneficial to us. Even with the obvious assumption that there's a lot of changes, removing patched files from llamafile-files will leave us with a much smaller, cleaner set of files we know are not originally from llama.cpp. WDYT? Happy to do a quick sync on that.
|
Thanks for the review @aittalam! I appreciate your in-depth review, and it was helpful to me as I think about the philosophy of the AI assisted coding world we're entering. In the case of this PR, I used Claude to help with the refactoring and took the time to verify that my refactor was technically correct, but I didn't take the time to actually review how it was doing it. What this resulted in was that I effectively shifted the cognitive burden of thinking about the PR off of me and onto you, the reviewer. This is bad practice and not good for me as the author or you the reviewer. Imo the author (me) should remain the person primarily responsible for thinking about the design of the changes, not the reviewer. In this case, you had to both think about the design and also be the backstop, which isn't ideal 🙈 . Anyways, thank you for the solid review, sorry for my spaghetti code, and let me dig in to clean up the code (as David says, "humanify" it 😆 ). |
There was a problem hiding this comment.
Thank you @njbrake! I think seeing what's left is very interesting.
I took the liberty of suggesting two more directories to remove (I spotted them with a diff -r llama.cpp /tmp/llamafile/llama.cpp, perhaps git diff ignores empty dirs?), and I also got the following:
diff -r llama.cpp /tmp/llamafile/llama.cpp
Only in llama.cpp: .git
diff -r llama.cpp/ggml-cuda.cu /tmp/llamafile/llama.cpp/ggml-cuda.cu
366c366
< kernelVersion % 100000);
---
> kernelVersion % 100000);
diff -r llama.cpp/ggml-metal.m /tmp/llamafile/llama.cpp/ggml-metal.m
3381c3381
<
---
>
diff -r llama.cpp/server/public/index.html /tmp/llamafile/llama.cpp/server/public/index.html
882c882
< // such as "strings" or /* comments */. These regexps are then utilizied by the
---
> // such as "strings" or /* comments */. These regexps are then utilizied by the
1085,1086c1085,1086
< // This transforms _some_ markdown to html by replacing code blocks and
< // urls with a placeholder, so that any markdown within these already
---
> // This transforms _some_ markdown to html by replacing code blocks and
> // urls with a placeholder, so that any markdown within these already
diff -r llama.cpp/server/server.cpp /tmp/llamafile/llama.cpp/server/server.cpp
3196c3196
< int written = snprintf(url, sizeof(url), "http://%s:%d%s/",
---
> int written = snprintf(url, sizeof(url), "http://%s:%d%s/",
3233c3233
< exit(1);
---
> exit(1);
... which I believe we could keep (it's I think the same lines you were referring to).
I pre-approved the PR so you can merge it immediately after the (minor) fix!
Co-authored-by: Davide Eynard <davide.eynard@gmail.com>
To test
You should see
This is just some whitespace changes from autoformat rules, no content changes.