Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Keep defaults? #2

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Keep defaults? #2

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce8627e
Keep defaults for /completions parameters
johnd0e Apr 4, 2024
d39ab68
Keep defaults for /chat/completions parameters
johnd0e Apr 4, 2024
7252fb3
No need in defaults for model as well
johnd0e Apr 7, 2024
f9a2559
Host header is not in specs
johnd0e Apr 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions openai/init.moon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ unpack = table.unpack or unpack

import types from require "tableshape"

parse_url = require("socket.url").parse

empty = (types.nil + types.literal(cjson.null))\describe "nullable"

content_format = types.string + types.array_of types.one_of {
Expand Down Expand Up @@ -264,8 +262,6 @@ class OpenAI
assert test_messages messages

payload = {
model: "gpt-3.5-turbo"
temperature: 0.5
:messages
}

Expand All @@ -282,13 +278,7 @@ class OpenAI
-- opts: additional parameters as described in https://platform.openai.com/docs/api-reference/completions
completion: (prompt, opts) =>
payload = {
model: "text-davinci-003"
:prompt
temperature: 0.5
max_tokens: 600
-- top_p: 1
-- frequency_penalty: 0
-- presence_penalty: 0
}

if opts
Expand Down Expand Up @@ -372,7 +362,6 @@ class OpenAI
cjson.encode payload

headers = {
"Host": parse_url(@api_base).host
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Host header is part of HTTP spec, not related to any of the APIs. As this library tries to support many environments/clients, in my experience I've had some issues with Lua http clients not correctly setting host header based on the request URL. For that reason I would not remove it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly, in my humble opinion, it appears that we are trying to solve a problem that does not yet exist.

Having tested the LuaSocket with numerous different services without explicitly setting the Host in the library code, I didn’t encounter any problems. Of course, there is always the possibility someone, someday, might encounter an issue in a different environment.

In such an event, we would still have three options:

  1. Request a proper fix in the appropriate location (i.e. the custom Lua HTTP client).
  2. Instead of waiting for it to be fixed, set the header in the user code with solution Support additional headers #4.
  3. As a last resort, we can always modify the Lua-OpenAI code itself, preferably not prematurely.

However, the final decision is ultimately up to you and I will withdraw the last commit if you insist.

"Accept": "application/json"
"Content-Type": "application/json"
"Content-Length": body and #body or nil
Expand Down
Loading