Fix typing errors from mypy when calling completion

[achrome]

Confirm this is an issue with the Python library and not an underlying OpenAI API

• This is an issue with the Python library

Describe the bug

Mypy doesn't seem to play very well with the type annotations that the SDK has generated. I can see that OpenAI uses Stainless and that might be the issue in itself, but as it stands the type annotations aren't very well documented.

See below for the exact use case and how to reproduce.

I'd propose either updating the annotations to be more semantically correct, or updating the documentation and usage if the idea is to keep the annotations more strict.

Option 1 seems like a better option until a full migration to stricter types can be reached, likely in a major version.

To Reproduce

When calling completion like so

client = AsyncOpenAI(api_key=<API_KEY>)
messages = [
    {"role": "user", "content": "some test message"}
]
completion = await client.chat.completions.create(
    model="gpt-3.5-turbo-0613", # Actual model doesn't matter here
    messages=messages # Mypy shows a type mismatch error here
)

I did a little bit of digging and found that the messages arg in chat.completions.create is annotated as ChatCompletionMessageParam, however the method is smart enough to take and serialize plain dicts as well. I haven't seen the rest of the endpoints, but my guess is that this annotation issue is present throughout.

I can get around it by using the appropriate types instead. So, the following works

from openai.types.chat import ChatCompletionMessageParam, ChatCompletionUserMessageParam
messages: list[ChatCompletionMessageParam] = [
    ChatCompletionUserMessageParam(role="user", content="some test message")
]
# This seems to satisfy mypy

Code snippets

No response

OS

Linux

Python version

3.12

Library version

v1.3.6

[RobertCraigie]

This is an unfortunate limitation of type inferring in the current state of python's static types.

If you don't inline the messages param you need to give it an explicit type

from openai.types.chat import ChatCompletionMessageParam

messages: list[ChatCompletionMessageParam] = [
    {
        "role": "user",
        "content": "Say this is a test",
    },
]

You don't need to actually instantiate the type like in your example unless you really want to.

[RobertCraigie]

but as it stands the type annotations aren't very well documented.

What more documentation would you want to see?

I'd propose either updating the annotations to be more semantically correct

What do you mean by semantically correct? If we updated the type annotations to accept any abritrary dictionary then all of our types would be essentially meaningless.

[nathan-chappell]

@RobertCraigie Python's typing might be limiting to some extent, but the fact is that you guys automatically generated your client library with some tool that did a pretty good, but not perfect, job.

Let's suppose I want to use the openai types to create a list of messages. Up until the new library I was using a definition like so:

class MessageDict(TypedDict):
    role: Literal["system", "user", "assistant"]
    content: str

