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

feat: upgrade to new fastapi, update models to handle both pydantic v… #3374

Merged
merged 7 commits into from
Mar 3, 2024

Conversation

timothyjlaurent
Copy link
Contributor

…1 an v2, add matrix conditions for pydantic_v1 and pydantic_v2

What this PR does / why we need it:
This work builds on this PR #3273 that adds pydantic 2 support to the kserve package.

The following is copied from that PR:

  • 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'] = ()`.

In addition to handling those incompatibilities it also allows for both pydantic 1.x and 2.x versions.

To accomplish this:

  • FastAPI is upgraded to the latest version. This version supports both pydantic 1.x and 2.x. As such, projects that add kserve as a dependency will also be able to use both pydantic versions.
  • inspired by how https://github.com/docarray/docarray added support for v1 and v2 pydantic an is_pydantic_2 flag is used to change the underlying pydantic implementations
  • each pydantic model that has incompatible syntax, ie nested Config class in retains the class if it is pydantic 1 and uses the model_config syntax if it is pydantic 2. Keys are changed where needed, eg schema_extra -> json_schema_extra.
  • protected_namespace=() is set on the InferenceResponse model
  • to test, again inspired by docarray's CI, a new matrix component pydantic-version is added to CI. As the default version will be pydantic 2, in the case that the version is pydantic_1 then the make target install_pydantic_1 is executed which downgrades pydantic to v1.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3373

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

Feature/Issue validation/testing:

Please describe the tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

  • Added matrix to test both pydantic 1 and pydantic 2

  • Logs

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.

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:

Upgrades FastAPI version and now supports Pydantic v1.x and v2.x. This allows dependent projects to upgrade their Pydantic version as needed.

@sivanantha321
Copy link
Member

@timothyjlaurent Can you help rebase the PR and sign off the commits ?

@timothyjlaurent
Copy link
Contributor Author

@sivanantha321 I've rebased and signed all the commits, can you take another look?

…dependencies

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>
@sivanantha321
Copy link
Member

@yuzisun Can you help trigger the tests ?

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>
@timothyjlaurent
Copy link
Contributor Author

We had a small bug in the last test and then I somehow borked my rebase -- I've cleaned up the commits on a fresh branch and think it should pass now.

@sivanantha321
Copy link
Member

@timothyjlaurent I don't see a updated version of pydantic in poetry.lock file ? I think we can upgrade the pydantic version to >2.0.0 for model servers.

@timothyjlaurent
Copy link
Contributor Author

@sivanantha321 Ah this is because we hack/python-release runs with --no-update and the sdk is compatible with pydantic v1 or v2.

I can either run it to update all packages, or I could add pydantic as an explicit dep to each model server. What do you think?

@sivanantha321
Copy link
Member

@sivanantha321 Ah this is because we hack/python-release runs with --no-update and the sdk is compatible with pydantic v1 or v2.

I can either run it to update all packages, or I could add pydantic as an explicit dep to each model server. What do you think?

You can update the packages as long as it does not breaks anything

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>
@timothyjlaurent
Copy link
Contributor Author

@sivanantha321 I've updated the deps.

@yuzisun
Copy link
Member

yuzisun commented Feb 25, 2024

@timothyjlaurent Thanks for upgrading pydantic! Can you help take a look a look at the testing failures?
https://github.com/kserve/kserve/actions/runs/7993801929/job/21947239919?pr=3374

@timothyjlaurent
Copy link
Contributor Author

@timothyjlaurent Thanks for upgrading pydantic! Can you help take a look a look at the testing failures? https://github.com/kserve/kserve/actions/runs/7993801929/job/21947239919?pr=3374

I took the time to figure out how to properly run the test suite locally -- needs to be done from the ./python directory.

I've updated the tests one notable workaround is that the RayServe test asserted b'{"name":"TestModel","ready":"True"}' <- notice the stringified bool. In pydantic V2 we correctly get an actual json bool for the ready param. I added a special case to the test and left comments explaining this behavior.

assert resp.content == b'{"name":"TestModel","ready":"True"}'
# for some reason the RayServer responds with the stringified python bool
# when run on pydantic < 2 and the bool when run on pydantic >= 2
# eg {"name":"TestModel","ready":"True"} vs {"name":"TestModel","ready":true}
Copy link
Member

Choose a reason for hiding this comment

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

I think it is a different bug altogether. Before moving to fastapi, it seems the health handler returned bool values,
but after the migration it started returning string values because how the type is defined ?
@yuzisun

Copy link
Member

Choose a reason for hiding this comment

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

The code seems to be returning bool, as @timothyjlaurent mentioned the issue is for the rayserve case?

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 is for the RayServe case.

In the previous assertion with pydantic v1 - we were getting {"name":"TestModel","ready":"True"} the string value, and testing for that. Seems like it was a bug that got "blessed" by enshrinining it in a testcase.
With the pyanticv2 we're (correctly) getting a bool value for the ready key (not a stringified python string) - This, I believe is the correct behavior we should expect.

@yuzisun
Copy link
Member

yuzisun commented Mar 3, 2024

@timothyjlaurent seems like it keeps failing after updating the poetry lock file
https://github.com/kserve/kserve/actions/runs/8087460553/job/22214167194?pr=3374

yuzisun added 2 commits March 3, 2024 03:13
Signed-off-by: Dan Sun <dsun20@bloomberg.net>
Signed-off-by: Dan Sun <dsun20@bloomberg.net>
@yuzisun
Copy link
Member

