Upgrade OpenAI SDK to v1 #1017

tonybaloney · 2023-11-29T02:12:27Z

Summary of changes

Open AI SDK v1 uses a client object, however it doesn't accept a deployment name as the parameter to the chat completions or embeddings methods, so we still need to create two clients. I've raised an issue with the Open AI team about this. In the meantime, it removed a lot of code to store state in global variables
I've implemented both Azure OpenAI clients using a token provider instead of a runtime token, this means it will fetch a token for each session call and removed a lot of workaround code to refresh tokens
The return types in the new SDK are Pydantic models. I've converted them back into dictionaries at the exit-point for the approach classes so it shouldn't leak too much of the abstraction
The new Pydantic models have required fields that we didn't bother to mock/patch in the original test code. I wanted to make the mocked responses as realistic as possible (within reason), so I've included them and that meant updating all the snapshots
I've changed the two mock fixtures for the chat completions and embeddings API to be fixture-factories that patch the method of the live client object instead of mocking the underlying OpenAI methods. This should make them more robust if OpenAI refactor their code

TODO List

Reviewers : please test as many bits of functionality as you can. I've tested this locally and deployed it to test in a production instance, but many hands find more bugs :-)

…ome redundant methods

…object and remove it from the approach constructors (and all the logic that went with it)

pamelafox · 2023-11-29T04:46:28Z

