Update pydantic to >= 2 by dvzrv · Pull Request #15979 · matrix-org/synapse

dvzrv · 2023-07-23T14:15:24Z

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 · 2023-07-24T22:19:14Z

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

DMRobertson

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.

DMRobertson · 2023-07-24T22:51:38Z

scripts-dev/check_pydantic_models.py

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?

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.

DMRobertson · 2023-07-24T22:55:06Z

pyproject.toml

 # 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"


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

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

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.

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

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

DMRobertson · 2023-07-24T23:08:14Z

synapse/http/servlet.py

                errcode = Codes.MISSING_PARAM
-            elif isinstance(raw_error, PydanticValueError):
+            else:
                errcode = Codes.INVALID_PARAM


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

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

DMRobertson · 2023-07-24T23:08:35Z

synapse/http/servlet.py

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


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

I also only found https://github.com/pydantic/pydantic-core/blob/f5ef7afbf3d0db1126f12557c10a4c1f6cf6b6c6/python/pydantic_core/core_schema.py#L3824 and https://docs.pydantic.dev/2.0/usage/validation_errors/#missing, so I am not sure. Can the ErrorType literal be used for this?

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

DMRobertson · 2023-07-24T23:31:50Z

tests/rest/client/test_account.py

    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",
        )


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?)

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

dvzrv · 2023-07-25T09:51:48Z

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 · 2023-07-30T16:59:16Z

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 · 2023-09-10T08:14:54Z

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 <dave@sleepmap.de>

clokep · 2023-09-11T18:35:06Z

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 · 2023-09-25T15:41:07Z

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

dvzrv requested a review from a team as a code owner July 23, 2023 14:15

dvzrv force-pushed the dvzrv/pydantic_v2 branch 3 times, most recently from 8618ca9 to a465aa8 Compare July 24, 2023 22:18

DMRobertson reviewed Jul 24, 2023

View reviewed changes

DMRobertson mentioned this pull request Aug 1, 2023

Add dict instance check before accessing it #15944

Closed

4 tasks

TinfoilSubmarine mentioned this pull request Aug 22, 2023

[WIP] python3-pydantic: update to 2.2.1. void-linux/void-packages#45722

Closed

dvzrv force-pushed the dvzrv/pydantic_v2 branch from a465aa8 to d46dd59 Compare September 10, 2023 08:44

matrixbot mentioned this pull request Dec 22, 2023

Update pydantic to >= 2 element-hq/synapse#15979

Closed

4 tasks

Uh oh!

Conversation

dvzrv commented Jul 23, 2023

Pull Request Checklist

Uh oh!

dvzrv commented Jul 24, 2023

Uh oh!

DMRobertson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvzrv commented Jul 25, 2023

Uh oh!

dvzrv commented Jul 30, 2023

Uh oh!

dvzrv commented Sep 10, 2023

Uh oh!

clokep commented Sep 11, 2023

Uh oh!

clokep commented Sep 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants