Skip to content
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 api_url param in init function #40

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tiankonglan
Copy link

in some sence api_url is not "https://api.openai.com", so I commit the pr.

@howard0su
Copy link

Will this support Azure OpenAI API?

@talk2MeGooseman
Copy link

Will this support Azure OpenAI API?

It doesn't look like it would support Azure because of the base URL configs for the various endpoints include v1.
I think the v1 specific in the various endpoint modules would need to be moved to API base URI instead.

@base_url "/v1/completions"

@cigrainger
Copy link

It would be amazing if this would support Azure. I'd be happy to help but it seems like a trivial change. Just dropping a vote in favour of moving that v1 to the API base URL. 👍

@mgallo
Copy link
Owner

mgallo commented Jun 18, 2023

Hi @tiankonglan! Thanks for this PR The api_url param in the config was only designed to be used as a test url (this is the original issue: #20), However, is correct to have it in the GenServer init function, but in this PR implementation it will not consider the default value, I'll change it to:

  def init(_opts) do
    config = %{
      api_key: get_config_value(:api_key),
      organization_key: get_config_value(:organization_key),
      http_options: get_config_value(:organization_key),
      api_url: get_config_value(:api_url, @openai_url)
    }

    {:ok, config}
  end

and remove the @config_keys module attribute, WDYT?

Azure integration topic

@howard0su @talk2MeGooseman @cigrainger
overriding api_url is not sufficient to fully support Azure APIs, because the azure auth token header differs from the standard one, as you can see here: https://learn.microsoft.com/en-us/azure/cognitive-services/openai/reference#example-request (api_key is passed as an api-key header). In the standard client (current implementation) the token is Authorization: Bearer OPENAI_API_KEY https://platform.openai.com/docs/api-reference/authentication.

To support azure in a proper way we should create a specific client for it, that share the logic of the existing one for handling the response, but pass specific url and headers for authentication.

I don't think that moving the v1 to the API base url is a good idea, because it can impact future releases of the APIs that can have different versions for different actions.

FYI I converted the original issue about Azure in a discussion thread here

@mtanzi
Copy link

mtanzi commented Jun 27, 2023

Hey @mgallo @howard0su @cigrainger @talk2MeGooseman, I started to work on the Azure support and I have something roughly working on a branch inside my fork, I currently have tested only the OpenAI.chat_completion because that was the endpoint I needed to put together a prototype. I didn't spend to much time on polishing the whole API wrapper.

If you are interested I can open a PR here where we can discuss what should be changed and added to have something mergeable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants