-
Notifications
You must be signed in to change notification settings - Fork 99
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
Add support for proxying open AI chat completion through cloud #148
Conversation
wavesrv/pkg/remote/openai/openai.go
Outdated
if opts.Model == "" { | ||
return nil, fmt.Errorf("no openai model specified") | ||
} | ||
conn, _, err := websocket.DefaultDialer.Dial(AWSLambdaCentralWSAddr, nil) |
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.
Use DialContext and we should set some sensible timeout value for connecting
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.
Addressed, set a 20 second timeout
I'm thinking we should completely remove the non-streaming paths in the code. Not sure they are really useful. Can't think of a time when we wouldn't want to stream the results back? |
High latency environments might want non streaming completion? web security considerations? I think removing it makes sense |
8abde7e
to
5d0dd32
Compare
…ouod completion, added capability to unset token, other small fixes
2175ae9
to
037c4b8
Compare
} | ||
cloudCompletionRequestConfig := sstore.OpenAICloudCompletionRequest{ | ||
Prompt: prompt, | ||
MaxTokens: opts.MaxTokens, |
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 still let the user specify max tokens and max choices for the cloud, should I use the defaults instead or is this ok?
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 think we shouldn't let the user specify these. We should just set them in the cloud.
@@ -66,6 +67,10 @@ const TermFontSizeMax = 24 | |||
|
|||
const TsFormatStr = "2006-01-02 15:04:05" | |||
|
|||
const OpenAIPacketTimeout = 10 * time.Second | |||
|
|||
const OpenAICloudCompletionTelemetryOffErrorMsg = "In order to protect against abuse, you must have telemetry turned on in order to use Wave's free AI features. If you do not want to turn telemetry on, you can still use Wave's AI features by adding your own OpenAI key in Settings. Note that when you use your own key, requests are not proxied through Wave's servers and will be sent directly to the OpenAI API." |
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.
nit: there's a few double spaces between sentences in this message
… and error). render cmd status 'error' with red x as well. show exitcode in tooltip of 'x'
If token is not set, chat completion will now be proxied through the cloud. You can unset your token through the UI.
the cloud uses the default model, currently "gpt-3.5-turbo".
It allows you to set max tokens and max choices parameter
If you don't have telemetry set to on, you cannot use this feature
-- CONSIDERATIONS --
For now, this is set to only hit our dev lambda server. When we deploy the completion code to our prod lambda server, we should add a check for is dev or prod
-- TESTING --
I tested with telemetry on and off, and confirmed that the correct error shows. I have tested the channel timeout and confirm that if the socket hangs, the channel will timeout and send an error, and then the terminal can continue as normal.
I have tested all of the standard code paths, confirmed that when token is set we will still do completion locally, and when token is not set we will do cloud completion. I confirmed that token can now be set to empty string, and it looks good in the ui (it goes back to "not set"). I have also checked socket failed to connect error.
There are some more unlikely error paths that I haven't checked because I didn't reproduce that error, like json decode, etc etc. I also haven't tested socket connect timeout error, but I expect these paths to work as described because the behavior is fairly standard
What do we think about the way I do testing? I can write unit tests in the future