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

Make the trailing / optional at openai.base_url setting #780

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

Conversation

kylehh
Copy link

@kylehh kylehh commented Nov 11, 2023

  • I understand that this repository is auto-generated and my pull request may not be merged

Changes being requested

Use self._base_url to replace self.base_url in BaseClient class's _prepare_url method.

Additional context & links

It does not hurt OpenAI class client, because self.base_url is referred to self._base_url base on this line

But this change would help the module_client cases, when we are setting
openai.base_url="an_endpoint_url_WITHOUT_training_/_at_the_end"
Due to this line, the base_url would directly return the URL WITHOUT trailing / and fail the requst
The workaround is always add training / at the end of the module client base_url, but with this PR fix, this step is optional (_base_url is / padded already here)

@kylehh kylehh requested a review from a team as a code owner November 11, 2023 00:23
@kylehh kylehh changed the title use self._base_url in _prepare_url Make the trailing / optional at openai.base_url setting Nov 11, 2023
@kylehh
Copy link
Author

kylehh commented Nov 11, 2023

Here is how to reproduce the problem. Following code would fail without the trailing /

openai.base_url = "https://api.openai.com/v1"
# Instead, you have to add the trailing /
#openai.base_url = "https://api.openai.com/v1/"
openai.api_key = "sk-xxxx"
chat_completion = openai.chat.completions.create()

@rattrayalex
Copy link
Collaborator

cc @RobertCraigie can you take a look at this on Monday?

Copy link

@chrishalbert chrishalbert left a comment

Choose a reason for hiding this comment

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

Can you add tests to the test_client.py to address the possible scenarios? Otherwise, it looks good to me.

@kylehh
Copy link
Author

kylehh commented Jan 11, 2024

base_url is the testing parameters get from "base_url = os.environ.get("TEST_API_BASE_URL", "http://127.0.0.1:4010")"
With this fix, ppl should be able to work with URL with and without trailing '/'. Not sure how to include in the unit test for this scenaries

@rattrayalex
Copy link
Collaborator

gentle ping @RobertCraigie

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.

3 participants