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.

Sign up for GitHub

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

Jump to bottom

[Serve] Optimize DeploymentDetails.deployment_route_prefix_not_set() #46305

Merged
merged 2 commits into from
Jun 28, 2024
Merged

[Serve] Optimize DeploymentDetails.deployment_route_prefix_not_set() #46305

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed05af3
optimize DeploymentDetails.deployment_route_prefix_not_set()
JoshKarpel Jun 27, 2024
75d7b2d
Merge branch 'master' into optimize-deployment_route_prefix_not_set
JoshKarpel Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions python/ray/serve/schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,10 +947,12 @@ class DeploymentDetails(BaseModel, extra=Extra.forbid, frozen=True):
def deployment_route_prefix_not_set(cls, v: DeploymentSchema):
# Route prefix should not be set at the deployment level. Deployment-level route
# prefix is outdated, there should be one route prefix per application
if "route_prefix" in v.dict(exclude_unset=True):
if (
"route_prefix" in v.__fields_set__
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to support both pydantic v1 and v2. Could you import

from ray._private.pydantic_compat import IS_PYDANTIC_2

and also add support for pydantic 2?

Copy link
Contributor Author

@JoshKarpel JoshKarpel Jun 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If I understand

ray/python/ray/_private/pydantic_compat.py

Lines 55 to 57 in 4ae8128

else:
IS_PYDANTIC_2 = True
from pydantic.v1 import (
correctly, it looks like when Pydantic v2 is installed, you're using the pydantic.v1 import to keep using the v1 API, so this call will be valid either way (I tested this code with Pydantic v2 installed and it worked as expected). But it would cause an issue when you switch to actually using Pydantic v2 models internally 🤔

... maybe this field will be fully deprecated by then and this validator will be deleted before the migration happens? 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

yea this should be fine because it's only touching our internal models

user models can be pydantic v2

): # in Pydantic v2, this becomes `in v.model_fields_set`
raise ValueError(
"Unexpectedly found a deployment-level route_prefix in the "
f'deployment_config for deployment "{cls.name}". The route_prefix in '
f'deployment_config for deployment "{v.name}". The route_prefix in '
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an unrelated bug - I suspect that either no one has ever hit this block, or they just accepted the AttributeError and figured out what was wrong some other way?

"deployment_config within DeploymentDetails should not be set; please "
"set it at the application level."
)
Expand Down
Loading