Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Update pydantic to >= 2 #15979

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

dvzrv
Copy link

@dvzrv dvzrv commented Jul 23, 2023

Adapt codebase to use pydantic >= 2 models and functionalities.

Remove unneeded checks from scripts-dev/check_pydantic_models.py, since pydantic can now be used in a strict mode which will prevent the type coercion:
https://docs.pydantic.dev/2.0/usage/strict_mode/#type-coercions-in-strict-mode

Closes #15858

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@dvzrv dvzrv requested a review from a team as a code owner July 23, 2023 14:15
@dvzrv dvzrv force-pushed the dvzrv/pydantic_v2 branch 3 times, most recently from 8618ca9 to a465aa8 Compare July 24, 2023 22:18
@dvzrv
Copy link
Author

dvzrv commented Jul 24, 2023

It seems further pydantic v1 changes had been added. I have adapted the code now and hope it'll work out this time :)

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Thank you for looking into this; it looks like a very helpful and comprehensive overview to using Pydantic 2.X.

I've left some comments, but I'm not sure how we proceed, to be honest, since I think that other packagers aren't able to use Pydantic 2.x.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this file if we use strict=True everywhere? I guess its purpose now becomes enforcing that we do use strict=True everywhere?

Copy link
Author

Choose a reason for hiding this comment

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

I am honestly not sure. The improvements with pydantic v2 seem to hint at it at least.
The documentation is in part not up-to-date though.

Comment on lines 206 to +214
# This is the most recent version of Pydantic with available on common distros.
# We are currently incompatible with >=2.0.0: (https://github.com/matrix-org/synapse/issues/15858)
pydantic = "^1.7.4"
pydantic = ">=2.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

We'd need to consult with other packagers here. From https://pkgs.org/search/?q=pydantic I would expect that >=2 will cause pain for other distros

Copy link
Author

Choose a reason for hiding this comment

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

In the context of our rebuild on Arch Linux I have opened a few PRs to fix the remaining projects that I am aware of (also my own).

I realize that for some this might be a bigger change. However, the alternative is using the v1, which in part has changed nonetheless and is not the same as using pydantic v1.

From a packager perspective I fear that not upgrading will leave us with a python-pydantic1 package, that needs to conflict with the python-pydantic package (so software that uses either can not be installed with the respective other).

Copy link
Author

Choose a reason for hiding this comment

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

As noted elsewhere, introducing a python-pydantic1 package seems out of the question, as it has to conflict with python-pydantic and synapse pulls in pydantic>2 via twisted -> inflect.

Copy link
Author

Choose a reason for hiding this comment

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

So far no other packager of any other distribution has chimed in to raise concerns.
From my perspective I do not see any issues with moving to pydantic v2, as distributions can adapt/update all other pydantic dependents (that I was able to check) appear to be compatible with v2 now (or are about to drop its use).

Copy link
Contributor

Choose a reason for hiding this comment

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

Re. other distributions:

I am the new pydantic maintainer in Fedora Linux. I am working on updating our development branch (Fedora 40) to pydantic v2, but Fedora 38 and 39 will stay on pydantic v1. It would be nice to add support for pydantic v2 while keeping support for pydantic v1 a little while longer so we can continue to update those releases.

/cc @V02460 who maintains our matrix-synapse package in case they have something to add.

synapse/config/_util.py Show resolved Hide resolved
elif isinstance(raw_error, PydanticValueError):
else:
errcode = Codes.INVALID_PARAM
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a behaviour change to me; it seems like you can compare to "value_error" per here

Copy link
Contributor

Choose a reason for hiding this comment

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

(though as noted below, maybe this is an improvement)

raw_errors = e.errors()
if len(raw_errors) == 1:
error_type = raw_errors[0].get("type")
if error_type == "missing":
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there some kind of constant we can use in place of "missing" here?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Frankly, I have a hard time understanding how to better look into this.
How do I get better log output for the servlet part of things? Where does the logger write to? It does not seem to end up in the general test log (_trial_temp/test.log).

Other than querying the PydanticKnownError type attribute I also don't know of a better way of getting to any identifier that would be helpful in comparing.

synapse/rest/client/devices.py Show resolved Hide resolved
Comment on lines 794 to 812
def test_add_email_no_at(self) -> None:
self._request_token_invalid_email(
"address-without-at.bar",
expected_errcode=Codes.BAD_JSON,
expected_errcode=Codes.INVALID_PARAM,
expected_error="Unable to parse email address",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a nice improvement, and presumably corresponds to the changes in validate_json_object. Do we know why this gave BAD_JSON before? (i.e. what is the type of exception caught by validate_json_object here?)

Copy link
Author

Choose a reason for hiding this comment

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

My hope is that I didn't break any valid assumptions with this, but looking at the tests it seemed correct.

@dvzrv
Copy link
Author

dvzrv commented Jul 25, 2023

I'm yet clueless as to how to fix https://github.com/matrix-org/synapse/actions/runs/5650365015/job/15307598753?pr=15979. Maybe @H-Shay, as the author of that change I've rebased on top, has some input on it? 👀 😸

@dvzrv
Copy link
Author

dvzrv commented Jul 30, 2023

Meanwhile this is now the only blocking package for our pydantic rebuild on Arch Linux.

It seems introducing a python-pydantic1 package (providing pydantic < 2) is also not possible, as it would need to conflict with python-pydantic (providing pydantic > 2) and synapse pulls in python-pydantic (>2) via twisted -> inflect.

@dvzrv
Copy link
Author

dvzrv commented Sep 10, 2023

I'm yet clueless as to how to fix https://github.com/matrix-org/synapse/actions/runs/5650365015/job/15307598753?pr=15979. Maybe @H-Shay, as the author of that change I've rebased on top, has some input on it? 👀 😸

Ping @H-Shay ?!

Adapt codebase to use pydantic >= 2 models and functionalities.

Remove unneeded checks from `scripts-dev/check_pydantic_models.py`,
since pydantic can now be used in a strict mode which will prevent the
type coercion:
https://docs.pydantic.dev/2.0/usage/strict_mode/#type-coercions-in-strict-mode

Closes matrix-org#15858

Signed-off-by: David Runge <[email protected]>
@clokep
Copy link
Member

clokep commented Sep 11, 2023

I've left some comments, but I'm not sure how we proceed, to be honest, since I think that other packagers aren't able to use Pydantic 2.x.

I think this is the crux here -- it seems unlikely we'll want to update to pydantic 2 extremely quickly unless the benefits are really massive.

I wonder if an interim way forward is to try using the transitional bits (#15858 (comment)) and land that as a separate PR.

@clokep
Copy link
Member

clokep commented Sep 25, 2023

Note that #16332 added support for both pydantic v1 & v2. I'm unsure if we want to continue down this path currently or not.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Synapse does not take advantage of Pydantic 2.0
4 participants