Skip to content
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

common : reimplement logging #9418

Merged
merged 8 commits into from
Sep 15, 2024
Merged

common : reimplement logging #9418

merged 8 commits into from
Sep 15, 2024

Conversation

ggerganov
Copy link
Owner

@ggerganov ggerganov commented Sep 10, 2024

ref #8566

Merge ETA: Sep 15

Overhaul common/log.h. The main goal is to offload print IO to a separate thread in order to not affect the performance of the examples. Also add convenience options for timestamps, colors based on the log type and output to file.

By default, the logs should look the same as they do on master.

Adding the following options will make them look like this:

# set once in your env
export LLAMA_LOG_COLORS=1
export LLAMA_LOG_PREFIX=1
export LLAMA_LOG_TIMESTAMPS=1

# or pass CLI args
./llama-cli ... --log-colors --log-prefix --log-timestamps

image

Another important change is that the logs in llama-server have been significantly reformatted. I've always had trouble reading the output, so I changed the text in a way that is IMO easier to read. Also removed the option to output json logs as I don't think it has any practical value.

@github-actions github-actions bot added examples ggml changes relating to the ggml tensor library for machine learning testing Everything test related labels Sep 10, 2024
@github-actions github-actions bot added python python script changes devops improvements to build systems and github actions labels Sep 12, 2024
@bviksoe
Copy link
Contributor

bviksoe commented Sep 13, 2024

Please don't take away the optional JSON formatting for logs.
This has been an important step to automate sending warnings and errors to the front-end when hosting the server process. Parsing the often very unstructured text output is tedious and error prone, while having the structured format like JSON makes it a whizz. Some logs can even function as progress notifications for a front-end, such as LOG_INFO("model loaded")

@ggerganov
Copy link
Owner Author

@bviksoe The server logs were never meant to be used in such way. These messages can be disabled and changed (even when in JSON format) without notice and therefore 3rd-party code should never rely on them. Instead, your frontend can query the server through the available endpoints. If you have a specific functionality in mind that is currently missing, submit a feature request and it will get implemented. Model loading and server status can already be queried properly through the existing API.

@ggerganov ggerganov added the merge ready indicates that this may be ready to merge soon and is just holding out in case of objections label Sep 14, 2024
@bviksoe
Copy link
Contributor

bviksoe commented Sep 14, 2024

Thanks. I understand your desire to clean up and streamline the various parts of the project.
My argument is specifically actual error and warning messages produces during loading and even during streaming inference.
In server there are many such messages that are conveyed only in log. An example: LOG_ERROR("failed to get embeddings"...) If you want to create a user-friendly front-end, these should be accessible to the website/api user.

@ggerganov
Copy link
Owner Author

If you provide sample curl requests, combined with the output that you would expect server to return for each of them, we can extend the API and the responses. Feel free to open a feature request and list the missing functionality.

@ggerganov ggerganov merged commit 6262d13 into master Sep 15, 2024
59 checks passed
@ggerganov ggerganov deleted the gg/log branch September 15, 2024 17:46
ggerganov added a commit to ggerganov/ggml that referenced this pull request Sep 20, 2024
ggerganov added a commit to ggerganov/ggml that referenced this pull request Sep 20, 2024
@kyx0r
Copy link

kyx0r commented Sep 21, 2024

Prior to this, it was possible to use --log-disable to stop llama-cli from printing all model loading debug logs. It would only print the prompt loaded with -p flag and of course model output + user input when in interactive mode. Now this is broken, it doesn't print the prompt and the model output. As far as I can see the verbosity options are not implemented completely yet to be able to facilitate the old behavior.

I hope with gets fixed in the future, as right now it only works if --log-disable is removed. But that will make it dump everything instead of just the prompt. Also, --log-enable was removed, which was useful too, but my guess is once the verbosity level settings work as expected this will cover that aspect.

@ggerganov
Copy link
Owner Author

All messages with non-zero log level are output to stderr, so you can redirect stderr to /dev/null to get the old behavior.

@kyx0r
Copy link

kyx0r commented Sep 23, 2024

Hi Georgi,
Understood, I did not check if it was still using stderr or not. It just wasn't obvious at a glance. I suppose users can continue using the stderr redirection hack for the time being. My hope that in the future though, more logging verbose levels get implemented so that it is more obvious and easier to control for the end users.

Also, even if you redirect to stderr, it doesn't remove everything. For example, you still have this message:
"== Running in interactive mode. =="
At least it's not too bad to just edit the source code and change the logging function it uses.

Kind Regards,
Kyryl

ggerganov added a commit to ggerganov/whisper.cpp that referenced this pull request Sep 24, 2024
ggerganov added a commit to ggerganov/whisper.cpp that referenced this pull request Sep 24, 2024
@ericcurtin
Copy link
Contributor

@ggerganov , I noticed the same thing. --log-disable now breaks conversation mode

@ggerganov
Copy link
Owner Author

Don't use --log-disable. Instead redirect stderr to /dev/null if you don't need it.

ericcurtin added a commit to containers/ramalama that referenced this pull request Sep 25, 2024
Now instead of --log-disable we need to redirect to stderr:

ggerganov/llama.cpp#9418

Signed-off-by: Eric Curtin <[email protected]>
ericcurtin added a commit to containers/ramalama that referenced this pull request Sep 25, 2024
Now instead of --log-disable we need to redirect to stderr:

ggerganov/llama.cpp#9418

Signed-off-by: Eric Curtin <[email protected]>
ericcurtin added a commit to containers/ramalama that referenced this pull request Sep 25, 2024
Now instead of --log-disable we need to redirect stderr to /dev/null:

ggerganov/llama.cpp#9418

Signed-off-by: Eric Curtin <[email protected]>
ericcurtin added a commit to containers/ramalama that referenced this pull request Sep 25, 2024
Now instead of --log-disable we need to redirect stderr to /dev/null:

ggerganov/llama.cpp#9418

Signed-off-by: Eric Curtin <[email protected]>
ericcurtin added a commit to containers/ramalama that referenced this pull request Sep 25, 2024
Now instead of --log-disable we need to redirect stderr to /dev/null:

ggerganov/llama.cpp#9418

Signed-off-by: Eric Curtin <[email protected]>
ericcurtin added a commit to containers/ramalama that referenced this pull request Sep 25, 2024
Now instead of --log-disable we need to redirect stderr to /dev/null:

ggerganov/llama.cpp#9418

Signed-off-by: Eric Curtin <[email protected]>
dsx1986 pushed a commit to dsx1986/llama.cpp that referenced this pull request Oct 29, 2024
lyapple2008 pushed a commit to lyapple2008/whisper.cpp.mars that referenced this pull request Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops improvements to build systems and github actions examples ggml changes relating to the ggml tensor library for machine learning merge ready indicates that this may be ready to merge soon and is just holding out in case of objections python python script changes server testing Everything test related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants