-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: allow changing the API base URL #9
Conversation
Maybe I'm not understanding something, but why are we using |
It's just a way to safely concatenate paths in case of unsanitized user input for the baseUrl, normalizing the path and avoiding double slashes. |
That feels really icky to me. Double slashes have no effect on URL resolution (at least according to spec), so I don't really see an argument there. Does OpenAI handle double slashes incorrectly? |
No, but the baseUrl can also be used with other services that implement the openai API, like gpt-llama.cpp. There might be a case of a server handling double slashes incorrectly, so I thought it's best to normalize them just to be on the safe side. I think there is no downside of this approach. |
I am not in favor of this change, as it makes the code much harder to parse and maintain. This await fetch(`${baseUrl}/files/${fileId}/content`) is a lot more clear than await fetch(new URL(
join(this.#baseUrl.pathname, `/files/${fileId}/content`),
this.#baseUrl.origin,
).href) If we really want to prevent the client from shooting themselves in the foot, we should normalize the URL in the constructor, and not at every individual fetch request. |
examples/chatCompletion.ts
Outdated
@@ -1,9 +1,12 @@ | |||
import { OpenAI } from "../mod.ts"; | |||
|
|||
const openAI = new OpenAI(Deno.env.get("YOUR_API_KEY")!); | |||
const openAI = new OpenAI( |
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.
Could this be split out into a separate example (I think the current one is a lot more applicable to average users)?
src/openai.ts
Outdated
|
||
constructor(privateKey: string) { | ||
constructor(privateKey: string, baseUrl?: string) { |
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.
Could we change this to a "bag of options". I find it likely that we may want to add more options in the future and would hate to make a breaking change to the API.
constructor(privateKey: string, baseUrl?: string) { | |
constructor(privateKey: string, options?: { baseUrl?: string }) { |
I think if you fix the normalization issue and my two small comments, this LGTM. Thanks for the PR! We love to see community improvements! |
Whoops, I did not mean to commit the changes in the chatCompletion.ts example. I'll fix that and include suggested changes. |
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
This allows you to specify a different base URL for the API. This can be helpful when using Microsoft Azure OpenAI Services for example.