It doesn't handle every possible way to access the api, but it's been good enough (usually if I'm doing function calling there is a lot more ceremony involved anyways).

I would like to use such a class as follows:

completion = await client.chat.completions.create(
    messages=[MessageDict(role=r, content=c) for r,c in some_dict.items()],
    ...
)

As of right now the type checkers don't like this. Fine, how can I do it with the types you've provided us?

class ChatCompletionFunctionMessageParam(TypedDict, total=False):
    content: Required[Optional[str]]
    name: Required[str]
    role: Required[Literal["function"]]

class ChatCompletionToolMessageParam(TypedDict, total=False):
    content: Required[Optional[str]]
    role: Required[Literal["tool"]]
    tool_call_id: Required[str]

class ChatCompletionAssistantMessageParam(TypedDict, total=False):
    content: Required[Optional[str]]
    role: Required[Literal["assistant"]]
    function_call: FunctionCall
    tool_calls: List[ChatCompletionMessageToolCallParam]

class ChatCompletionUserMessageParam(TypedDict, total=False):
    content: Required[Union[str, List[ChatCompletionContentPartParam], None]]
    role: Required[Literal["user"]]

class ChatCompletionSystemMessageParam(TypedDict, total=False):
    content: Required[Optional[str]]
    role: Required[Literal["system"]]

# openai.types.chat

ChatCompletionMessageParam = Union[
    ChatCompletionSystemMessageParam,
    ChatCompletionUserMessageParam,
    ChatCompletionAssistantMessageParam,
    ChatCompletionToolMessageParam,
    ChatCompletionFunctionMessageParam,
]

# Since this is a union type an not a base, it can't be used to create dicts like all the other types here can.

Listen man, this is an ugly mess. It's pretty good for automatically generated, and I'd certainly be willing to deal if it was somewhat convenient, but no man, it's not:

• either import a bunch of long ass names everywhere (ugly and inconvenient)
• make a bunch of aliases (somewhat ugly, adds complexity)
• add #type: ignore (as ugly as it gets)

What's funny is that the MessageDict could essentially serve as a base class to all those other TypedDict classes, but the Union type is generated, so it can't be used for creating objects.

TLDR: whatever

The new client is a huge improvement on that old one, it almost feels like modern python. But it's not perfect, so please don't act like it is. Oh yea, and the documentation I would like to see is an OpenAI python-programmer using the types from the new library effectively.

[antont]

@nathan-chappell Hm I don't quite get this.

Am doing:

messages: list[ChatCompletionMessageParam] #I get this as param to a function and then add more messages to it

messages.extend([
    system_message("You are a helpful assistant."),
    user_message(question)
])

completion = await client.chat.completions.create(
    model="gpt-4",
    messages=messages
)

where am using a couple factory funcs to have the message instance creation more simple and robust

def system_message(content: str):
    return ChatCompletionSystemMessageParam(
        content=content,
        role="system"
    )

def user_message(content: str):
    return ChatCompletionUserMessageParam(
        content=content,
        role="user"
    )

And at least in VSCode (with I guess with pyright?) the type checker is happy.

Is this perhaps a limitation of mypy? I think the author mentioned that in some discussion here.

Or did I miss something? Only gave a quick read so far, and figured that might be useful to share what am doing.

[RobertCraigie]

Your first method snippet will not work regardless of what our types are, it's entirely dependant on how you've typed some_dict.

Listen man, this is an ugly mess. It's pretty good for automatically generated, and I'd certainly be willing to deal if it was somewhat convenient, but no man, it's not:

I'm not sure where you're coming from here - why do you want to explicitly instantiate an object for the params?

All of this goes away if you in-line the dictionary (assuming some_dict is typed properly):

completion = await client.chat.completions.create(
    messages=[{'role': r, 'content': 'c'} for r,c in some_dict.items()],
    ...
)

What's funny is that the MessageDict could essentially serve as a base class to all those other TypedDict classes, but the Union type is generated, so it can't be used for creating objects.

Are you suggesting that we update the params type to just be your MessageDict type? That will just result in less type-safety, as it would have to include all of the properties from the actual message types and let you represent states that are not valid.

So even if we extracted some properties into a common base class the params type should still be a Union so that it represents the actual types that the API expects. Which then makes the MessageDict base class entirely moot because you wouldn't be able to assign it to our union type.

Then additionally, using that as a base class is potentially unsound which is why Pyright will report an error in strict mode for this definition:

class BaseMessageParam(TypedDict):
    role: Literal["system", "user", "assistant"]
    content: str

class AssistantMessageParam(BaseMessageParam):
    role: Literal['assistant']
    # ...

# Type of TypedDict field "role" cannot be redefined
#    Type in parent class is "Literal['system', 'user', 'assistant']" and type in child class is "Literal['assistant']"

I found your comment a bit rude; some parts did not contribute to a productive discussion.

[nathan-chappell]

@nathan-chappell Hm I don't quite get this.

Am doing:

messages: list[ChatCompletionMessageParam] #I get this as param to a function and then add more messages to it

messages.extend([
    system_message("You are a helpful assistant."),
    user_message(question)
])

completion = await client.chat.completions.create(
    model="gpt-4",
    messages=messages
)

where am using a couple factory funcs to have the message instance creation more simple and robust

def system_message(content: str):
    return ChatCompletionSystemMessageParam(
        content=content,
        role="system"
    )

def user_message(content: str):
    return ChatCompletionUserMessageParam(
        content=content,
        role="user"
    )

And at least in VSCode (with I guess with pyright?) the type checker is happy.

Is this perhaps a limitation of mypy? I think the author mentioned that in some discussion here.

Or did I miss something? Only gave a quick read so far, and figured that might be useful to share what am doing.

This is what I refer to as make a bunch of aliases (somewhat ugly, adds complexity). See, the thing is that probably everyone would like to make these factory functions, but if that's the case then why isn't it just provided by the client library? Also, why would you have to provide the role parameter to the class ChatCompletionSystemMessageParam? There is one reasonable choice, and it should be filled in automatically if I'm putting System in the damn class name. But let's look at the class definition:

class ChatCompletionSystemMessageParam(TypedDict, total=False):
    content: Required[Optional[str]]
    role: Required[Literal["system"]]

Okay, can't put a default value in there because it's a TypedDict. At this point as library writers we have two options: say screw it, user's problem, or come up with a better design. I don't know if this is some sort of impossible thing to do because the API is too overloaded, but right now the current design is not tuned towards convenience.

[nathan-chappell]

@RobertCraigie I apologize for being rude. I was being intentionally confrontational, and I would rather proceed in a cordial manner.

Your first method snippet will not work regardless of what our types are, it's entirely dependant on how you've typed some_dict

Fair enough, but in general you are missing my point. Basically, there are times when we can in our own code guarantee that a some variable is an appropriate role, but then getting this information into your api is difficult. For example:

tuples: list[tuple[Literal["user", "assistant", "system"], str]] = []

# type checker not happy
completion = await client.chat.completions.create(
    messages=[{"role": r, "content": c} for r, c in tuples],
    model="foobar",
)

# invalid - can't construct with Union type
completion = await client.chat.completions.create(
    messages=[ChatCompletionMessageParam(role=r, content=c) for r, c in tuples],
    model="foobar",
)

# I hope you aren't serious...
completion = await client.chat.completions.create(
    messages=[
        (
            ChatCompletionUserMessageParam(role=r, content=c)
            if r == "user"
            else ChatCompletionAssistantMessageParam(role=r, content=c) if r == "assistant" else ChatCompletionSystemMessageParam(role="system", content=c)
        )
        for r, c in tuples
    ],
    model="foobar",
)

So, first of all, I want to check if you are defending the current implementation and feel that it is a good design that shouldn't be improved. If that's not the case, then what I'm telling you is that there are issues in the design that make seemingly obvious things difficult. Blaming python doesn't really inspire confidence in your python programmers who actually try to solve such design problems.

Again, it's a huge improvement, but there are a number of small issues that hinder the usability of the library that would likely be avoided if the API endpoints weren't so heavily overloaded or you guys weren't trying to automatically generate the whole client. Do I want you guys to just use MessageDict? No, that's two second easy solution. But it might demonstrate what your target for usability could be.

[nathan-chappell]

@RobertCraigie I don't want to be rude here either, I would just like to put out there that I found your initial response to the first commenter a bit dismissive. If you are a person of authority, than you being dismissive will always stifle discussion more than someone without it being confrontational. This very page and discussion is my case in point.

[antont]

@nathan-chappell

class ChatCompletionSystemMessageParam(TypedDict, total=False):
    content: Required[Optional[str]]
    role: Required[Literal["system"]]

Okay, can't put a default value in there because it's a TypedDict

Ah, I didn't even know that.

Maybe the message types should be Pydantic models, like the API responses already are. They can be instantiated quite nicely like ChatCompletionUserMessageParam(content=text) and have extra validation such as string lengths and number ranges etc. which I guess could be useful.

OpenAI folks want to support passing just dicts as the messages, but I guess the functions could accept both the correct Pydantic type, and a dict that'd be then used to create an instance of that type, and thus be validated at that stage. Maybe this change could be done in a backwards compatible way, I'm not sure.

[antont]

@nathan-chappell

That's actually what I would do, and I think completely sensible if for some reason you have the roles as strings.

tuples: list[tuple[Literal["user", "assistant", "system"], str]] = []
(...)
# I hope you aren't serious...
    messages=[
        (
            ChatCompletionUserMessageParam(role=r, content=c)
            if r == "user"
            else ChatCompletionAssistantMessageParam(role=r, content=c) if r == "assistant" else ChatCompletionSystemMessageParam(role="system", content=c)
        )
        for r, c in tuples
    ],

I'd probably use a dict to map the strings to classes. Well, actually I do have something like this in our system, just from a different role enum that we use internally.

role2message = {
  'user': ChatCompletionUserMessageParam,
  'assistant': ChatCompletionAssistantMessageParam
  'system': ChatCompletionSystemMessageParam
}

messages=[role2message[role](content=content, role=role)
          for role, content in tuples]

I don't find this unreasonable when you have strings that define which type should be used.

[nathan-chappell]

@antont I like all of the solutions you've proposed, and would be happy to see something like that packaged with the client. I really want the openai client to be good enough to prevent the proliferation of useless convenience tools like we've seen this "ai spring."