openai-python icon indicating copy to clipboard operation
openai-python copied to clipboard

ChatCompletions create() doesn't type-check enums as role

Open npip99 opened this issue 1 year ago • 2 comments

Confirm this is a feature request for the Python library and not the underlying OpenAI API.

  • [X] This is a feature request for the Python library

Describe the feature or improvement you're requesting

This was discussed previously @ #911, but just opening a new issue so that it's in the issue tracker.

The Bug

When calling,

role: Literal["system", "user", "assistant"] = ...
completion = await client.chat.completions.create(
    model="gpt-4",
    messages={"role": role, "content": "Hi"}
)

Reason

The main issue is that it's defined as an enum of typed dicts,

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

Of course, for tooling & functions, we need to know exactly whether or not it's a user or assistant message. But role: str, content: str is the most common use-case, so it would be nice if system/user/assistant can all use that kind of interface when necessary.

Additional context

Potential Solution

From https://github.com/openai/openai-python/issues/911#issuecomment-2038669860,

I think a very simple solution would be to add another type to the ChatCompletionMessageParam union.

Currently, we have

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

If instead we had,

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

Where ChatCompletionGenericMessageParam was,

class ChatCompletionGenericMessageParam(TypedDict, total=False):
    content: Required[Optional[str]]
    role: Required[Literal["system", "user", "assistant"]]
  • Under my understanding, adding an option to a Union does not break compatibility with anybody (Lmk if I'm wrong)
  • This handles all possible situations (pydantic model, custom TypedDict with Literal, a mixture of both, etc).
  • And, if the user wants to pass in a tool/FunctionCall/ChatCompletionContentPartParam, then obviously they need to use the other types and prove that it's specifically a system/user/etc.
  • OpenAI can still internally type check safely, because it can write a type-safe function to convert ChatCompletionGenericMessageParam into a Union[ChatCompletionSystemMessageParam, ChatCompletionUserMessageParam,ChatCompletionAssistantMessageParam].

npip99 avatar Apr 06 '24 17:04 npip99

Solution is also provided at https://github.com/openai/openai-python/issues/911#issuecomment-1857142928

It's described as pydantic, but I don't think we can have messages= accept both dictionaries and pydantic models. However, Message can be a function taking 2 kwargs role and user. And, it returns a Union[ChatCompletionSystemMessageParam, ChatCompletionUserMessageParam, ChatCompletionAssistantMessageParam], implemented internally using an if/elif/else.

Maybe name can be CreateChatCompletionMessageParam, not sure.

Just different ideas.


If the reason for making the User have to call a function is because the typing is automatically generated, then maybe there's a way to manually add in a type there that can be brought into the automated system. I feel it will be hard for Users to navigate into this issue and know to use a factory for this type. But, I have no idea, maybe it's not possible to work this into the auto-generated system.

But, having a factory will be nice regardless, because it means you can do my_messages = [CreateMessage(...)], and that my_messages variable will be typed as a list of messages rather than as a list of dicts (Causing mypy errors if you later try to use my_messages).

npip99 avatar Apr 06 '24 18:04 npip99

Thanks for the suggestion. We'll take it into consideration as we think about ways to improve this experience.

rattrayalex avatar Apr 08 '24 01:04 rattrayalex