Re tests- they did post some example mocks:
openai/openai-python#715 (comment)
It might be useful to compare our approach to theirs. (I haven't had time yet myself)

… in the chat or embeddings methods

tonybaloney · 2023-11-29T06:19:52Z

Re tests- they did post some example mocks: openai/openai-python#715 (comment) It might be useful to compare our approach to theirs. (I haven't had time yet myself)

thanks, I've done something very similar. Most of the remaining work is because the old SDK used to return dictionaries and the new one returns typed pydantic models

…ls for streaming responses.

…ions. Update the iterator to pass pydantic model validation

pamelafox

Krista says that we should use model to specify deployment, so you can use a single client, which I think is better for code complexity.

app/backend/approaches/chatreadretrieveread.py

pamelafox · 2023-11-30T17:41:16Z

@tonybaloney I tested it and discovered an issue with follow-up questions. Previously, an event choice would either have a content string or have no content property at all, but now it can have "content": None. I've adjusted the code and test fixture to account for that.

pamelafox · 2023-11-30T20:45:46Z

I tested it with non-Azure OpenAI with an existing environment and there's an issue due to this new logic in the approaches:

chatgpt_model = self.chatgpt_deployment if self.chatgpt_deployment else self.chatgpt_model

The problem is that AZURE_OPENAI_CHATGPT_DEPLOYMENT has a value even if you're using non-Azure OpenAI (defaults to "chat").

One way around that is to clear out the relevant variables in app.py, before they get passed into the approaches. This change worked for me:

    AZURE_OPENAI_CHATGPT_DEPLOYMENT = OPENAI_HOST == "azure" and os.getenv("AZURE_OPENAI_CHATGPT_DEPLOYMENT")
    AZURE_OPENAI_EMB_DEPLOYMENT = OPENAI_HOST == "azure" and os.getenv("AZURE_OPENAI_EMB_DEPLOYMENT")

The error can't be replicated in test, by the way, as it's an error from the server that the model "chat" cannot be found.

tonybaloney · 2023-11-30T20:56:23Z

@tonybaloney I tested it and discovered an issue with follow-up questions. Previously, an event choice would either have a content string or have no content property at all, but now it can have "content": None. I've adjusted the code and test fixture to account for that.

I added the and content on this line because I saw a bug where it would crash with follow up questions, but maybe I introduced another bug in the process? c2d20f2#diff-a346ba4129613c27f42a9a4de5439423af8ab6df6eb79eaf555204da3d588001R302 could this be simplified if we removed the and content in the if statement?

pamelafox · 2023-11-30T21:01:50Z

@tonybaloney You could remove "and content" now, since my line takes care of that (so that in operator works). But we'd need to keep the line that turns it into an empty string.
I think this is just an effect of the move to Pydantic models with model_dump

tonybaloney · 2023-11-30T21:17:47Z

I tested it with non-Azure OpenAI with an existing environment and there's an issue due to this new logic in the approaches:
chatgpt_model = self.chatgpt_deployment if self.chatgpt_deployment else self.chatgpt_model
The problem is that AZURE_OPENAI_CHATGPT_DEPLOYMENT has a value even if you're using non-Azure OpenAI (defaults to "chat").

One way around that is to clear out the relevant variables in app.py, before they get passed into the approaches. This change worked for me:
    AZURE_OPENAI_CHATGPT_DEPLOYMENT = OPENAI_HOST == "azure" and os.getenv("AZURE_OPENAI_CHATGPT_DEPLOYMENT")
    AZURE_OPENAI_EMB_DEPLOYMENT = OPENAI_HOST == "azure" and os.getenv("AZURE_OPENAI_EMB_DEPLOYMENT")
The error can't be replicated in test, by the way, as it's an error from the server that the model "chat" cannot be found.

Good spot. Fixed

app/backend/requirements.in

app/backend/app.py

app/backend/approaches/chatreadretrieveread.py

app/backend/approaches/retrievethenread.py

app/backend/core/messagebuilder.py

scripts/prepdocslib/embeddings.py

scripts/requirements.in

tests/test_app.py

tests/test_chatapproach.py

tests/test_prepdocs.py

pamelafox

Done with my deploy tests and code review. No big items, just minor comments.

mattgotteiner

LGTM

…e for the OpenAI SDK calls

* Update version ranges for 1.3.5 openai lib * Update the embeddings library in scripts to use OpenAI 1.3.5 remove some redundant methods * Update the embedding response to use a typed model * Rewrite test_prepdocs to patch out OpenAI v1 models and responses * Update approaches to use new APIs * Update backend service and read approaches to use new SDK * Fix get_search_query. Update RRR approach tests * Update search manager tests * Change patching for app tests * Use deployment ID only in the constructor of the Azure OpenAI Client object and remove it from the approach constructors (and all the logic that went with it) * Explicitly include aiohttp in prepdocs requirements * Use two clients because the new SDK doesn't support a deployment name in the chat or embeddings methods * Ruff ruff * Simplify typing constructor * Update types for message history * Convert RRR to dict before returning * Bend the rules of physics to get mypy to pass * Run black over scripts again * Fix content filtering, update snapshot tests, implement pydantic models for streaming responses. * Update the snapshots with the new required fields for chunked completions. Update the iterator to pass pydantic model validation * Force keyword arguments as the list of arguments is long and complicated * Refactor to have a single client object * Drop argument * Type the chat message builder with pydantic * Rebuild requirements from merge conflicts * Update formatting * Fix issue with follow-up questions * Simplify content check * Don't use deployment field for non azure * Update requirements.in * Remove upper bound * Remove dependabot constraint * Merge the clients again * Fix test_app client name * Inline the ternary statement to pick either a model or deployment name for the OpenAI SDK calls --------- Co-authored-by: Pamela Fox <pamela.fox@gmail.com> Co-authored-by: Pamela Fox <pamelafox@microsoft.com>

tonybaloney added 11 commits November 29, 2023 12:34

Update version ranges for 1.3.5 openai lib

f16948b

Update the embeddings library in scripts to use OpenAI 1.3.5 remove s…

07b6a8f

…ome redundant methods

Update the embedding response to use a typed model

4dd74d1

Rewrite test_prepdocs to patch out OpenAI v1 models and responses

dfdb2af

Update approaches to use new APIs

53fdb3e

Update backend service and read approaches to use new SDK

d47fc93

Fix get_search_query. Update RRR approach tests

fff7493

Update search manager tests

77b1756

Change patching for app tests

288e961

Use deployment ID only in the constructor of the Azure OpenAI Client …

7b92ef7

…object and remove it from the approach constructors (and all the logic that went with it)

Explicitly include aiohttp in prepdocs requirements

20dc9c7

bharatsb mentioned this pull request Nov 29, 2023

Migrating to the OpenAI Python API library 1.x #996

Closed

tonybaloney added 2 commits November 29, 2023 17:09

Use two clients because the new SDK doesn't support a deployment name…

84e46ae

… in the chat or embeddings methods

Ruff ruff

119205a

tonybaloney added 7 commits November 29, 2023 17:23

Simplify typing constructor

d3ecae1

Update types for message history

5ed4ce0

Convert RRR to dict before returning

b2ab908

Bend the rules of physics to get mypy to pass

0bc3b58

Run black over scripts again

c6d9c7e

Fix content filtering, update snapshot tests, implement pydantic mode…

43500ef

…ls for streaming responses.

Update the snapshots with the new required fields for chunked complet…

5ec7ba9

…ions. Update the iterator to pass pydantic model validation

tonybaloney marked this pull request as ready for review November 29, 2023 09:12

tonybaloney requested review from pamelafox, mattgotteiner and srbalakr November 29, 2023 09:12

pamelafox requested changes Nov 29, 2023

View reviewed changes

Force keyword arguments as the list of arguments is long and complicated

43b50ac

pamelafox reviewed Nov 29, 2023

View reviewed changes

app/backend/approaches/chatreadretrieveread.py Show resolved Hide resolved

Fix issue with follow-up questions

c2d20f2

tonybaloney added 2 commits December 1, 2023 08:12

Simplify content check

21eedc5

Don't use deployment field for non azure

cc7df80

pamelafox reviewed Nov 30, 2023

View reviewed changes

app/backend/requirements.in Outdated Show resolved Hide resolved