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

Bump pydantic version from 1.10.13 to >= 1.9.0, < 3 #2322

Closed
wants to merge 1 commit into from

Conversation

kmcleste
Copy link

@kmcleste kmcleste commented Jan 3, 2024

Updating pydantic version from 1.10.13 to >= 1.9.0, < 3. The version was originally pinned due to a dependency on the OpenAI python library. OpenAI has since updated their version requirements for pydantic. See OpenAI docs for the version change: pyproject.toml.

Pydantic V1 is no longer supported, and suggests migration to V2 (Migration Guide).

@@ -100,7 +100,7 @@ def parse_args():
def create_error_response(status_code: HTTPStatus,
message: str) -> JSONResponse:
return JSONResponse(ErrorResponse(message=message,
type="invalid_request_error").dict(),
type="invalid_request_error").model_dump(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is something that was added after 2.0, so won't work for pydantic>=1.9,<2. to fix this, you could instead do from pydantic.v1 import BaseModel and the rest of the file could remain as-is

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the ErrorResponse object is super simple (only has str or None values), we can just let this remain as dict(), as the outcome is the same, without needing any new methods?

Copy link
Contributor

@jvmncs jvmncs Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no dict() method in pydantic v2, per the migration guide. do you mean using the builtin dict function? that would also work here and would allow usage of v2's BaseModel instead of the pydantic.v1 compat class, which seems preferable 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wow, pydantic v1 doesn't even have the v1 module. seems pretty strange to not do that for the last few v1 minor versions. anyway looks like dict(ErrorResponse(...)) is the only obvious/clean way to keep the code consistent for v1 & v2 installations. alternative would be to restrict the dependency to >=2,<3

Copy link

@dbuades dbuades Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option, based on your original idea, I suggest you do:

try:
    from pydantic.v1 import BaseModel, Field
except ImportError:
    from pydantic import BaseModel, Field

in https://github.com/vllm-project/vllm/blob/79d64c495463665cf80937a05226806785cfa583/vllm/entrypoints/openai/protocol.py#L6C2-L6C2.

That will work with both versions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 @kmcleste can you fix this?

@jvmncs
Copy link
Contributor

jvmncs commented Jan 8, 2024

bumping this! currently blocking our deployment of vllm, which has to integrate with other pydantic ecosystem packages

looks like you need to format the code @kmcleste to pass checks, yapf -r vllm tests should do the trick

This was referenced Jan 9, 2024
@saattrupdan
Copy link

Bumping this as well, as we also need this to support Pydantic 2.x in our integrations.

Copy link
Collaborator

@zhuohan123 zhuohan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! Can you make sure this change works with older version of pydantic as well?

@@ -100,7 +100,7 @@ def parse_args():
def create_error_response(status_code: HTTPStatus,
message: str) -> JSONResponse:
return JSONResponse(ErrorResponse(message=message,
type="invalid_request_error").dict(),
type="invalid_request_error").model_dump(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 @kmcleste can you fix this?

@simon-mo
Copy link
Collaborator

Thank you for your contribution. I will close this in favor of #2531

@simon-mo simon-mo closed this Jan 21, 2024
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

Successfully merging this pull request may close these issues.

None yet

6 participants