-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Rename OpenAIModel
to OpenAIChatCompletionsModel
#2283
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
base: main
Are you sure you want to change the base?
Conversation
PR Change SummaryRenamed
Modified Files
How can I customize these reviews?Check out the Hyperlint AI Reviewer docs for more information on how to customize the review. If you just want to ignore it on this PR, you can add the Note specifically for link checks, we only check the first 30 links in a file and we cache the results for several hours (for instance, if you just added a page, you might experience this). Our recommendation is to add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR renames the OpenAIModel
class to OpenAIChatCompletionsModel
to better reflect its specific functionality within the OpenAI API ecosystem. The original OpenAIModel
class is preserved as a deprecated alias to maintain backward compatibility.
- Replaces all imports and usages of
OpenAIModel
withOpenAIChatCompletionsModel
throughout the codebase - Updates documentation references to use the new class name
- Preserves backward compatibility by keeping
OpenAIModel
as a deprecated alias
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 7 comments.
Show a summary per file
File | Description |
---|---|
pydantic_ai_slim/pydantic_ai/models/openai.py | Renames class and adds deprecated alias |
pydantic_ai_slim/pydantic_ai/models/init.py | Updates model inference to use new class name |
tests/* | Updates all test files to use new class name |
docs/* | Updates documentation to reference new class name |
pydantic_ai_slim/pydantic_ai/providers/* | Updates provider comments to reference new class name |
Docs Preview
|
- **Run all checks**: `make` (format, lint, typecheck, test with coverage) | ||
- **Format code**: `make format` | ||
- **Lint code**: `make lint` | ||
- **Type checking**: `make typecheck` (uses pyright) or `make typecheck-both` (pyright + mypy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we drop these? I use make format
all the time, and I like going through make
better than pre-commit run --all-files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now this is CLAUDE.md and not docs/contributing.md, so I care less if Claude works better with this.
docs/agents.md
Outdated
from pydantic_ai.settings import ModelSettings | ||
|
||
# 1. Model-level defaults | ||
model = OpenAIModel( | ||
model = OpenAIChatCompletionsModel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love how long this is, what do you think about calling it OpenAIChatModel
? That's still clearly different from the Responses model and people who are familiar with both APIs will get that it refers to Chat Completions. But maybe it is better to be precise at the expense of "writeability".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to keep the original name because it reflects the actual name in OpenAI. IDE is responsible for suggesting and completing the long words.
But for someone who reads the code and is unsure, it creates a very good feeling and builds confidence, saying, "Oh, this is the same as in OpenAI."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we change the openai:
prefix in infer_provider
as well, and add openai-responses:
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to add openai-responses
. Or maybe the correct is <provider>:<class>:<model>
e.g. "openai:responses:gpt-4.1"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind that. Maybe we should then also continue to support openai:<model>
and have it default to the responses model? Assuming that API supports all models. If that's not the case (which would surprise me), maybe we should maintain a dict of which class to use for which model name, if a class wasn't explicitly specified. But generally I think we should be pushing people to the responses class/API, as that's where OpenAI is going.
This PR is stale, and will be closed in 3 days if no reply is received. |
I think I agree with the comment above. Do we really need to rename this? |
@Kludex I think a rename is a good idea, as anyone actually using OpenAI should really be using |
## Best Practices | ||
|
||
This is the list of best practices for working with the codebase. | ||
|
||
### Rename a class | ||
|
||
When asked to rename a class, you need to rename the class in the code and add a deprecation warning to the old class. | ||
|
||
```python | ||
from typing_extensions import deprecated | ||
|
||
class NewClass: ... # This class was renamed from OldClass. | ||
|
||
@deprecated("Use NewClass instead") | ||
class OldClass(NewClass): ... | ||
``` | ||
|
||
In the test suite, you need to use the `NewClass` instead of the `OldClass`. | ||
|
||
### Writing documentation | ||
|
||
Always reference Python objects with the "`" (backticks) around them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DouweM I think this would have been useful for the PR that Claude created for GoogleModel. 👀
…-openaichatcompletionsmodel
No description provided.