yuzisun commented Mar 3, 2024

Tests passed, Thanks @timothyjlaurent for the great work!

/lgtm
/approve

Copy link

oss-prow-bot bot commented Mar 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: timothyjlaurent, yuzisun

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

The pull request process is described 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

@oss-prow-bot oss-prow-bot bot added the approved label Mar 3, 2024
@yuzisun yuzisun merged commit 426fe21 into kserve:master Mar 3, 2024
58 checks passed
tjandy98 pushed a commit to tjandy98/kserve that referenced this pull request Apr 10, 2024
kserve#3374)

* feat: add support for pydantic v2, tests for pydantic v1, and update dependencies

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>

* feat: regenerate lock files

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>

* feat: regenerate lock files with updates

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>

* fix: update dependencies, fix broken tests

* feat: add special case for testing model ready, "True" for pydantic v1

* Update poetry lock

Signed-off-by: Dan Sun <dsun20@bloomberg.net>

---------

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>
Signed-off-by: Dan Sun <dsun20@bloomberg.net>
Co-authored-by: Dan Sun <dsun20@bloomberg.net>
Signed-off-by: tjandy98 <3953059+tjandy98@users.noreply.github.com>
cmaddalozzo pushed a commit to cmaddalozzo/kserve that referenced this pull request Apr 19, 2024
kserve#3374)

* feat: add support for pydantic v2, tests for pydantic v1, and update dependencies

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>

* feat: regenerate lock files

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>

* feat: regenerate lock files with updates

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>

* fix: update dependencies, fix broken tests

* feat: add special case for testing model ready, "True" for pydantic v1

* Update poetry lock

Signed-off-by: Dan Sun <dsun20@bloomberg.net>

---------

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>
Signed-off-by: Dan Sun <dsun20@bloomberg.net>
Co-authored-by: Dan Sun <dsun20@bloomberg.net>
cmaddalozzo pushed a commit to cmaddalozzo/kserve that referenced this pull request Apr 19, 2024
kserve#3374)

* feat: add support for pydantic v2, tests for pydantic v1, and update dependencies

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>

* feat: regenerate lock files

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>

* feat: regenerate lock files with updates

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>

* fix: update dependencies, fix broken tests

* feat: add special case for testing model ready, "True" for pydantic v1

* Update poetry lock

Signed-off-by: Dan Sun <dsun20@bloomberg.net>

---------

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>
Signed-off-by: Dan Sun <dsun20@bloomberg.net>
Co-authored-by: Dan Sun <dsun20@bloomberg.net>
Signed-off-by: Curtis Maddalozzo <cmaddalozzo@bloomberg.net>
yuzisun added a commit that referenced this pull request Apr 21, 2024
* feat: upgrade to new fastapi, update models to handle both pydantic v… (#3374)

* feat: add support for pydantic v2, tests for pydantic v1, and update dependencies

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>

* feat: regenerate lock files

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>

* feat: regenerate lock files with updates

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>

* fix: update dependencies, fix broken tests

* feat: add special case for testing model ready, "True" for pydantic v1

* Update poetry lock

Signed-off-by: Dan Sun <dsun20@bloomberg.net>

---------

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>
Signed-off-by: Dan Sun <dsun20@bloomberg.net>
Co-authored-by: Dan Sun <dsun20@bloomberg.net>
Signed-off-by: Curtis Maddalozzo <cmaddalozzo@bloomberg.net>

* Remove python 3.8

---------

Signed-off-by: Timothy Laurent <tlaurent@genedx.com>
Signed-off-by: Dan Sun <dsun20@bloomberg.net>
Signed-off-by: Curtis Maddalozzo <cmaddalozzo@bloomberg.net>
Co-authored-by: Timothy J Laurent <timothyjlaurent@gmail.com>
Co-authored-by: Dan Sun <dsun20@bloomberg.net>
wmfgerrit pushed a commit to wikimedia/machinelearning-liftwing-inference-services that referenced this pull request Apr 23, 2024
Bump kserve to 0.12.1 that includes the following fixes we need:

* support for pydantic v2 (kserve/kserve#3374) which is used by knowledge_integrity v0.6
* fix for ray serve compatibility issue (kserve/kserve#3556).

Bug: T363127
Change-Id: I3fd7c5963c647ab1f407f21c4bd9e2b530fe8a47
wmfgerrit pushed a commit to wikimedia/machinelearning-liftwing-inference-services that referenced this pull request Apr 23, 2024
Bump kserve to 0.12.1 that includes the following fixes we need:

* support for pydantic v2 (kserve/kserve#3374) which is used by knowledge_integrity v0.6
* fix for ray serve compatibility issue (kserve/kserve#3556).

Bug: T363129
Change-Id: I6a4babe2155b0638beb83a0a03af99ef396a666b
wmfgerrit pushed a commit to wikimedia/machinelearning-liftwing-inference-services that referenced this pull request Apr 23, 2024
Bump kserve to 0.12.1 that includes the following fixes we need:

* support for pydantic v2 (kserve/kserve#3374) which is used by knowledge_integrity v0.6
* fix for ray serve compatibility issue (kserve/kserve#3556).

Bug: T363130
Change-Id: I9b13d5235b2c52cc71d92db19fe3adc7cdafea1a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support pydantic 2
5 participants