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

Upgrade OpenAI SDK #2384

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Upgrade OpenAI SDK #2384

merged 1 commit into from
Feb 20, 2024

Conversation

yifanmai
Copy link
Collaborator

@yifanmai yifanmai commented Feb 20, 2024

Migrate to OpenAI SDK v1.0.0, using the guide and CLI tool here.

I manually tested this by sending requests and checking that the RequestResult looked acceptable for the following models: gpt-3.5-turbo-instruct (completion), gpt-3.5-turbo-0613 (chat), text-embedding-ada-002 (embedding), and dall-e-2 (image).

The only behavior change is that we now use "model" instead of "engine" in the request to completion and embedding models, because "engine" is deprecated. This means that new requests will not match existing cache keys for completion and embedding models. Chat models were already using "model" and are unaffected.

Also fixes an existing issue where ModerationAPIClient is instantiated eagerly instead of lazily in ServerService.

Also add a configurable base_url, which should allow the client to be used with vLLM.

Addresses #1997.

@yifanmai yifanmai force-pushed the yifanmai/fix-upgrade-openai-sdk branch from 57b7aca to b28d827 Compare February 20, 2024 22:15
Copy link
Contributor

@JosselinSomervilleRoberts JosselinSomervilleRoberts left a comment

Choose a reason for hiding this comment

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

I am not super familiar with the ModerationClient but the rest looks good

@@ -43,25 +41,17 @@ def __init__(
cache_config: CacheConfig,
api_key: Optional[str] = None,
org_id: Optional[str] = None,
base_url: Optional[str] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this base_url set anywhere or should this be specified somewhere? Since in old requests, the URL was wet by default to https://api.openai.com/v1, I am wondering if old requests that are not explicitly this will now fail? Or is the behaviour of OpenAI to set it as this if it is None?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OpenAI sets the default here. It's probably a good idea to let OpenAI do it, and not duplicate this in our own code.

@yifanmai yifanmai merged commit 149141c into main Feb 20, 2024
6 checks passed
@yifanmai yifanmai deleted the yifanmai/fix-upgrade-openai-sdk branch February 20, 2024 23:53
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.

2 participants