Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Resolve Typing Issues on Wrapping OpenAI Client #1156

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

17jmumford
Copy link
Contributor

@17jmumford 17jmumford commented Jan 27, 2025

Details

When wrapping the OpenAI client with the tracker, it set the type as a Union of both the regular and async client. This caused downstream type check errors when using OpenAI classes. TypeVar can resolve this by allowing the output type to match the input type. The IDE will now understand that the type of the client remains the same.

This is the same pattern that currently exists in the Anthropic wrapper.

Issues

Resolves #1152

Testing

Tested locally, fixed the problem. This is my first time contributing, so would love advice/help on what the order of operations is for contributing.

Documentation

No updates to documentation required.

@17jmumford 17jmumford requested a review from a team as a code owner January 27, 2025 19:03
@17jmumford 17jmumford changed the title added TypeVar so that output type matches input type Resolve Typing Issues on Wrapping OpenAI Client Jan 27, 2025
@alexkuzmik
Copy link
Collaborator

Good improvement, thanks @17jmumford!

@17jmumford
Copy link
Contributor Author

17jmumford commented Jan 27, 2025

Good improvement, thanks @17jmumford!

You're welcome! I'll leave it to you to merge it in. Looks I don't have write access to the repo to hit the merge button. Also sad that the CI checks don't work for me 😢 . Would love to contribute more in the future, lmk what can be done to reduce friction in this process.
image

@alexkuzmik
Copy link
Collaborator

It's a bit tricky with the CI because we have a lot integration tests that require different API keys configured as GitHub secrets.
External contributors do not have access to those secrets(they should configure their own in their repositories), so at first we only look at e2e tests and unit tests, then we might check the integration ones by ourselves.
We'll definitely look into improving the flow in the future to make the process smoothier :)

@alexkuzmik alexkuzmik merged commit 4e06ae3 into comet-ml:main Jan 28, 2025
12 of 40 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Update OpenAI Type Casting
2 participants