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

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

Open
1 task done
npip99 opened this issue Apr 6, 2024 · 2 comments
Open
1 task done

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

npip99 opened this issue Apr 6, 2024 · 2 comments

Comments

@npip99
Copy link

npip99 commented Apr 6, 2024

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

  • 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 #911 (comment),

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 npip99 changed the title ChatCompletion Message Types don't allow enumerations ChatCompletions create() doesn't type-check enums as role Apr 6, 2024
@npip99
Copy link
Author

npip99 commented Apr 6, 2024

Solution is also provided at #911 (comment)

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).

@rattrayalex
Copy link
Collaborator

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

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

No branches or pull requests

2 participants