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

[Serve] Optimize DeploymentDetails.deployment_route_prefix_not_set() #46305

Merged

Conversation

JoshKarpel
Copy link
Contributor

@JoshKarpel JoshKarpel commented Jun 27, 2024

Why are these changes needed?

We've identified this method as a hotspot when the Serve Controller is tracking a lot of apps, taking ~5% of total time at 3k apps. Serializing the whole object to check if a field is set is unnecessary in both Pydantic v1 and v2... if you're willing to use a dunder field in v1 :)

After this change, this check takes no significant time.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

This behavior didn't seem to be tested anywhere already, and it looks like it's just protecting a deprecation, so I didn't add a unit test, but I checked that it does have the desired behavior manually:

DeploymentDetails(
        name="foo",
        status=DeploymentStatus.HEALTHY,
        status_trigger=DeploymentStatusTrigger.UNSPECIFIED,
        message="m",
        deployment_config=DeploymentSchema(
            name="foo",
            route_prefix="/foo",
        ),
        target_num_replicas=1,
        replicas=[],
    )
pydantic.error_wrappers.ValidationError: 1 validation error for DeploymentDetails
deployment_config
  Unexpectedly found a deployment-level route_prefix in the deployment_config for deployment "foo". The route_prefix in deployment_config within DeploymentDetails should not be set; please set it at the application level. (type=value_error)

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?

@JoshKarpel JoshKarpel marked this pull request as ready for review June 27, 2024 22:18
@@ -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

Choose a reason for hiding this comment

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

If I understand

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

@@ -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.

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

user models can be pydantic v2

@edoakes edoakes enabled auto-merge (squash) June 28, 2024 18:34
@github-actions github-actions bot added the go add ONLY when ready to merge, run all tests label Jun 28, 2024
@edoakes edoakes merged commit 85f9a08 into ray-project:master Jun 28, 2024
8 checks passed
@JoshKarpel JoshKarpel deleted the optimize-deployment_route_prefix_not_set branch June 28, 2024 20:05
@JoshKarpel
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants