Adding OpenAI wrapper class to notdiamond.toolkit.openai. [ENG-1107]

[acompa]

This PR creates sync and async OpenAI client wrappers for Not Diamond, allowing users to introduce ND-based routing to their code with one tiny change:

# before
from openai import OpenAI

#after
from notdiamond.toolkit.openai import OpenAI

Tests are mostly designed to ensure functionality for the encapsulated OpenAI classes remains as expected.

[acompa]

Still working on async tests, please hold!

[acompa]

Async tests done, please feel free to review.

[r0ymanesco]

This this good as far as what it is meant to do. But I wonder if we might have set the wrong objective. If I look at what this user's comment says, I think what they want to do is use the OpenAI client to do the same thing as the NotDiamond client. If this is the design objective then this current design doesn't route to non-OAI models. I wonder if all we need to do is use the NotDiamond client inside the OpenAI wrapper and simply map the .create method arguments to the NotDiamond client .create arguments. And either use a list or a , separator for specifying the list of model choices as the user's comment suggests. Then in terms of change in the code, they would just have the following:

from notdiamond.toolkit.openai import OpenAI

client = OpenAI() # assume that API keys are set in the envvar (both ND api key and the LLM provider keys)

chat_completion = client.chat.completions.create(
    messages=[
        {
            "role": "user",
            "content": "Say this is a test",
        }
    ],
    model="openai/gpt-3.5-turbo, togetherai/Meta-Llama-3.1-8B-Instruct-Turbo",
)

What do you think?

[acompa]

I wonder if all we need to do is use the NotDiamond client inside the OpenAI wrapper and simply map the .create method arguments to the NotDiamond client .create arguments. And either use a list or a , separator for specifying the list of model choices as the user's comment suggests.

We cannot necessarily assume .create since that requires notdiamond[create] and therefore langchain. This new toolkit capability will be installed as pip install notdiamond[openai] instead; by omitting the langchain dependency there's a lower risk of conflicting with the user's runtime environment. (Also, frankly, we don't need langchain to implement this when openai alone will suffice!)

I didn't take the user's feedback as a hard requirement, since I personally don't love the model="model1,model2,..." approach they propose. But I agree that we should accept model as a keyword to create, so I can make that happen! The openai.OpenAI client doesn't use pydantic (as far as I know) to validate model: str anyway, so nothing keeps us from accepting multiple types for that kwarg value.

[r0ymanesco]

I wonder if all we need to do is use the NotDiamond client inside the OpenAI wrapper and simply map the .create method arguments to the NotDiamond client .create arguments. And either use a list or a , separator for specifying the list of model choices as the user's comment suggests.

We cannot necessarily assume .create since that requires notdiamond[create] and therefore langchain. This new toolkit capability will be installed as pip install notdiamond[openai] instead; by omitting the langchain dependency there's a lower risk of conflicting with the user's runtime environment. (Also, frankly, we don't need langchain to implement this when openai alone will suffice!)

I didn't take the user's feedback as a hard requirement, since I personally don't love the model="model1,model2,..." approach they propose. But I agree that we should accept model as a keyword to create, so I can make that happen! The openai.OpenAI client doesn't use pydantic (as far as I know) to validate model: str anyway, so nothing keeps us from accepting multiple types for that kwarg value.

Ok that's fair. We can make this client exclusively route between openai models and expand if we get user request. In that case I would say let's make the model argument accept a list or stringmodel: Union[str, List] so that it can either be a single model or a list of models and map it to the llm_providers argument in ND client.

[r0ymanesco]

Tests seem to be failing but might have to do with the staging API key?

[r0ymanesco]

This looks good! I'm approving but noting that the tests are failing because of the ND api keys not working. Do these tests run against the staging instance or the prod instance?

[acompa]

TogetherAI test is failing, otherwise fine. Will merge this.