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

fix: compatibility with pydantic v2 #3273

Conversation

dagondagon666
Copy link

@dagondagon666 dagondagon666 commented Nov 29, 2023

What this PR does / why we need it:

  • Follows Optional fields guidance for pydantic v2 ref
  • Overwrites protected_namespace in pydantic v2 to get rid of warning logs about namespace conflicts in pydantic ref. An example is shown as follows
E   UserWarning: Field "model_name" has conflict with protected namespace "model_".
E
E   You may be able to resolve this warning by setting `model_config['protected_namespaces'] = ()`.

Type of changes
Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Special notes for your reviewer:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
    No image version increase

Checklist:

  • Have you added unit/e2e tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?

Release note:

NONE

@kserve-oss-bot
Copy link
Collaborator

kserve-oss-bot commented Nov 29, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: *dagondagon666* To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **yuzisun** after the PR has been reviewed. You can assign the PR to them by writing `/assign @yuzisun` in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

oss-prow-bot bot commented Nov 29, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dagondagon666
Once this PR has been reviewed and has the lgtm label, please assign yuzisun for approval by writing /assign @yuzisun in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dagondagon666
Copy link
Author

/assign @yuzisun

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Could you add a test for this?

@@ -14,7 +14,7 @@
from typing import Optional, List, Union, Dict

import orjson
from pydantic import BaseModel
from pydantic import BaseModel, ConfigDict
Copy link
Member

Choose a reason for hiding this comment

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

Should we surround the imports with try-catch in for backwards compatibility purposes?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do -- we should likely also likely add if/else for the the class definitions to switch from the nested Config class to model_config

outputs: List[ResponseOutput]
model_config = ConfigDict(protected_namespaces='')
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the model_config ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the new syntax for pydantic 2 that's analogous to the nested Config class in pydantic 1.

My guess is that Pydantic2 uses "model_" a protected namespace which we need for model_version

Copy link
Contributor

Choose a reason for hiding this comment

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

This is to suppress this warning:

E   UserWarning: Field "model_name" has conflict with protected namespace "model_".
E
E   You may be able to resolve this warning by setting `model_config['protected_namespaces'] = ()`.

So it really ought to be an empty tuple.

outputs: List[ResponseOutput]
model_config = ConfigDict(protected_namespaces='')

class Config:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pydantic 1 syntax.

We should move the schema_extra into the json_schema_extra field in the model_config above.

https://docs.pydantic.dev/latest/migration/#changes-to-config

@@ -183,7 +183,7 @@ class InferenceRequest(BaseModel):
"outputs" : [ $request_output, ... ] #optional
}
"""
id: Optional[str]
id: Optional[str] = None
parameters: Optional[Parameters] = None
inputs: List[RequestInput]
outputs: Optional[List[RequestOutput]] = None
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 replace Config with model_config if we want it to work in pydantic 2

@sivanantha321
Copy link
Member

closing in favor of #3374

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