openai / openai-python

The official Python library for the OpenAI API
https://pypi.org/project/openai/
Apache License 2.0
22.23k stars 3.08k forks source link

Mypy typing error when treating a ChatCompletionSystemMessageParam as dict #1123

Open pamelafox opened 7 months ago

pamelafox commented 7 months ago

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

Describe the bug

We have a list like:

messages: list[ChatCompletionMessageParam]

We then convert a message to a dict:

dict(messages[0])

Mypy does not like that:

Argument 1 to "dict" has incompatible type "ChatCompletionSystemMessageParam | ChatCompletionUserMessageParam | ChatCompletionAssistantMessageParam | ChatCompletionToolMessageParam | ChatCompletionFunctionMessageParam"; expected "SupportsKeysAndGetItem[str, str]" [arg-type]

I'm not sure if this is a bug with mypy or with the SDK, to be honest. Or maybe there's even a better approach in our code. I see that your classes extend TypedDict, so I'd think that should be considered as SupportsKeysAndGetItem, but I'm probably missing something.

To Reproduce

See code above. You can also see our CI failing here: https://github.com/Azure-Samples/azure-search-openai-demo/actions/runs/7786990746/job/21233073297?pr=1233

Code snippets

No response

OS

MacOS

Python version

3.11

Library version

1.10.0

rattrayalex commented 7 months ago

Hey thanks for the report @pamelafox ! I think this is sort of "just how mypy works" but we are exploring ways to make this sort of situation better in the near term (cc @RobertCraigie) so hopefully we'll have an update / follow-up questions or proposals to share within a week or two.

RobertCraigie commented 7 months ago

Unfortunately this looks to just be a mypy bug. Pyright doesn't report any errors in this case.

Note that in general I would strongly recommend just relying on Pyright as bugs and features are implemented at a much faster rate compared to mypy.

I am curious why you're converting a message param into a dictionary, isn't it already a dictionary?

pamelafox commented 7 months ago

Is there a mypy bug pertaining to this particular situation? I found related issues but didn't see an obvious one. Let me know if I should file one.

I agree that we shouldn't have to wrap in dict, I think that was just our attempt to make mypy happy (which didn't work). We are passing it into a function that accepts a dict, and it has the same error if we remove the dict() as well. The error will either be on dict() if we wrap it, or our internal count_tokens(message:dict) function if not.

pamelafox commented 7 months ago

We can look into pyright, we've been using mypy for more of our projects. Pyright makes sense given we're from Microsoft but mypy seems to be more popular with Python community.

RobertCraigie commented 7 months ago

Ah @pamelafox, you can't pass TypedDicts to functions that expect a dict because that dictionary could be mutated to include keys that aren't present on the TypedDict or other similar issues.

You have two options that I can see:

I'd personally recommend referencing our types directly if possible, it'll be much more maintainable long term.

pamelafox commented 7 months ago

Ah, great point! Mapping[str, object] works for those functions (they do not mutate) and makes mypy happy. Feel free to close this, unless there are additional improvements planned.

By the way, I just looked up DL numbers for mypy vs pyright, and its 5 million weekly vs 400K weekly, so we might move to pyright ourselves but I expect much of the community will still use mypy.