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

Support both Pydantic v1 and v2 #24

Merged
merged 17 commits into from
Sep 19, 2023
Merged

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented Sep 3, 2023

Unpin pydantic in the requirements to allow for v2.

Added helper methods to utils in order to:

  • identify pydantic v1 or v2, and based on this:
  • use new method name .model_dump_json(), or old deprecated name .json(), for serialisation
  • use new method name .model_validate(), or old deprecated name .parse_obj(), for deserialisation
  • and for RpcResponse, use GenericModel in pydantic v1, or BaseModel in v2

@ff137
Copy link
Contributor Author

ff137 commented Sep 4, 2023

No changes required for migration, hence the PR name: "permit" v2.
But, use of GenericModel here may raise a deprecated warning if the environment is using v2. A fix for that would involve checking the pydantic version, and importing BaseModel instead in case of v2

@orweis
Copy link
Contributor

orweis commented Sep 4, 2023

Hi @ff137 ! thanks for this great contribution, I'll have someone form the team review soon.
I do think we're already seeing an issue with the tests - perhaps even the one you mentioned.

@ff137
Copy link
Contributor Author

ff137 commented Sep 4, 2023

Cool! Thanks.

GenericModel is logged as a deprecation warning, which can be fixed.
But, the error is complaining about import attr not found ... So maybe a dependency conflict. I'll take a closer look tomorrow 👌

Signed-off-by: ff137 <ff137@proton.me>
@ff137
Copy link
Contributor Author

ff137 commented Sep 6, 2023

@orweis I've looked again, and the error was import attr not found. I can't understand why, because pydantic has no dependencies impacting attr. And the only change in this PR is that it now installs pydantic 2.3.0.

Fortunately, that import is unused! So, I've cleaned up unused imports, and tests pass locally for me.

I just notice there will indeed be deprecation warnings that can spam logs for users.
One way to address that would be to require pydantic >2, and to rename the deprecated methods.
e.g. "The json method is deprecated; use model_dump_json instead", and "The parse_obj method is deprecated; use model_validate instead".
Trivial to fix those, but for environment compatibility, we'll require pinning pydantic >2. Which should be fine - 2.0 was released more than 2 months ago, so it's about time to switch over :-) and allow people to use fastapi_websocket_rpc with the latest pydantic

@ff137 ff137 marked this pull request as draft September 6, 2023 13:37
@roekatz roekatz self-requested a review September 12, 2023 11:34
@roekatz roekatz marked this pull request as ready for review September 12, 2023 11:34
@roekatz
Copy link
Contributor

roekatz commented Sep 12, 2023

@orweis I've looked again, and the error was import attr not found. I can't understand why, because pydantic has no dependencies impacting attr. And the only change in this PR is that it now installs pydantic 2.3.0.

Fortunately, that import is unused! So, I've cleaned up unused imports, and tests pass locally for me.

I just notice there will indeed be deprecation warnings that can spam logs for users. One way to address that would be to require pydantic >2, and to rename the deprecated methods. e.g. "The json method is deprecated; use model_dump_json instead", and "The parse_obj method is deprecated; use model_validate instead". Trivial to fix those, but for environment compatibility, we'll require pinning pydantic >2. Which should be fine - 2.0 was released more than 2 months ago, so it's about time to switch over :-) and allow people to use fastapi_websocket_rpc with the latest pydantic

@ff137 I'm convinced :)
Feel free to make the adjustments you've suggested (pinning pydantic >2, renaming the deprecated methods).

@ff137
Copy link
Contributor Author

ff137 commented Sep 12, 2023

@ff137 I'm convinced :) Feel free to make the adjustments you've suggested (pinning pydantic >2, renaming the deprecated methods).

Cool! Thank you. I had some ideas for supporting both v1 and v2 of pydantic. Quite inundated with other work atm, but I'll wrap this one up in the next few days

@ff137
Copy link
Contributor Author

ff137 commented Sep 17, 2023

So, the deprecated GenericModel usage can be handled quite simply:

import pydantic
from packaging import version
from pydantic import BaseModel

# Check pydantic version to handle deprecated GenericModel
if version.parse(pydantic.VERSION) < version.parse("2.0.0"):
    from pydantic.generics import GenericModel

    class RpcResponse(GenericModel, Generic[ResponseT]):
        result: ResponseT
        result_type: Optional[str]
        call_id: Optional[UUID] = None

else:

    class RpcResponse(BaseModel, Generic[ResponseT]):
        result: ResponseT
        result_type: Optional[str]
        call_id: Optional[UUID] = None

That should be the only adjustment necessary to support pydantic 2 without deprecation warnings.

Note: This requires packaging as an extra dependency. It can be avoided by using the built-in distutils library, but this is deprecated in python 3.10, so using packaging is recommended. It's just for parsing the pydantic version in a reliable way.

@ff137 ff137 marked this pull request as draft September 17, 2023 10:37
@ff137 ff137 changed the title Permit pydantic v2 Support both Pydantic v1 and v2 Sep 17, 2023
@ff137 ff137 marked this pull request as ready for review September 17, 2023 14:31
@ff137
Copy link
Contributor Author

ff137 commented Sep 17, 2023

@orweis @roekatz There we go! I added a way to support both pydantic v1 and v2, without deprecation warnings.
I tested locally with v1 and v2. Pytests pass without any warning logs.

Previously it complained about .json and .parse_obj, which are renamed in v2, and you can see how I implemented it in utils. Cleaned up some unused imports + applied black formatting along the way for the files I changed, because I'm pedantic like that ;-). Let me know if you have any suggestions or change requests

@ff137
Copy link
Contributor Author

ff137 commented Sep 17, 2023

A similar approach was followed to add support in the pubsub library: permitio/fastapi_websocket_pubsub#65

@orweis orweis requested a review from asafc September 18, 2023 13:46
Copy link
Contributor

@roekatz roekatz left a comment

Choose a reason for hiding this comment

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

@ff137 That's great!
Had one request and one suggestion, see my comments.
Otherwise let's merge that already :)

setup.py Outdated Show resolved Hide resolved
fastapi_websocket_rpc/utils.py Outdated Show resolved Hide resolved
@ff137
Copy link
Contributor Author

ff137 commented Sep 19, 2023

Updated and suggestions applied in _pubsub PR as well 👌

@roekatz roekatz merged commit 1138667 into permitio:master Sep 19, 2023
7 checks passed
@roekatz
Copy link
Contributor

roekatz commented Sep 19, 2023

Amazing! :)

@ff137 ff137 deleted the upgrade/pydantic-v2 branch September 20, 2023 08:58
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

3 participants