tmc / langchaingo

LangChain for Go, the easiest way to write LLM-based programs in Go
https://tmc.github.io/langchaingo/
MIT License
4.39k stars 597 forks source link

Consider using panic instead of log.Fatalf for handling unexpected lines in parseStreamingChatResponse function #822

Closed wangjiancn closed 4 months ago

wangjiancn commented 4 months ago

https://github.com/tmc/langchaingo/blob/03b74ec5c76370198108c00a64c36f33d10c9601/llms/openai/internal/openaiclient/chat.go#L367-L396

Could you kindly provide more information on why log.Fatalf("unexpected line: %v", line) is used in the code snippet you shared? This action results in the process terminating unexpectedly. Would it be more appropriate to consider using panic instead for better handling of such situations? Thank you for your insights.

devalexandre commented 4 months ago

I think, in not use fatal, maybe we need return the error

if !strings.HasPrefix(line, "data:") { 
                                return nil, errors.New("unexpected line")
            } 
wangjiancn commented 4 months ago

When an exception occurs within a Go goroutine, it appears that employing an error might not produce the intended outcome. Is there an intended update or change to address this? My concern centers around the use of log.Fatalf, which can lead to an unintended program termination.

androidsr commented 4 months ago

我的意思是任何框架代码里都不应该出现 log.Fatalf 导致应用程序退出。因为这里是在子任务中,就算defer func() { recover() }()也都无法避免应用程序退出。

androidsr commented 4 months ago

What I mean is that log.Fatalf should not appear in any framework code causing the application to quit. Since this is in the subtask, defer func() { recover() }() also can not avoid the application exit.

devalexandre commented 4 months ago

in this case fatal is because can't be without data

androidsr commented 4 months ago

In fact, we use the local model and see that the message content is a message sample of the ping content. If it is impossible, why add this judgment?

douglarek commented 4 months ago

in this case fatal is because can't be without data

The key issue here is the abrupt termination at this point. Your user end cannot obtain any useful exit information from your API calls; all they get is an "unexpected line: xxxx" message. So if this library suddenly terminates when called in a user end main function, can you tell me where this error comes from? This design is clearly unacceptable. Even using panic instead of fatal would be better here, as fatal is too abrupt.

And the errors here are easily handled. Just encapsulate the errors within each StreamedChatResponsePayload.