-
Notifications
You must be signed in to change notification settings - Fork 11.9k
main: log file #2748
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
main: log file #2748
Conversation
Somewhat similar PR: #2657 |
@staviq Yes, this looks similar to what I had in mind - the idea is to use this to generate a trace of the You will probably need some sort of helper utils to dump a vector of tokens (e.g. |
The screenshot shows The idea is, that this PR would provide a I only added a small snippet at the start of The goal is to later use At this point, there is one header file |
Looks very good. When you think the functionality is in a good state, I can make a pass and add the traces that I am mostly interested to |
I think I'm gonna wrap all of the helper functions in macros, so in the future they can be removed compile time with a define flag I might add some more tostring helpers, but this will become clear when we start using it in Other than that, I think the core concept is roughly done. |
I think CI is stuck :) There seems to be a job running continuously for the past 2 months ? |
I wanted to un-draft this, but windows seems to hate me, so a bit of help with MSVC would be appreciated, otherwise I'm gonna need some time to find a pc to install windows and msvc on, because the CI logs are not very helpful. |
I've been checking up on this PR - Here's 1 of the logs from a run:
|
Fun fact, IntelliSense and MSVC compiler, on Windows, in the same VisualStudio environment, installed from the same installer, use different preprocessors and have wildly incompatible opinions on variadics :) I ended up dumping preprocessed code directly with the cl compiler. No errors now, and none of the warnings from CI are mine. I'm un-drafting this, and going to sleep, it's already getting bright outside :) |
I think b97958a broke the Windows build, but I don't have a machine to debug and fix it |
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.
After fixing the Windows build we can merge
@JackJollimore
You can use this branch to run a test that fails for you and send the generated log for analysis. The generated trace is very useful and I've already discovered a bug where we are often using --in-prefix ' '
which is generally incorrect for the SPM tokenizer. I haven't fixed this in the PR - will do after we merge this and debug the issues that you reported
Here is a sample log from example/chat.sh
I saw the review and comments, and I'll be back at this in couple of hours after work. |
Here's Vicuna logs Edit: I'll exclude |
Still fighting MSVC. I fixed compilation errors, But i found a problem where std::isprint() is failing ? Can it be because Edit: Uhmmm, so changing Edit2: I see 3 solutions here, I can spend more time and make it play nice with unicode etc (not a problem for me), or I can naively cast to unsigned and let it potentially strip everything remotely non alnum, or I can remove that filter entirely and let it print whatever it wants. |
I went with quick and safe fix for std::isprint, if there is a need for proper unicode-nes in logs, I think It would be less convoluted if I do it in a separate PR. MSVC is fixed, things appear to be working properly, and if I didn't miss anything, code should be consistent with review suggestions now. |
Eliminate the redundant 'else' clauses after 'if' statements that return in the true case (which I think is what the NOLINT comments are suppressing), and I'll call it good enough. |
This comment was marked as resolved.
This comment was marked as resolved.
I've looked at the logs from clang tidy review CI, and they are all easily doable, do you want me to fix those too ? Shouldn't take too long. |
Sure, go ahead and fix them. I suppose the functions that start with edit: clang-tidy is clearly trying to generate markdown output like this (generated with jq after downloading the artifact): clang-tidy.md But it doesn't seem to be actually generating any review comments like it's supposed to. edit 2: It was disabled in a pull request: #1705 |
I think I just wasted half an hour to do something clang-tidy recommends only to realize it's a bug in clang-tidy :) It insists on wrapping lambda captures in round brackets, and i couldn't figure out why it doesn't compile, until it hit me: lambda captures cannot be expressions, they have to be identifiers and wrapping them in brackets makes them invalid identifiers :) |
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.
Thank you! I think these logs will be very helpful for debugging issues in the future
I don't have a mac to test it on, I'll see if I can get osx to run in VM. But basically, either removing |
Whoops, I added that macro in #3038, actually. I compile with GCC so I didn't see those warnings. Clang will show those warnings on any platform, macOS is not required. |
WIP implementation of #2694
Logs can be redirected runtime, to a file, stdout or stderr.
https://github.com/staviq/llama.cpp/blob/71d05b9ae438e7d2c3f05212c3dcaf3a53606fa9/examples/main/main.cpp#L61-L73
A way to disable logs completely is currently missing, and possibly i could add loglevels too, to filter by debuglevel.