-
Notifications
You must be signed in to change notification settings - Fork 6.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
Mechanical switch from log to slog #2056
Conversation
3ec7636
to
65517d5
Compare
Example output on linux with debug turned on
Normal linux without enabling debug:
|
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.
LGTM.
@@ -7,7 +7,7 @@ | |||
Install required tools: |
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.
The help text just above here is confusing. I'm not quite sure what to do w/
- Install cmake or (optionally, required tools for GPUs)
is the or
for the other tools for GPUs, or for go generate/go build
?
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'll tidy this part of the README up.
I misremembered how slog works. For dynamic log level checking, it'll need a custom handler. Something like this should work: type slogHandler struct {
h *slog.TextHandler
}
func (h slogHandler) Enabled(ctx context.Context, level Level) bool {
if _, ok := os.Getenv("OLLAMA_DEBUG"); ok {
return level >= slog.LevelDebug
}
return h.Enabled(ctx, level)
} |
@@ -892,6 +893,13 @@ func (s *Server) GenerateRoutes() http.Handler { | |||
} | |||
|
|||
func Serve(ln net.Listener) error { | |||
if debug := os.Getenv("OLLAMA_DEBUG"); debug != "" { |
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 should be declared globally in cmd so it configures non-server commands too
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.
My intent was to only impact the server to narrow the scope.
I think being explicit on level is better and makes it easier for us to start to adjust the levels for messages in follow-up incremental changes. I didn't do an analysis of every log output but just skimmed for obvious warn/err scenarios to adjust those, but I would like to continue refining the levels over time. |
There's some |
A few obvious levels were adjusted, but generally everything mapped to "info" level.
A few obvious levels were adjusted, but generally everything mapped to "info" level.